ENS contest - c3phas'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#L204 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L211

Vulnerability details

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when :

1. The withdrawer smart contract does not implement a payable fallback function.
2. The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
3. The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Proof of Concept

File: ETHRegistrarController.sol line 182-186

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

File: ETHRegistrarController.sol line 204

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

File: ETHRegistrarController.sol line 211

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

Tools Used

Visual Studio

Recommended Mitigation Steps

Call() should be used instead of transfer() on an address payable Additionally, note that the sendValue function available in OpenZeppelin Contract’s Address library can be used to transfer the withdrawn Ether without being limited to 2300 gas units. Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the “Check-effects-interactions” pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. For further reference on why using Solidity’s transfer is no longer recommended, refer to these articles:

• Stop using Solidity’s transfer now
• Reentrancy after Istanbul

QA FINDINGS

Missing checks for address(0x0) when assigning values to address state variables

File: Owned.sol line 19

    function setOwner(address newOwner) public owner_only {
        owner = newOwner;
    }

owner = newOwner;

constants should be defined rather than using magic numbers

There are several occurrences of literal values with unexplained meaning .Literal values in the codebase without an explained meaning make the code harder to read, understand and maintain, thus hindering the experience of developers, auditors and external contributors alike.

Developers should define a constant variable for every magic value used , giving it a clear and self-explanatory name.

File: ERC1155Fuse.sol line 143

        expiry = uint64(t >> 192);

File: ERC1155Fuse.sol line 147

            fuses = uint32(t >> 160);

Open todos

File: DNSSECImpl.sol line 238

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

Lock pragmas to specific compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.

File: DNSSECImpl.sol line 2

pragma solidity ^0.8.4;

File: ETHRegistrarController.sol line 1

pragma solidity >=0.8.4;

File: NameWrapper.sol line 2

pragma solidity ^0.8.4;

File: Controllable.sol line 2

pragma solidity ^0.8.4;

File: ERC1155Fuse.sol line 2

Unused named return

Using both named returns and a return statement isn’t necessary in a function.To improve code quality, consider using only one of those.

File: RRUtils.sol line 38-41

    function readName(bytes memory self, uint offset) internal pure returns(bytes memory ret) {
        uint len = nameLength(self, offset);
        return self.substring(offset, len);
    }

File: NameWrapper.sol line 741

    function _addLabel(string memory label, bytes memory name)
        internal
        pure
        returns (bytes memory ret)
    {
        if (bytes(label).length < 1) {
            revert LabelTooShort();
        }
        if (bytes(label).length > 255) {
            revert LabelTooLong(label);
        }
        return abi.encodePacked(uint8(bytes(label).length), label, name);
    }

Natspec is incomplete

File: NameWrapper.sol line 112-117

Missing @param tokenId

    /**
     * @notice Get the metadata uri
     * @return String uri of the metadata service
     */


    function uri(uint256 tokenId) public view override returns (string memory) {

Missing revert strings

Use revert strings or consider using custom errors You should attach error reason strings along with require statements to make it easier to understand why a contract call reverted.

File: ETHRegistrarController.sol line 57

        require(_maxCommitmentAge > _minCommitmentAge);

File: ETHRegistrarController.sol line 121

        require(commitments[commitment] + maxCommitmentAge < block.timestamp);

File: ETHRegistrarController.sol line 246

        require(duration >= MIN_REGISTRATION_DURATION);

File: BytesUtils.sol line 12

        require(offset + len <= self.length);

File: BytesUtils.sol line 146

        require(idx + 2 <= self.length);

File: BytesUtils.sol line 159

        require(idx + 4 <= self.length);

More instances:

File: BytesUtils.sol line 172 File: BytesUtils.sol line 185 File: BytesUtils.sol line 199-200 File: BytesUtils.sol line 235 File: BytesUtils.sol line 262 File: BytesUtils.sol line 270

FINDINGS

Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block see resource

File: ETHRegistrarController.sol line 184

                msg.value - (price.base + price.premium)

The above line cannot underflow due to the check on line 182 which ensures that msg.value is greater than (price.base + price.premium) before performing the operation

File: ETHRegistrarController.sol line 204

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

The above line cannot underflow due to the check on line 203 which ensures that msg.value is greater than price.base

Using unchecked blocks to save gas - Increments in for loop can be unchecked ( save 30-40 gas per loop iteration)

The majority of Solidity for loops increment a uint256 variable that starts at 0. These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). The default over/underflow check wastes gas in every iteration of virtually every for loop . eg.

