ENS contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L182-L186 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L203-L205

Vulnerability details

ETHRegistrarController's register() and renew() transfer out remainder native tokens via payable(to).transfer call. This is unsafe as transfer has hard coded gas budget and can fail when msg.sender is a smart contract. Such transactions will fail for smart contract users which don't fit to 2300 gas stipend transfer have.

The affected users are smart contracts, so it is a programmatic usage failure, i.e. denial of service for downstream systems.

Setting severity to be medium as while register() is msg.sender based, it is initiating functionality and can be easily repeated with a different caller, while renew() do not have any msg.sender accounting, so the impact is core functionality unavailability that can be mitigated.

Proof of Concept

User facing register() calls transfer:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L182-L186

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

renew() as well:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L203-L205

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

Both functions return only excess values only and the impact is their partial unavailability. I.e. when user is a smart contract that can't handle transfer the register() and renew() will be unavailable whenever msg.value exceeds the amount needed and msg.sender can't accept the change.

Also, owner's withdraw():

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L210-L212

    function withdraw() public {
        payable(owner()).transfer(address(this).balance);
    }

Notice that BulkRenewal's renewAll() does the same (not in scope, listed for completeness):

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/BulkRenewal.sol#L66-L68

        // Send any excess funds back
        payable(msg.sender).transfer(address(this).balance);
    }

References

The issues with transfer() are outlined here:

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Recommended Mitigation Steps

Generally replacing the transfer() should be coupled with the introduction of non-reentrancy feature, but here the transfer calls happen after all internal updates, being the very last operation each time, so reentrancy isn't an issue.

This way the recommendation is to use low-level call.value(amount) with the corresponding result check or employ OpenZeppelin's Address.sendValue:

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

1. Native funds on BulkRenewal balance can be stolen (low)

renewAll() doesn't calculate the funds needed and sends back the whole balance instead.

Proof of Concept

BulkRenewal's renewAll sends the whole balance as a change to the function caller:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/BulkRenewal.sol#L66-L68

        // Send any excess funds back
        payable(msg.sender).transfer(address(this).balance);
    }

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/BulkRenewal.sol#L50-L68

    function renewAll(string[] calldata names, uint256 duration)
        external
        payable
        override
    {
        ETHRegistrarController controller = getController();
        for (uint256 i = 0; i < names.length; i++) {
            IPriceOracle.Price memory price = controller.rentPrice(
                names[i],
                duration
            );
            controller.renew{value: price.base + price.premium}(
                names[i],
                duration
            );
        }
        // Send any excess funds back
        payable(msg.sender).transfer(address(this).balance);
    }

Recommended Mitigation Steps

Consider reusing rentPrice's approach and send back msg.value - total only:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/BulkRenewal.sol#L40-L47

        ETHRegistrarController controller = getController();
        for (uint256 i = 0; i < names.length; i++) {
            IPriceOracle.Price memory price = controller.rentPrice(
                names[i],
                duration
            );
            total += (price.base + price.premium);
        }

2. RRUtils's nameLength and labelCount use infinite loop (low)

Both functions use while (true) loop with a break condition.

Proof of Concept

