Default reverse resolver

[makoto]

• Add DefaultResolver
• Add SignatureResolver which is the super class of DefaultResolver and L2ReverseRegistrar
• Change signature to use eip 191 version 0

[Arachnid]

I don't think this works with signed messages; someone replaying a series of messages could choose to include or omit the clear signed message, producing different results either way.

Perhaps instead we can include a version in each signature message, and if the version is greater than the one stored, increment it, effectively erasing all past records?

[makoto]

I am failing to understand why this is only issue for clearRecords but not for other set* functions. Also the inclusion of the version doesn't help when the someone keeps omitting to include the clear signed message.
Isn't inceptionDate enough against most abuses?

[Arachnid]

You're right; to be properly secure, inception dates would need to be recorded on each individual record (in the case of parameterised values such as text, on each key of each record); otherwise someone can replay just the last event, making it impossible to set earlier ones.

If someone omits a message, someone else can supply it - the issue is cases where omitting an earlier message but including a later one makes it impossible to supply the missing message later and arrive at the same end result.

[makoto]

otherwise someone can replay just the last event, making it impossible to set earlier ones. That would be problematic if we have a counter logic (eg: n=n+1) where the state depends on previous state. However, all setter* message simply overrides the previous state so there isn't much point supplying the missing message later.

I also didn't fully understand for inception dates would need to be recorded on each individual record

Are you suggesting to change from

    function _setLastUpdated(bytes32 node, uint256 inceptionDate) internal {
        lastUpdated[node] = inceptionDate;
    }

to

    function _setTextLastUpdated(bytes32 node, string key, uint256 inceptionDate) internal {
        lastUpdated[node][key] = inceptionDate;
    }

then change isAuthorisedWithSignature to take key ?

[Arachnid]

That would be problematic if we have a counter logic (eg: n=n+1) where the state depends on previous state. However, all setter* message simply overrides the previous state so there isn't much point supplying the missing message later.

They don't, though - setting the address for one coin type invalidates all other previous messages to set addresses for other coin types. The same goes for text records.

Either the last updated value needs to be per-key as you illustrate, or we need to change this to function differently - for example, by taking a complete set of records to set in a single operation, and increment the version at the same time. If going with the former, probably the last updated check should be moved outside isAuthorisedWithSignature.

[makoto]

Okay, it's getting a bit more complicating than simple fixes. You have been pointing out many changes to the functions not related to this PR but introduced at #265 (you only noticed because I moved the code to different file).

May I suggest to leave it for the separate PR by @jefflau

[makoto]

I think it should be incorporated, there's no reason for it to be a separate parameter here. at #335 (comment) , addr is used to derive node at bytes32 node = _getNamehash(addr); line 73 @Arachnid

[Arachnid]

You're right; to be properly secure, inception dates would need to be recorded on each individual record (in the case of parameterised values such as text, on each key of each record); otherwise someone can replay just the last event, making it impossible to set earlier ones.

If someone omits a message, someone else can supply it - the issue is cases where omitting an earlier message but including a later one makes it impossible to supply the missing message later and arrive at the same end result.

[Arachnid]

I think it should be incorporated, there's no reason for it to be a separate parameter here. at #335 (comment) , addr is used to derive node at bytes32 node = _getNamehash(addr); line 73 @Arachnid

Line 73 of what file? I can't see anywhere that would require this to be a separate parameter to the outer signature.