e.g Let's work with a sample loop below.

for(uint256 i; i < 10; i++){
//doSomething
}

can be written as shown below.

for(uint256 i; i < 10;) {
  // loop logic
  unchecked { i++; }
}

We can also write it as an inlined function like below.

function inc(i) internal pure returns (uint256) {
  unchecked { return i + 1; }
}
for(uint256 i; i < 10; i = inc(i)) {
  // doSomething
}

Affected code File: DNSSECImpl.sol line 93

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
        for(uint i = 0; i < input.length; i++) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
        }
        return proof;
    }

The above should be modified to:

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
        for(uint i = 0; i < input.length;) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
	    unchecked {
			 ++i;
		}
        }
        return proof;
    }

Other Instances to modify

File: ETHRegistrarController.sol line 256

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

File: ERC1155Fuse.sol line 92-94

        for (uint256 i = 0; i < accounts.length; ++i) {
            batchBalances[i] = balanceOf(accounts[i], ids[i]);
        }

File: ERC1155Fuse.sol line 205

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

see resource

Cache the length of arrays in loops (saves ~6 gas per iteration)

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

The solidity compiler will always read the length of the array during each iteration. That is,

1.if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first), 2.if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first), 3.if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

This extra costs can be avoided by caching the array length (in stack): When reading the length of an array, sload or mload or calldataload operation is only called once and subsequently replaced by a cheap dupN instruction. Even though mload , calldataload and dupN have the same gas cost, mload and calldataload needs an additional dupN to put the offset in the stack, i.e., an extra 3 gas. which brings this to 6 gas

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead:

File: DNSSECImpl.sol line 93

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
        for(uint i = 0; i < input.length; i++) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
        }
        return proof;
    }

The above should be modified to

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
				uint256 length = input.length;
        for(uint i = 0; i < length; i++) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
        }
        return proof;
    }

Other instances to modify

File: ETHRegistrarController.sol line 256

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

File: ERC1155Fuse.sol line 78-97

    function balanceOfBatch(address[] memory accounts, uint256[] memory ids)
        public
        view
        virtual
        override
        returns (uint256[] memory)
    {
        require(
            accounts.length == ids.length,
            "ERC1155: accounts and ids length mismatch"
        );
        uint256[] memory batchBalances = new uint256[](accounts.length);
        for (uint256 i = 0; i < accounts.length; ++i) {
            batchBalances[i] = balanceOf(accounts[i], ids[i]);
        }
        return batchBalances;
    }

In the above function accounts.length should be cached as it is being checked two times and also inside the loop

File: ERC1155Fuse.sol line 205

    function safeBatchTransferFrom(
       
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            ids.length == amounts.length,
            "ERC1155: ids and amounts length mismatch"
        );
     
        for (uint256 i = 0; i < ids.length; ++i) {

ids.length should be cached rather than being looked up for every iteration

++i costs less gas compared to i++ or i += 1 (~5 gas per iteration)

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

File: DNSSECImpl.sol line 93

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
        for(uint i = 0; i < input.length; i++) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
        }
        return proof;
    }

The above function should be modified to

    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
        bytes memory proof = anchors;
        for(uint i = 0; i < input.length; ++i) {
            RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
            proof = rrset.data;
        }
        return proof;
    }

Splitting require() statements that use && saves gas - (saves 8 gas per &&)

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).

File: ERC1155Fuse.sol line 215-218

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

File: File: ERC1155Fuse.sol line 290-293

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

Not using the named return variables when a function returns, wastes deployment gas

File: RRUtils.sol line 38-41

    function readName(bytes memory self, uint offset) internal pure returns(bytes memory ret) {
        uint len = nameLength(self, offset);
        return self.substring(offset, len);
    }

File: NameWrapper.sol line 741

    function _addLabel(string memory label, bytes memory name)
        internal
        pure
        returns (bytes memory ret)
    {
        if (bytes(label).length < 1) {
            revert LabelTooShort();
        }
        if (bytes(label).length > 255) {
            revert LabelTooLong(label);
        }
        return abi.encodePacked(uint8(bytes(label).length), label, name);
    }

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

