ENS contest - hake's results

Lines of code

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

Vulnerability details

Impact

It might be impossible for some addresses to receive ETH refund via transfer() because receiver address might have methods that exceed 2300 gas, ultimately leading to frozen funds.

Proof of Concept

Native transfer() function has a limit of 2300 gas, which might not be enough when the receiving end has gas-expensive operations, leading the transaction to revert.

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

            payable(msg.sender).transfer(

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Use call() instead of transfer() and make sure to implement a boolean return check on call().

Low Risk Findings Overview

Non-critical Findings Overview

QA overview per contract

Low Risk Findings

[L-01] Solidity 0.8.13 Compiler Bug

Many contracts use a floating pragma. If by chance contracts get deployed with version 0.8.13 they might be susceptible to severe issues if they use assembly code. Please see Certora POC 2 instances of this issue have been found:

[L-01] SHA1.sol#L7-L33

assembly {
            // Get a safe scratch location
            let scratch := mload(0x40)

            // Get the data length, and point data at the first byte
            let len := mload(data)
            data := add(data, 32)

            // Find the length after padding
            let totallen := add(and(add(len, 1), 0xFFFFFFFFFFFFFFC0), 64)
            switch lt(sub(totallen, len), 9)
            case 1 { totallen := add(totallen, 64) }

            let h := 0x6745230100EFCDAB890098BADCFE001032547600C3D2E1F0

            function readword(ptr, off, count) -> result {
                result := 0
                if lt(off, count) {
                    result := mload(add(ptr, off))
                    count := sub(count, off)
                    if lt(count, 32) {
                        let mask := not(sub(exp(256, sub(32, count)), 1))
                        result := and(result, mask)
                    }
                }
            }

[L-01b] ReverseRegistrar.sol#L162-L179

    function sha3HexAddress(address addr) private pure returns (bytes32 ret) {
        assembly {
            for {
                let i := 40
            } gt(i, 0) {

            } {
                i := sub(i, 1)
                mstore8(i, byte(and(addr, 0xf), lookup))
                addr := div(addr, 0x10)
                i := sub(i, 1)
                mstore8(i, byte(and(addr, 0xf), lookup))
                addr := div(addr, 0x10)
            }

            ret := keccak256(0, 40)
        }
    }

[L-02] Unsafe transfer()/transferFrom()

Some tokens might return 0 instead of reverting on failure. If return value of transfer()/transferFrom() is not checked the transaction might silently fail. 1 instance of this issue has been found:

[L-02] NameWrapper.sol#L231-L232

        registrar.transferFrom(registrant, address(this), tokenId);

[L-03] Floating pragma

A floating pragma might result in contract being tested/deployed with different compiler versions possibly leading to unexpected behaviour. 10 instances of this issue have been found:

[L-03] DNSSEC.sol#L2

pragma solidity ^0.8.4;

[L-03b] SHA1.sol#L1

pragma solidity >=0.8.4;

[L-03c] DNSSECImpl.sol#L2

pragma solidity ^0.8.4;

[L-03d] ENS.sol#L1

pragma solidity >=0.8.4;

[L-03e] ReverseRegistrar.sol#L1

pragma solidity >=0.8.4;

[L-03f] ETHRegistrarController.sol#L1

pragma solidity >=0.8.4;

[L-03g] ENSRegistry.sol#L1

pragma solidity >=0.8.4;

[L-03h] BaseRegistrarImplementation.sol#L1

pragma solidity >=0.8.4;

[L-03i] ERC1155Fuse.sol#L1-L2

//SPDX-License-Identifier: MIT
pragma solidity ^0.8.4;

[L-03j] NameWrapper.sol#L2

pragma solidity ^0.8.4;

[L-04] Not functionality to delete compromised access control from mapping

If any address in controllers gets compromised there is no functionality to delete it and prevent it from abusing its privileges. 1 instance of this issue has been found:

[L-04] Controllable.sol#L18-L21

    function setController(address controller, bool enabled) public onlyOwner {
        controllers[controller] = enabled;
        emit ControllerChanged(controller, enabled);
    }

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. 3 instances of this issue have been found:

[L-05] ReverseRegistrar.sol#L149-L152

return
            keccak256(
                abi.encodePacked(ADDR_REVERSE_NODE, sha3HexAddress(addr))
            );

[L-05b] ReverseRegistrar.sol#L82-L83

        bytes32 reverseNode = keccak256(
            abi.encodePacked(ADDR_REVERSE_NODE, labelHash)

[L-05c] NameWrapper.sol#L738-L739

        return keccak256(abi.encodePacked(node, labelhash));

Non-critical Findings

[N-01] Remove TODOs

They add unnecessary cluttler and harm readbility for auditors. 1 instance of this issue has been found:

[N-01] DNSSECImpl.sol#L238-L239

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

[N-02] Typo

Please fix typos. 2 instances of this issue have been found:

[N-02] NameWrapper.sol#L370-L371

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

[N-02b] NameWrapper.sol#L893-L894

        // Set PARENT_CANNOT_REPLACE to reflect wrapper + registrar control over the 2LD
						   PARENT_CANNOT_CONTROL

[N-03] Missing error string

Errors should output a string for better readability from both a user's and developer's perspective. 1 instance of this issue has been found:

[N-03] ETHRegistrarController.sol#L246-L247

        require(duration >= MIN_REGISTRATION_DURATION);

Gas Optimizations

Gas overview per contract

Gas Optimizations

[G-01] immutable variable missing zero address check

If variable is accidentally set to 0 the contract will have to be redeployed, spending unnecessary gas. 2 instances of this issue have been found:

[G-01] ETHRegistrarController.sol#L49-L65

    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;
    }

[G-01b] ReverseRegistrar.sol#L28-L29

    constructor(ENS ensAddr) {
        ens = ensAddr;

[G-02] Use custom errors rather than revert()/require() strings to save gas

Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. 27 instances of this issue have been found:

[G-02] ReverseRegistrar.sol#L52-L55

require(
            address(resolver) != address(0),
            "ReverseRegistrar: Resolver address must not be 0"
        );

[G-02b] ReverseRegistrar.sol#L41-L47

require(
            addr == msg.sender ||
                controllers[msg.sender] ||
                ens.isApprovedForAll(addr, msg.sender) ||
                ownsContract(addr),
            "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
        );

[G-02c] ETHRegistrarController.sol#L263-L266

resolver.functionCall(
                data[i],
                "ETHRegistrarController: Failed to set Record"
            );

[G-02d] ETHRegistrarController.sol#L259-L262

require(
                txNamehash == nodehash,
                "ETHRegistrarController: Namehash on record do not match the name being registered"
            );

[G-02e] ETHRegistrarController.sol#L242-L243

        require(available(name), "ETHRegistrarController: Name is unavailable");

[G-02f] ETHRegistrarController.sol#L238-L241

require(
            commitments[commitment] + maxCommitmentAge > block.timestamp,
            "ETHRegistrarController: Commitment has expired"
        );

[G-02g] ETHRegistrarController.sol#L232-L235

require(
            commitments[commitment] + minCommitmentAge <= block.timestamp,
            "ETHRegistrarController: Commitment is not valid"
        );

[G-02h] ETHRegistrarController.sol#L196-L199

require(
            msg.value >= price.base,
            "ETHController: Not enough Ether provided for renewal"
        );

[G-02i] ETHRegistrarController.sol#L137-L140

require(
            msg.value >= (price.base + price.premium),
            "ETHRegistrarController: Not enough ether provided"
        );

[G-02j] ETHRegistrarController.sol#L99-L102

require(
                resolver != address(0),
                "ETHRegistrarController: resolver is required when data is supplied"
            );

[G-02k] ERC1155Fuse.sol#L60-L63

require(
            account != address(0),
            "ERC1155: balance query for the zero address"
        );

[G-02l] ERC1155Fuse.sol#L85-L89

require(
            accounts.length == ids.length,
            "ERC1155: accounts and ids length mismatch"
        );

[G-02m] ERC1155Fuse.sol#L107-L110

require(
            msg.sender != operator,
            "ERC1155: setting approval status for self"
        );

[G-02n] ERC1155Fuse.sol#L176-L177

        require(to != address(0), "ERC1155: transfer to the zero address");

[G-02o] ERC1155Fuse.sol#L177-L180

require(
            from == msg.sender || isApprovedForAll(from, msg.sender),
            "ERC1155: caller is not owner nor approved"
        );

[G-02p] ERC1155Fuse.sol#L195-L198

require(
            ids.length == amounts.length,
            "ERC1155: ids and amounts length mismatch"
        );

[G-02q] ERC1155Fuse.sol#L199-L200

        require(to != address(0), "ERC1155: transfer to the zero address");

[G-02r] ERC1155Fuse.sol#L200-L203

require(
            from == msg.sender || isApprovedForAll(from, msg.sender),
            "ERC1155: transfer caller is not owner nor approved"
        );

[G-02s] ERC1155Fuse.sol#L215-L218

require(
                amount == 1 && oldOwner == from,
                "ERC1155: insufficient balance for transfer"
            );

[G-02t] ERC1155Fuse.sol#L215-L218

require(
                amount == 1 && oldOwner == from,
                "ERC1155: insufficient balance for transfer"
            );

[G-02u] ERC1155Fuse.sol#L249-L250

        require(newOwner != address(0), "ERC1155: mint to the zero address");

[G-02v] ERC1155Fuse.sol#L250-L253

require(
            newOwner != address(this),
            "ERC1155: newOwner cannot be the NameWrapper contract"
        );

[G-02w] ERC1155Fuse.sol#L290-L293

require(
            amount == 1 && oldOwner == from,
            "ERC1155: insufficient balance for transfer"
        );

[G-02x] ERC1155Fuse.sol#L322-L323

       revert("ERC1155: ERC1155Receiver rejected tokens");

[G-02y] ERC1155Fuse.sol#L327-L328

                revert("ERC1155: transfer to non ERC1155Receiver implementer");

[G-02z] ERC1155Fuse.sol#L354-L355

                    revert("ERC1155: ERC1155Receiver rejected tokens");

[G-02{] ERC1155Fuse.sol#L359-L360

                revert("ERC1155: transfer to non ERC1155Receiver implementer");

[G-03] require()/revert() checks used multiple times should be turned into a function or modifier

Doing so increases code readability decreases number of instructions for the compiler. 8 instances of this issue have been found:

[G-03] ERC1155Fuse.sol#L358-L360

catch {
                revert("ERC1155: transfer to non ERC1155Receiver implementer");
            }

[G-03b] ERC1155Fuse.sol#L326-L328

catch {
                revert("ERC1155: transfer to non ERC1155Receiver implementer");
            }

[G-03c] ERC1155Fuse.sol#L350-L355

if (
                    response !=
                    IERC1155Receiver(to).onERC1155BatchReceived.selector
                ) {
                    revert("ERC1155: ERC1155Receiver rejected tokens");
                }

[G-03d] ERC1155Fuse.sol#L319-L323

if (
                    response != IERC1155Receiver(to).onERC1155Received.selector
                ) {
                    revert("ERC1155: ERC1155Receiver rejected tokens");
                }

[G-03e] ERC1155Fuse.sol#L290-L293

      require(
            amount == 1 && oldOwner == from,
            "ERC1155: insufficient balance for transfer"
        );

[G-03f] ERC1155Fuse.sol#L215-L218

            require(
                amount == 1 && oldOwner == from,
                "ERC1155: insufficient balance for transfer"
            );

[G-03g] ERC1155Fuse.sol#L199-L200

        require(to != address(0), "ERC1155: transfer to the zero address");

[G-03h] ERC1155Fuse.sol#L176-L177

        require(to != address(0), "ERC1155: transfer to the zero address");

[G-04] Setting variable to default value is redundant

Variables are by default set to 0, so setting a variable to 0 is a waste of gas. Examples to avoid: uint i = 0 bool success = false 3 instances of this issue have been found:

[G-04] DNSSECImpl.sol#L93-L94

        for(uint i = 0; i < input.length; i++) {

[G-04b] ERC1155Fuse.sol#L205-L206

        for (uint256 i = 0; i < ids.length; ++i) {

[G-04c] ERC1155Fuse.sol#L92-L93

        for (uint256 i = 0; i < accounts.length; ++i) {

[G-05] array.length should be cached in for loop

Caching the length array would save gas on each iteration by not having to read from memory or storage multiple times. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

3 instances of this issue have been found:

[G-05] DNSSECImpl.sol#L93-L94

        for(uint i = 0; i < input.length; i++) {

[G-05b] ERC1155Fuse.sol#L205-L206

        for (uint256 i = 0; i < ids.length; ++i) {

[G-05c] ERC1155Fuse.sol#L92-L93

        for (uint256 i = 0; i < accounts.length; ++i) {

[G-06] for loop increments should be unchecked{} if overflow is not possible

From Solidity 0.8.0 onwards using the unchecked keyword saves 30 to 40 gas per loop. Example:

uint length = array.length;
for (uint i; i < length;){
		uncheck { ++i }
}

3 instances of this issue have been found:

[G-06] DNSSECImpl.sol#L93-L94

        for(uint i = 0; i < input.length; i++) {

[G-06b] ERC1155Fuse.sol#L205-L206

        for (uint256 i = 0; i < ids.length; ++i) {

[G-06c] ERC1155Fuse.sol#L92-L93

        for (uint256 i = 0; i < accounts.length; ++i) {

[G-07] Variable should be immutable

Variables set in the constructor that are not able to be changed by other functions should be set as immutable in order to save gas. 1 instance of this issue has been found:

[G-07] DNSSEC.sol#L7

bytes public anchors;

[G-08] require()/revert() check should be done sooner

Checks should be done as soon as all variables necessary to do them are present. Otherwise, we give room for unnecessary operations to happen before reaching a reverting point. 1 instance of this issue has been found:

[G-08] ETHRegistrarController.sol#L246-L247

        require(duration >= MIN_REGISTRATION_DURATION);