code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L187-L190 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/root/Root.sol#L22-L28 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/root/Root.sol#L34-L37

Vulnerability details

Proof of Concept

When claiming a domain in DNSRegistrar.sol (either through proveAndClaim() or proveAndClaimWithResolver()), the internal function _enableNode() will be called. The snippet bellow shows that when owner == address(0) or owner == previousRegistrar and parentNode == bytes(0). the Root.sol contract will be called by calling root.setSubnodeOwner(label, address(this)).

if (owner == address(0) || owner == previousRegistrar) {
    if (parentNode == bytes32(0)) {
        Root root = Root(ens.owner(bytes32(0)));
        root.setSubnodeOwner(label, address(this));

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L187-L190

And we can see that Root.sol will check if the label is not locked.

function setSubnodeOwner(
    bytes32 label,
    address owner
) external onlyController {
    require(!locked[label]);
    ens.setSubnodeOwner(ROOT_NODE, label, owner);
}

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/root/Root.sol#L22-L28

The issue is that there's only one function to toggle the lock to true, and there's no method currently on unlock a label.

function lock(bytes32 label) external onlyOwner {
    emit TLDLocked(label);
    locked[label] = true;
}

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/root/Root.sol#L34-L37

Impact

There are two scenarios where not being able to unlock a label becomes a problem.

the owner accidentally locks a label
an attacker takes control of the owner address and locks a label on purpose

Either one of these two scenarios are irreversible and can prevent particular domains of being claimed forever. Not being able to claim a DNS domain indefinitely would damage the ENS system.

Although Root.sol is not in scope for this contest, since it's called by DNSRegistrar.sol (in scope), locking a label will impact functionalities related to the logic in scope.

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to allow setting the locked mapping to false in Root.sol, e.g.

function unlock(bytes32 label) external onlyOwner {
    emit TLDUnlocked(label);
    locked[label] = false;
}

c4-pre-sort commented 1 year ago

thereksfour marked the issue as primary issue

c4-sponsor commented 1 year ago

Arachnid marked the issue as sponsor disputed

Arachnid commented 1 year ago

This is by design; locking is intended to be an irrevocable process. It's only available to the root, not to TLD owners.

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

dmvt commented 1 year ago

feature, not bug

code-423n4 / 2023-04-ens-findings