ENS contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L271-L285 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L189-L208

Vulnerability details

Impact

NameWrapper.renew() has an onlyController modifier. And ETHRegistrarController should be the controller of NameWrapper (Otherwise ETHRegistrarController.register cannot call NameWrapper.registerAndWrapETH2LD). Therefore, ETHRegistrarController.renew should call NameWrapper.renew. Otherwise, the second-level domain owners cannot renew their second-level domain in NameWrapper.

Proof of Concept

NameWrapper.renew has an onlyController modifier https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L271-L285

    function renew(
        uint256 tokenId,
        uint256 duration,
        uint64 expiry
    ) external override onlyController returns (uint256 expires) {
        bytes32 node = _makeNode(ETH_NODE, bytes32(tokenId));

        expires = registrar.renew(tokenId, duration);
        (address owner, uint32 fuses, uint64 oldExpiry) = getData(
            uint256(node)
        );
        expiry = _normaliseExpiry(expiry, oldExpiry, uint64(expires));

        _setData(node, owner, fuses, expiry);
    }

ETHRegistrarController should be the controller of NameWrapper. But ETHRegistrarController.renew doesn’t call NameWrapper.renew. https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L189-L208

    function renew(string calldata name, uint256 duration)
        external
        payable
        override
    {
        bytes32 label = keccak256(bytes(name));
        IPriceOracle.Price memory price = rentPrice(name, duration);
        require(
            msg.value >= price.base,
            "ETHController: Not enough Ether provided for renewal"
        );

        uint256 expires = base.renew(uint256(label), duration);

        if (msg.value > price.base) {
            payable(msg.sender).transfer(msg.value - price.base);
        }

        emit NameRenewed(name, label, msg.value, expires);
    }

Tools Used

None

Recommended Mitigation Steps

ETHRegistrarController.renew should call NameWrapper.renew

    function renew(string calldata name, uint256 duration, uint64 wrapperExpiry)
        external
        payable
        override
    {
        bytes32 label = keccak256(bytes(name));
        IPriceOracle.Price memory price = rentPrice(name, duration);
        require(
            msg.value >= price.base,
            "ETHController: Not enough Ether provided for renewal"
        );

        uint256 expires = NameWrapper.renew(uint256(label), duration, wrapperExpiry);

        if (msg.value > price.base) {
            payable(msg.sender).transfer(msg.value - price.base);
        }

        emit NameRenewed(name, label, msg.value, expires);
    }

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L211

Vulnerability details

Impact

Transfer ETH by using transfer() may cause this transaction to fail. In EIP-1884:

In many cases, a recipient of ether from a CALL will want to issue a LOG. The LOG operation costs 375 plus 375 per topic. If the LOG also wants to do an SLOAD, this change may make some such transfers fail.

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185

            payable(msg.sender).transfer(
                msg.value - (price.base + price.premium)
            );

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L211

        payable(owner()).transfer(address(this).balance);

Tools Used

None

Recommended Mitigation Steps

Use call{value: amount}() instead of transfer:

  (bool success, ) = payable(msg.sender).call{value: amount}("");
  require(success, "Address: unable to send value, recipient may have reverted");

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Note that call may suffer from reentrancy attack, it should follow Checks-Effects-Interactions.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L44-L80

Vulnerability details

Impact

There are some implementation mistakes in dnssec-oracle/BytesUtils.compare.

• There should be a sanity check for offsets and lens
• if (shortest > 32) is not a correct condition. It should check the size of the last block. shortest is the total size of the bytes array.
• Returned value disobey the dev comment https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L32. int diff = int(a & mask) - int(b & mask); and return int(len) - int(otherlen); give the opposite results.
• Should be careful of int underflow/overflow in int diff = int(a & mask) - int(b & mask) (e.g., type(int).min - 1 will revert)

