Registrar fixes #3

d-xo · 2021-02-05T16:41:15Z

This should address all the issues / improvements with the registrar we found in the report. In summary:

removed unused code
added logs for all state changes
account for ens fallback
do not set resolver and ttl when migrating to a new registrar
allow admin control over resolver & ttl values
set resolver and ttl for subdomains
give admin control over the ens registry address
introduce a commit / reveal scheme for name registration

The changes here are relatively substantial (+149/-83) in the registrar, so this needs a lot more testing and review.

Most of the above changes are self contained in their own commit, so you might find it helpful to review commit by commit.

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.

contracts/Registrar.sol

MrChico · 2021-02-05T16:45:28Z

contracts/Registrar.sol

+        commited[commitment] = block.number;
+    }
+
+    function mkCommitment(


this can live in the Registrar imo

contracts/Registrar.sol

cloudhead · 2021-02-05T17:06:47Z

contracts/Registrar.sol

-    uint256 public registrationFeeUsd = 10;
+    /// The minimum number of blocks that must have passed between a commitment and name registration
+    // TODO: justify this as a default value...
+    uint256 public minCommitmentAge = 50;


What is the purpose of having a minimum greater than 1? Is it in case of a re-org? Perhaps worth documenting.

yes, to cover network congestion, re-orgs, or cases where the register tx was submitted with a very low gas fee.

Definately agree re: docs, was planning to write up a deeper analysis of the commit / reveal scheme in the audit report.

cloudhead · 2021-02-05T17:08:44Z

contracts/Registrar.sol

+    /// Commit to a future name registration
+    function commit(bytes32 commitment) external {
+        require(commitments.commited(commitment) == 0, "Registrar::commit: already commited");
+        require(rad.balanceOf(msg.sender) >= fee, "Registrar::register: insufficient rad balance");

        rad.burnFrom(msg.sender, fee);


Ah so the burn happens on commit, not on reveal. I guess this means commitments shouldn't expire, right? (Which I believe they don't)

yes, commitments currently do not expire.

cloudhead · 2021-02-05T17:13:24Z

contracts/Registrar.sol

    }

-    function _register(bytes32 label, address owner) private {
-        bytes32 node = namehash(domain, label);
+    /// Register a subdomain (with a custom resolver and ttl)


So this function can be called by anyone. That could be useful if we want to bare the costs.

cloudhead · 2021-02-05T17:14:08Z

contracts/Registrar.sol

-
-        return valid(name) && !ens.recordExists(node);
+        bytes32 node = namehash(radNode, label);
+        return ens.owner(node) == address(0);


Perhaps worth documenting why we don't use recordExists?

cloudhead · 2021-02-05T17:19:27Z

Looks good to me so far.

- add commitWithPermit - fover RadicleToken over ERC20Burnable interface

CodeSandwich

Looks great! I only have a few nitpicks

CodeSandwich · 2021-02-08T09:29:55Z

contracts/Registrar.sol


    /// The token ID for the node in the `eth` TLD, eg. sha256("radicle").
-    uint256 public immutable tokenId;
+    uint256 public constant tokenId = uint(keccak256(abi.encodePacked("radicle")));


We're hardcoding the full radicle.eth domain here, is this necessary? It removes deployment elasticity making test deployments on real networks (ropsten/mainnet) impossible.

CodeSandwich · 2021-02-08T09:41:17Z

contracts/Registrar.sol


    /// Registration fee in *Radicle* (uRads).
-    uint256 public registrationFeeRad = 1e18;
+    uint256 public fee = 1e18;


This DeScRiPtIvE VaRiAbLe NaEms is a part of the public API, it probably should be both descriptive and future-proof without renaming, we are considering different payment schemes.

CodeSandwich · 2021-02-08T11:32:27Z

contracts/Registrar.sol

+    }
+
+    /// Set the address for the ENS registry
+    function setEns(address newEns) public adminOnly {


Can we realistically expect this to be ever used? If the ENS is moved, it's probably due to some API changes, which will require a full registrar update anyway. If we remove this function, we'll save some gas on accessing the ens variable.

I'm inclined to agree. If another migration were to happen it could be dealt with by transferring ownership to a new Registrar.

…gain

MrChico · 2021-02-08T15:41:21Z

Your comments should now be addressed @CodeSandwich

d-xo added 16 commits February 5, 2021 14:34

registrar: remove unused code

8b2067b

registrar: make immutable variables constant

9355838

These values will never change, so setting them as constants makes auditing the contract a little easier.

registrar: log on all state changes

3f90793

registrar: formatting & naming consistency

6090129

registrar: naming consistency

fcac3df

registrar: reinstate admin control over the fee

1f40518

registrar: allow admin control over resolver and ttl values

0536426

registrar: set resolver and ttl for subdomains

c63d86c

Either sets a default value for the resolver and ttl, or allows the caller to specify a custom one when registering a new name.

registrar: naming

39255a4

registrar: introduce commit / reveal scheme

8cf11c7

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.

registrar: give admin control over ens address

fa1cd8f

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.

registrar: adapt tests

c14a88a

MrChico reviewed Feb 5, 2021

View reviewed changes

contracts/Registrar.sol Outdated Show resolved Hide resolved

registrar: remove safemath

622cab2

MrChico reviewed Feb 5, 2021

View reviewed changes

contracts/Registrar.sol Outdated Show resolved Hide resolved

MrChico reviewed Feb 5, 2021

View reviewed changes

contracts/Registrar.sol Outdated

commited[commitment] = block.number;

}