File: ETHRegistrarController.sol line 21

    uint256 public constant MIN_REGISTRATION_DURATION = 28 days;

use shorter revert strings(less than 32 bytes)

Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.Each extra chunk of bytes past the original 32 incurs an MSTORE which costs 3 gas

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

File: ETHRegistrarController.sol line 99

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

Other instances to modify File: ETHRegistrarController.sol line 137 File: ETHRegistrarController.sol line 196 File: ETHRegistrarController.sol line 232 File: ETHRegistrarController.sol line 238-242 File: ETHRegistrarController.sol line 259 File: Controllable.sol line 17 File: ERC1155Fuse.sol line 60-63 File: ERC1155Fuse.sol line 85 File: ERC1155Fuse.sol line 176&177 File: ERC1155Fuse.sol line 248-250

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.

Use Custom Errors instead of Revert Strings to save Gas(~50 gas)

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source

Affected code:

File: ETHRegistrarController.sol line 99

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

Other instances File: ETHRegistrarController.sol line 137 File: ETHRegistrarController.sol line 196 File: ETHRegistrarController.sol line 232 File: ETHRegistrarController.sol line 238-242 File: ETHRegistrarController.sol line 259 File: Controllable.sol line 17 File: ERC1155Fuse.sol line 60-63 File: ERC1155Fuse.sol line 85 File: ERC1155Fuse.sol line 176

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Variable packing does not apply to functional arguments as such no inherent benefit in using smaller types.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

File: DNSSECImpl.sol line 58

    function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
        algorithms[id] = algo;
        emit AlgorithmUpdated(id, address(algo));
    }

id should be declared as a uint256 then if need be downcast.

File: DNSSECImpl.sol line 69

    function setDigest(uint8 id, Digest digest) public owner_only {
        digests[id] = digest;
        emit DigestUpdated(id, address(digest));
    }

id should be declared as a uint256 then if need be downcast.

File: DNSSECImpl.sol line 335 digesttype should be declared as a uint256 then if need be downcast.

    function verifyDSHash(uint8 digesttype, bytes memory data, bytes memory digest) internal view returns (bool) {
        if (address(digests[digesttype]) == address(0)) {
            return false;
        }
        return digests[digesttype].verify(data, digest);
    }
}

File: ETHRegistrarController.sol line 133,134 fuses and wrapperExpiry should be declared as a uint256 then if need be downcast.

    function register(
        string calldata name,
        address owner,
        uint256 duration,
        bytes32 secret,
        address resolver,
        bytes[] calldata data,
        bool reverseRecord,
        uint32 fuses,
        uint64 wrapperExpiry
    ) public payable override {
        IPriceOracle.Price memory price = rentPrice(name, duration);
        require(

File: NameWrapper.sol line 210-237 fuses and expiry should be declared as a uint256 then if need be downcast.

    function wrapETH2LD(
        string calldata label,
        address wrappedOwner,
        uint32 fuses,
        uint64 expiry,
        address resolver
    ) public override returns (uint64) {

File: NameWrapper.sol line 256-257 fuses and expiry should be declared as a uint256 then if need be downcast.

    function registerAndWrapETH2LD(
        string calldata label,
        address wrappedOwner,
        uint256 duration,
        address resolver,
        uint32 fuses,
        uint64 expiry
    ) external override onlyController returns (uint256 registrarExpiry) {

File: NameWrapper.sol line 456-493

    function setChildFuses(
        bytes32 parentNode,
        bytes32 labelhash,
        uint32 fuses,
        uint64 expiry
    ) public {
        bytes32 node = _makeNode(parentNode, labelhash);
        (address owner, uint32 oldFuses, uint64 oldExpiry) = getData(
            uint256(node)
        );
        uint64 maxExpiry;
        if (parentNode == ETH_NODE) {

File: NameWrapper.sol line 508-509

        uint32 fuses,
        uint64 expiry

File: NameWrapper.sol line 544,545,546

    function setSubnodeRecord(
        bytes32 parentNode,
        string memory label,
        address newOwner,
        address resolver,
        uint64 ttl,
        uint32 fuses,
        uint64 expiry
    )