In conclusion, these mistakes may lead to serious problems.

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L44-L80

    /*
    * @dev Returns a positive number if `other` comes lexicographically after
    *      `self`, a negative number if it comes before, or zero if the
    *      contents of the two bytes are equal. Comparison is done per-rune,
    *      on unicode codepoints.
    * @param self The first bytes to compare.
    * @param offset The offset of self.
    * @param len    The length of self.
    * @param other The second bytes to compare.
    * @param otheroffset The offset of the other string.
    * @param otherlen    The length of the other string.
    * @return The result of the comparison.
    */
    function compare(bytes memory self, uint offset, uint len, bytes memory other, uint otheroffset, uint otherlen) internal pure returns (int) {
        uint shortest = len;
        if (otherlen < len)
        shortest = otherlen;

        uint selfptr;
        uint otherptr;

        assembly {
            selfptr := add(self, add(offset, 32))
            otherptr := add(other, add(otheroffset, 32))
        }
        for (uint idx = 0; idx < shortest; idx += 32) {
            uint a;
            uint b;
            assembly {
                a := mload(selfptr)
                b := mload(otherptr)
            }
            if (a != b) {
                // Mask out irrelevant bytes and check again
                uint mask;
                if (shortest > 32) {
                    mask = type(uint256).max;
                } else {
                    mask = ~(2 ** (8 * (32 - shortest + idx)) - 1);
                }
                int diff = int(a & mask) - int(b & mask);
                if (diff != 0)
                return diff;
            }
            selfptr += 32;
            otherptr += 32;
        }

        return int(len) - int(otherlen);
    }

Tools Used

None

Recommended Mitigation Steps

    function compare(bytes memory self, uint offset, uint len, bytes memory other, uint otheroffset, uint otherlen) internal pure returns (int) {
        require(offset + len <= self.length && otheroffset + otherlen <= other.length); //## sanity check
        uint shortest = len;
        if (otherlen < len)
        shortest = otherlen;

        uint selfptr;
        uint otherptr;

        assembly {
            selfptr := add(self, add(offset, 32))
            otherptr := add(other, add(otheroffset, 32))
        }
        for (int remain = int(shortest); remain > 0; remain -= 32) {  //safely assume shortest does not overflow
            uint a;
            uint b;
            assembly {
                a := mload(selfptr)
                b := mload(otherptr)
            }
            if (a != b) {
                // Mask out irrelevant bytes and check again
                uint mask;
                if (remain >= 32) { //##
                    mask = type(uint256).max;
                } else {
                    mask = ~(2 ** (8 * (32 - remain)) - 1);
                }
                //int diff = int(b & mask) - int(a & mask);  //## overflow while converting to int for non ascii character
                //if (diff != 0)
                //return diff;
                a &= mask;
                b &= mask;
                if (a < b) return 1;
                else if(a > b) return -1;
            }
            selfptr += 32;
            otherptr += 32;
        }

        return int(otherlen) - int(len);  //reasonable to safely assume that length cannot overflow int
    }

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L186-L190

Vulnerability details

Impact

DNSSEC allows parent zones to sign for its child zones. To check validity of a signature, RFC4034 3.1.7 requires the Signer's Name in any RRSIG RDATA to contain the zone of covered RRset. This requirement is reasonable since any child zone should be covered by its parent zone.

ENS tries to implement the concept of name coverage in DNSSECImpl.verifySignature, but unfortuantely does it wrong, resulting in possibiliy of coverage between two unrelated domains. In the worst case, an attacker can utilize this bug to forge malicious trust chains and authenticate invalid domains.

Proof of Concept

In DNSSECImpl.verifySignature, ENS tries to verify the name of RRSet zone (name) is contained by Signer's Name (rrset.signerName).

    if(rrset.signerName.length > name.length
            || !rrset.signerName.equals(0, name, name.length - rrset.signerName.length))    //## This allows matches such as name="SubString.com" signerName="String.com", which is clearly incorrect, use label counts instead
        {
            revert InvalidSignerName(name, rrset.signerName);
        }

In DNS, for a parent zone to contain another child zone, we generally require the child zone to be a subdomain of the parent. For instance, example.eth. in considered to cover sub.example.eth., while xample.eth. should not be cover example.eth..