function mkCommitment(

Copy link

MrChico Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can live in the Registrar imo

registrar: remove custom resolver / ttl

ececfe0

MrChico reviewed Feb 5, 2021

View reviewed changes

contracts/Registrar.sol Outdated Show resolved Hide resolved

d-xo added 2 commits February 5, 2021 17:50

registrar: make register external

a127bef

registrar: remove debugging code

8516530

cloudhead reviewed Feb 5, 2021

View reviewed changes

registrar: update comments

9ce8d22

cloudhead reviewed Feb 5, 2021

View reviewed changes

MrChico added 2 commits February 6, 2021 11:52

- inline mkCommitment

8198113

- add commitWithPermit - fover RadicleToken over ERC20Burnable interface

reuse commit function in commitwithpermit

31ee950

CodeSandwich reviewed Feb 8, 2021

View reviewed changes

MrChico marked this pull request as ready for review February 8, 2021 11:48

MrChico added 2 commits February 8, 2021 13:31

0 tx registration flow

6da8bae

allow tokenId, radNode to be set in constructor. Make ens immutable a…

1abd66e

…gain

d-xo merged commit 11b7dde into master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registrar fixes #3

Registrar fixes #3

d-xo commented Feb 5, 2021 •

edited

Loading

MrChico Feb 5, 2021

cloudhead Feb 5, 2021

d-xo Feb 5, 2021 •

edited

Loading

cloudhead Feb 5, 2021

d-xo Feb 5, 2021 •

edited

Loading

cloudhead Feb 5, 2021 •

edited

Loading

cloudhead Feb 5, 2021

cloudhead commented Feb 5, 2021

CodeSandwich left a comment

CodeSandwich Feb 8, 2021

CodeSandwich Feb 8, 2021

CodeSandwich Feb 8, 2021

MrChico Feb 8, 2021

MrChico commented Feb 8, 2021

Registrar fixes #3

Registrar fixes #3

Conversation

d-xo commented Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-xo Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-xo Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

cloudhead Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudhead commented Feb 5, 2021

CodeSandwich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrChico commented Feb 8, 2021

d-xo commented Feb 5, 2021 •

edited

Loading

d-xo Feb 5, 2021 •

edited

Loading

d-xo Feb 5, 2021 •

edited

Loading

cloudhead Feb 5, 2021 •

edited

Loading