ENS contest - panprog's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L820-L821 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L524 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L572

Vulnerability details

Impact

Due to re-entrancy possibility in NameWrapper._transferAndBurnFuses (called from setSubnodeOwner and setSubnodeRecord), it is possible to do some stuff in onERC1155Received right after transfer but before new owner and new fuses are set. This makes it possible, for example, to unwrap the subdomain, but owner and fuses will still be set even for unwrapped domain, creating fake ERC1155 NameWrapper token for domain, which is not owned by NameWrapper.

Fake token creation scenario:

1. Account1 registers and wraps test.eth domain
2. Account1 calls NameWrapper.setSubnodeOwner for sub.test.eth subdomain with Account1 as owner (to make NameWrapper owner of subdomain)
3. Contract1 smart contract is created, which calls unwrap in its onERC1155Received function, and a function to send sub.test.eth ERC1155 NameWrapper token back to Account1
4. Account1 calls NameWrapper.setSubnodeOwner for sub.test.eth with Contract1 as new owner, which unwraps domain back to Account1 but due to re-entrancy, NameWrapper sets fuses and ownership to Contract1
5. Account1 calls function to send ERC1155 token from Contract1 back to self.

After this sequence of events, sub.test.eth subdomain is owned by Account1 both in ENS registry and in NameWrapper (with fuses and expiry correctly set to the future date). Lots (but not all) of functions in NameWrapper will fail to execute for this subdomain, because they expect NameWrapper to have ownership of the domain in ENS, but some functions will still work, making it possible to make the impression of good domain.

At this point, ownership in NameWrapper is "detached" from ownership in ENS and Account1 can do all kinds of malcious stuff with its ERC1155 token. For example:

1. Sell subdomain to the other user, transfering ERC1155 to that user and burning PARENT_CANNOT_CONTROL to create impression that he can't control the domain. After receiving the payment, Account1 can wrap the domain again, which burns existing ownership record and replaces with the new one with clear fuses and Account1 ownership, effectively stealing domain back from unsuspecting user, who thought that ERC1155 gives him the right to the domain (and didn't expect that parent can clear fuses when PARENT_CANNOT_CONTROL is set).

2. Transfer subdomain to some other smart contract, which implements onERC1155Received, then take it back, fooling smart contract into believing that it has received the domain.

Proof of Concept

Copy these to test/wrapper and run: yarn test test/wrapper/NameWrapperReentrancy.js

https://gist.github.com/panprog/3cd94e3fbb0c52410a4c6609e55b863e

Recommended Mitigation Steps

Consider adding nonReentrant modifiers with ReentrancyGuard implementation from openzeppelin. Alternatively just fix this individual re-entrancy issue. There are multiple ways to fix it depending on expected behaviour, for example saving ERC1155 data and requiring it to match the data after transfer (restricting onERC1155Received to not change any data for the token received):

function _transferAndBurnFuses(
    bytes32 node,
    address newOwner,
    uint32 fuses,
    uint64 expiry
) internal {
    (address owner, uint32 saveFuses, uint64 saveExpiry) = getData(uint256(node));
    _transfer(owner, newOwner, uint256(node), 1, "");
    uint32 curFuses;
    uint64 curExpiry;
    (owner, curFuses, curExpiry) = getData(uint256(node));
    require(owner == newOwner && saveFuses == curFuses && saveExpiry == curExpiry);
    _setFuses(node, newOwner, fuses, expiry);
}

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.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L66-L70

Vulnerability details

Impact

Due to incorrect condition in ByteUtil.compare function, irrelevant characters are masked out only for strings shorter than 32 characters. However, they must be masked out for strings of all lengths in the last pass of the loop (when remainder of the string is 32 characters or less). This leads to incorrect comparision of strings longer than 32 characters where len or otherlen is smaller than string length (characters beyond provided length are still accounted for in the comparision in this case while they should be ignored).

This wrong compare behaviour also makes RRUtils.compareNames fail to correctly compare DNS names in certain cases.

While the BytesUtil.compare and RRUtils.compareNames methods are currently not used in the functions in the scope (but are used in mainnet's DNSSECImpl.checkNsecName, which is out of scope here), they're public library functions relied upon and can be used by the other users or the ENS project in the future. And since the functions in scope provide incorrect result, that's a wrong (unexpected) behaviour of the smart contract. Moreover, since the problem can be seen only with the large strings (more than 32 characters), this might go unnoticed with any code that uses compare or compareNames method and can potentially lead to high security risk of any integration project or ENS itself.

Example strings to compare which give incorrect result: 01234567890123450123456789012345ab 01234567890123450123456789012345aa

Each string is 34 characters long, first 33 characters are the same, the last one is different. If we compare first 33 characters of both strings, the result should be equal (as they only differ in the 34th character), but compare will return >, because it fails to ignore the last character of both strings and simply compares strings themselves.

If we compare the first 33 characters from the first string vs all 34 characters of the second string, the result of compare will be >, while the correct result is <, because compare fails to ignore the last character of the first string.

Example dns names to compare which give incorrect result: 01234567890123456789012345678901a0.0123456789012345678901234567890123456789012345678.eth 01234567890123456789012345678901a.0123456789012345678901234567890123456789012345678.eth

The first dns name should come after the second, but dnsCompare returns -1 (the first name to come before), because the length of the 2nd domain (49 characters) is ASCII character 1 and is not correctly masked off during strings comparision.

Proof of Concept

git diff

https://gist.github.com/panprog/32adefdc853ccd0fd0f1aad85c526bea

then:

yarn test test/dnssec-oracle/TestSolidityTests.js

Recommended Mitigation Steps

In addition to the incorrect condition, the mask calculation formula: 32 - shortest + idx will also overflow since shortest can be more than 32, so addition should be performed before subtractions.

            if (shortest - idx >= 32) {
                mask = type(uint256).max;
            } else {
                mask = ~(2 ** (8 * (idx + 32 - shortest)) - 1);
            }

Lines of code

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

Vulnerability details

Impact

There is a general incorrect logic of allowing to burn only PARENT_CANNOT_CONTROL fuse without burning CANNOT_UNWRAP fuse. If only PARENT_CANNOT_CONTROL fuse is burnt, then domain can be unwrapped by its owner and then wrapped again, which clears PARENT_CANNOT_CONTROL fuse, making it possible for parent to bypass the limitation of parent control before the expiry.

Bypassing parent control scenario:

1. Alice registers and wraps test.eth domain
2. Alice creates subdomain bob.test.eth and burns PARENT_CANNOT_CONTROL fuse with max expiry, transferring this domain to Bob
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. Bob unwraps bob.test.eth domain.
5. Bob wraps bob.test.eth domain, which clears fuses and expiry
6. Alice changes bob.test.eth domain ownership to her breaking Bob's impression that his domain was secure until expiry.

Proof of Concept

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

https://gist.github.com/panprog/71dea0fd1875b4d7d5849f7da822ea8b

Recommended Mitigation Steps

Burning any fuse (including PARENT_CANNOT_CONTROL) must require CANNOT_UNWRAP fuse to be burned (because otherwise it's possible to unwrap+wrap to clear that fuse).

In NameWrapper._canFusesBeBurned, condition should be different:

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