Unfortunately in the implementation shown above, both cases will path the check, and ample.eth. will be considered appropriate to sign for example.eth.. This is against the original design of DNS, and would result in breach of zone hierarchy.

In practice, the requirement to exploit this is a bit more complex. Since names are stored as a sequence of packed labels, example.eth. should be stored as \x06example\x03eth\x00, while xample.eth. is stored as \x05xample\x03eth\x00. Thus to successfully pull off the attack ,we have to make sure that the packed signer's name is actually a substring of child zone.

A simple (yet unrealistic) example can be like this xample.eth. can sign for e\x05xample.eth., since packed format of those two names are \x05xample\x03eth\x00 and \x07e\x05ample\x03eth\x00.

In general, it would require some effort for an attacker to find attackable zones, nevertheless, this should still be considered as a potential threat to the integrity of ENS.

Tools Used

None

Recommended Mitigation Steps

Check label by label instead of comparing the entire name. To actually meet all requirements specified in RFC4034 and RFC4035, there are still a lot to do, but we will discuss that in a seperate issue for clarity.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L266-L268

Vulnerability details

Impact

Comparing serial numbers should follow RFC1982 due to the possibility of numbers wrapping around. RRUtils.serialNumberGte tried to follow the RFC but failed to do so, leading to incorrect results in comparison.

Proof of Concept

For a serial number i1 to be greater than i2, the rules provided by RFC1982 is as follow ((i1 < i2) && ((i2 - i1) > (2**31))) || ((i1 > i2) && ((i1 - i2) < (2**31)))

ENS implements int32(i1) - int32(i2) > 0, which will suffer from revert in cases such as i1=0x80000000, i2=0x7fffffff

Tools Used

None

Recommended Mitigation Steps

Use the naive implementation instead return (i1 == i2) || ((i1 < i2) && ((i2 - i1) > (2**31))) || ((i1 > i2) && ((i1 - i2) < (2**31)));

Summary

We list 2 low-critical findings:

• (Low) floating pragma
• (Low) Open TODO

(Low) floating pragma

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

dnssec-oracle/BytesUtils.sol
1:pragma solidity ^0.8.4;

dnssec-oracle/SHA1.sol
1:pragma solidity >=0.8.4;

dnssec-oracle/DNSSECImpl.sol
2:pragma solidity ^0.8.4;

dnssec-oracle/DNSSEC.sol
2:pragma solidity ^0.8.4;

dnssec-oracle/RRUtils.sol
1:pragma solidity ^0.8.4;

Recommended Mitigation Steps

Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.8.4;

(Low) Open TODO

Impact

Open TODO should be resolved.

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L238

        // TODO: Check key isn't expired, unless updating key itself

Recommended Mitigation Steps

Resolve the TODO.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L49-L65 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/BaseRegistrarImplementation.sol#L46-L49 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L49-L74

Vulnerability details

Impact

ETHRegistrarController, BaseRegistrarImplementation and NameWrapper should have some sanity checks in their constructor, Otherwise it could lead to irreversible errors.

For instance, base and price are set in ETHRegistrarController.constructor. And they are both immutable. So if they are set to wrong addresses(e.g., address(0)). it is impossible to change them back to correct values.

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L49-L65

    BaseRegistrarImplementation immutable base;
    IPriceOracle public immutable prices;
    …

    constructor(
        BaseRegistrarImplementation _base,
        IPriceOracle _prices,
        uint256 _minCommitmentAge,
        uint256 _maxCommitmentAge,
        ReverseRegistrar _reverseRegistrar,
        INameWrapper _nameWrapper
    ) {
        require(_maxCommitmentAge > _minCommitmentAge);

        base = _base;
        prices = _prices;
        minCommitmentAge = _minCommitmentAge;
        maxCommitmentAge = _maxCommitmentAge;
        reverseRegistrar = _reverseRegistrar;
        nameWrapper = _nameWrapper;
    }

Tools Used

None

Recommended Mitigation Steps

Add sanity checks in those constructor.