Parent domain owner can steal ownership and clear any fuses for any sub-domain if CANNOT_UNWRAP is not burnt on his own domain

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L954-L962

Vulnerability details

Impact

There is a general incorrect logic of burning fuses throughout NameWrapper, which allows parent domain owner to burn subdomain fuses (including PARENT_CANNOT_CONTROL) regardless of parent domain's own fuses (only subdomain fuses are checked, parent fuses are ignored). This opens possibility for the parent domain owner to unwrap parent domain and steal control of any subdomain via ENS registry, ignoring any subdomain ownerships and/or fuses and expiry set.

Stealing subdomain scenario:

1. Alice registers and wraps test.eth domain (with no fuses burnt and max expiry set)
2. Alice creates subdomain bob.test.eth and burns PARENT_CANNOT_CONTROL fuse with max expiry, transferring this domain to Bob (for example by calling NameWrapper.setSubnodeOwner)
3. At this point Bob can verify that he is indeed domain owner of bob.test.eth in NameWrapper, PARENT_CANNOT_CONTROL fuse is burnt for this domain and fuse expiry is set to expiry of test.eth domain. So Bob thinks his domain is secure and can not be taken from him before the expiry.
4. Alice unwraps test.eth domain.
5. Alice call EnsRegistry.setSubnodeOwner for bob.test.eth to herself.
6. Alice wraps bob.test.eth to herself, overwriting bob's ownership and any fuses and expiry to new (clear) ones.
7. At this point Alice has successfully stolen bob.test.eth domain ignoring any fuses, expiry and ownership set for that domain.

Proof of Concept

Copy this to test/wrapper and run:
yarn test test/wrapper/NameWrapperStealSubdomain.js

https://gist.github.com/panprog/aec67ebd8d6b976edf81cb97b41466e0

Recommended Mitigation Steps

It should be possible to burn node fuses only if parent's node PARENT_CANNOT_CONTROL and CANNOT_UNWRAP fuses are burnt.

All fuses checks go through NameWrapper._canFusesBeBurned, however adding parent node fuses check here is tricky, because parent node is not available here. Here is one possible fix, however it is quite gas heavy:

function _canFusesBeBurned(bytes32 node, uint32 fuses) internal view {

    // any fuses can be burnt only if parent node's PARENT_CANNOT_CONTROL and CANNOT_UNWRAP are set
    // or if parent node is ETH
    if (fuses != 0) {
        bytes memory name = names[node];
        (, uint256 offset) = name.readLabel(0);
        bytes32 parentNode = name.namehash(offset);

        if (parentNode != ETH_NODE) {
            (, uint32 parentFuses, ) = getData(uint256(parentNode));
            if (
                parentFuses & (PARENT_CANNOT_CONTROL | CANNOT_UNWRAP) !=
                (PARENT_CANNOT_CONTROL | CANNOT_UNWRAP)
            ) {
                revert OperationProhibited(node);
            }
        }

    }

    if (
        fuses & ~PARENT_CANNOT_CONTROL != 0 &&
        fuses & (PARENT_CANNOT_CONTROL | CANNOT_UNWRAP) !=
        (PARENT_CANNOT_CONTROL | CANNOT_UNWRAP)
    ) {
        revert OperationProhibited(node);
    }
}

Another solution could be to add a map from each node to parent node, which will increase gas usage when adding new nodes, but will reduce gas usage for the fuses burn check.

Either way, this should be more extensively checked and tested for the best solution.

[jefflau]

duplicate of #173