nameLength() and labelCount() use infinite loop:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L19-L29

    function nameLength(bytes memory self, uint offset) internal pure returns(uint) {
        uint idx = offset;
        while (true) {
            assert(idx < self.length);
            uint labelLen = self.readUint8(idx);
            idx += labelLen + 1;
            if (labelLen == 0) {
                break;
            }
        }
        return idx - offset;

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L49-L60

    function labelCount(bytes memory self, uint offset) internal pure returns(uint) {
        uint count = 0;
        while (true) {
            assert(offset < self.length);
            uint labelLen = self.readUint8(offset);
            offset += labelLen + 1;
            if (labelLen == 0) {
                break;
            }
            count += 1;
        }
        return count;

Recommended Mitigation Steps

Consider updating the logic with control of the loop, for example:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L19-L29

    function nameLength(bytes memory self, uint offset) internal pure returns(uint) {
        uint idx = offset;
+       bool lenReached;
+       while (idx < self.length && !lenReached) {
-       while (true) {
-           assert(idx < self.length);
            uint labelLen = self.readUint8(idx);
            idx += labelLen + 1;
            if (labelLen == 0) {
                lenReached = true;
            }
        }
+       require(lenReached, "nameLength: length exceeded");
        return idx - offset;

3. Incorrect description for ReverseRegistrar's claimForAddr (low)

calling account is referred in the description, while it's any account provided:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L69-L88

    /**
     * @dev Transfers ownership of the reverse ENS record associated with the
     *      calling account.
     * @param addr The reverse record to set
     * @param owner The address to set as the owner of the reverse record in ENS.
     * @return The ENS node hash of the reverse record.
     */
    function claimForAddr(
        address addr,
        address owner,
        address resolver
    ) public override authorised(addr) returns (bytes32) {
        bytes32 labelHash = sha3HexAddress(addr);
        bytes32 reverseNode = keccak256(
            abi.encodePacked(ADDR_REVERSE_NODE, labelHash)
        );
        emit ReverseClaimed(addr, reverseNode);
        ens.setSubnodeRecord(ADDR_REVERSE_NODE, labelHash, owner, resolver, 0);
        return reverseNode;
    }

Recommended Mitigation Steps

Consider updating to the account provided

4. Incorrect description for setNameForAddr (low)

First updates the resolver to the default reverse resolver if necessary is claimed in the description, while it's not happening (most probably the line copied over from ReverseRegistrar's setName):

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/registry/ReverseRegistrar.sol#L122-L141

    /**
     * @dev Sets the `name()` record for the reverse ENS record associated with
     * the account provided. First updates the resolver to the default reverse
     * resolver if necessary.
     * Only callable by controllers and authorised users
     * @param addr The reverse record to set
     * @param owner The owner of the reverse node
     * @param name The name to set for this address.
     * @return The ENS node hash of the reverse record.
     */
    function setNameForAddr(
        address addr,
        address owner,
        address resolver,
        string memory name
    ) public override returns (bytes32) {
        bytes32 node = claimForAddr(addr, owner, resolver);
        NameResolver(resolver).setName(node, name);
        return node;
    }

NameResolver(resolver).setName only sets the record:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/resolvers/profiles/NameResolver.sol#L10-L18

    /**
     * Sets the name associated with an ENS node, for reverse records.
     * May only be called by the owner of that node in the ENS registry.
     * @param node The node to update.
     */
    function setName(bytes32 node, string calldata newName) virtual external authorised(node) {
        names[node] = newName;
        emit NameChanged(node, newName);
    }

Recommended Mitigation Steps

Consider removing the First updates the resolver to the default reverse resolver if necessary part.

5. RRUtils's nameLength and labelCount use assert (non-critical)

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.

Proof of Concept

nameLength() and labelCount() use assert:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L19-L22

    function nameLength(bytes memory self, uint offset) internal pure returns(uint) {
        uint idx = offset;
        while (true) {
            assert(idx < self.length);

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L49-L52

    function labelCount(bytes memory self, uint offset) internal pure returns(uint) {
        uint count = 0;
        while (true) {
            assert(offset < self.length);

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases.

6. Typo in NameWrapper's setFuses description (non-critical)

To be PARENT_CANNOT_CONTROL:

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

     * @param fuses fuses to burn (cannot burn PARENT_CANOT_CONTROL)

upgradeContract is a storage variable that is being directly read multiple times:

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

    function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
        public
        onlyOwner
    {
        if (address(upgradeContract) != address(0)) {
            registrar.setApprovalForAll(address(upgradeContract), false);
            ens.setApprovalForAll(address(upgradeContract), false);
        }

        upgradeContract = _upgradeAddress;

        if (address(upgradeContract) != address(0)) {
            registrar.setApprovalForAll(address(upgradeContract), true);
            ens.setApprovalForAll(address(upgradeContract), true);
        }
    }

Recommended Mitigation Steps

Consider introducing memory variable instead of reading storage

    function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
        public
        onlyOwner
    {
+       address _upgradeContract = address(upgradeContract);
        if (_upgradeContract != address(0)) {
        ...
    }