Allow duplicate proposal actions #5
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These values will never change, so setting them as constants makes auditing the contract a little easier.
The new ens registry has a fallback mechanism where the old registry will be queried if an owner for a record cannot be found in the new registry. The logic for this mechanism makes use to two sentinel values for the `owner` field in a record: - `address(0)`: no owner found in the new registry, try the old one - `address(this)`: record is unowned, do not fallback to the old registrar Calling `setOwner(record, address(0))` on the new registrar will in fact set a value of `address(this)` for the owner field on that record. the `recordExists` method only checks if the owner of a domain is `address(0)` and returns `true` otherwise. The combination of the above two points meant that revoked names (where the owner had been set to `address(0)`) could not be reregistered. This commit replaces the call to `recordExists` with a check based on `ens.owner(node)` (which returns `address(0)` when the owner of a record is `address(this)`), thus allowing reregistration of intentionally revoked names.
This check ensures that the owner of the "eth" tld in the ens registry has been set. However the call to `setDomainOwner` will always fail if this is not the case (due to the call to `transferFrom` later in the method). Removing as this is not a user facing method, and clarity of implementation trumps clarity of error messages in this case.
The `setDomainOwner` method should not set the resolver and ttl values. In particular the previous behaviour (setting the resolver to the address of the new domain owner) seems almost certain to be wrong (and cause disruption for downstream clients who may rely on the resolver for the `radicle.eth` domain).
Either sets a default value for the resolver and ttl, or allows the caller to specify a custom one when registering a new name.
This prevents an attack where a malicious actor could scan the mempool and front run name registration before charging the original registrant an extra fee.
The ENS registry has migrated before, and may do so again in the future, this change gives admin a little more flexibility in case such a situation arises again in the future.
This prevents an attack where a malicious actor could frontrun a call to `register` with an atomic call to `commit` followed by `register` for the same name. 50 blocks has been (somewhat arbitrarily) chosen as a default for now, some more work is required to justify the safety of this value in the face of e.g. heavy network congestion.
Co-authored-by: David Terry <xwvvvvwx@users.noreply.github.com>
- add commitWithPermit - fover RadicleToken over ERC20Burnable interface
Registrar fixes
|
Looks like you've added the hevm cache as a submodule? I think that's why CI is failing... |
remove treasury test
|
I guess this needs to be rebased on latest master to get both CI jobs passing as well. Code changes seem sensible. Have you tried running the existing gov tests against this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.