ENS contest - CRYP70's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L231 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L341-L345

Vulnerability details

Impact

Use safeTransferFrom instead of transferFrom for ERC721 instances so tokens don't get lost in the void by being locked up if they get sent to contracts who don't know how to handle them. In addition to this, the transaction may also result in a silent failure.

This was found in contracts/wrapper/NameWrapper.sol:341 (where tokens are transferred from the address of NameWrapper to the newRegistrant) and in contracts/wrapper/NameWrapper.sol:231 where the registrant tokens are being transferred from the user to the NameWrapper

Proof of Concept

• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L231
• https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L341-L345

Notes on recommendations

I recommend the use of safeTransferFrom in the referenced lines from the source code as the OpenZeppelin documentation highly encourages the use of safeTransferFrom as opposed to transferFrom where possible as this safely transfers the tokenId by first checking the contract recipients are aware of the ERC721 protocol. Source: https://docs.openzeppelin.com/contracts/4.x/api/token/erc721#IERC721-transferFrom-address-address-uint256-

In addition to this, the source code for openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721.sol:71 requires the implementation of onERC721Received() or the inheritance of IERC721Receiver in NameWrapper.sol when tokens are transferred from the user to the NameWrapper contract.

Hello! CRYP70 here, thank you so much for allowing me to participate in the ENS contest. I have found a couple of valid gas optimisations for this platform which may help keep platform transaction prices down - please see my findings below. :)

++i saves more gass than i++

++i generally costs less gas than i++ or i = i + 1 (about 5 units per increment) because i++ must increment a value and then "return" the old value which means the program may need to hold two numbers in memory. When ++i is used, it will only ever use one number in memory.

See the example below for an simplified illustration:

pragma solidity ^0.8.13;

contract MyFavouriteCounter {

    uint public count;

    function incrementPrefixCount() public returns (uint) {
        count = 1;
        return (++count); // returns 2
    }

    function incrementPostfixCount() public returns (uint) {
        count = 1;
        return (count++); // returns 1
    }
}

I managed to identify this in the following locations:

• contracts/BytesUtils.sol:266
• contracts/dnssec-oracle/DNSSECImpl.sol:93
• contracts/dnssec-oracle/algorithms/EllipticCurve.sol:304
• contracts/ethregistrar/BulkRenewal.sol:41,56
• contracts/ethregistrar/ETHRegistrarController.sol:256

<array>.length can be cached as opposed to looking up the length with each interation

<array>.length can be cached in a variable so the program doesn't need to spend additional gas to determine the length of the target array. I believe memory arrays use 3 gas units through the mload opcode and calldata arrays will use 3 gas units via the calldataload opcode. See the very simple example below with resulting gas units spent (note that this is excluding gas optimisation of the index incrementer and includes gas price spent after reading from storage for both function calls).

pragma solidity 0.8.13;


contract MyFavouriteLooper {

    string[] testarr = ["a", "b"];

    function stored() public { // gas units spent: 23713
        uint256 len = testarr.length;
        for(uint256 i = 0; i < len; i++){

        }
    }

    function notstored() public { // gas units spent: 27479
        for(uint256 i = 0; i < testarr.length; i++){

        }

    }


}

I managed to identify this in the following solidity files:

• contracts/dnssec-oracle/DNSSECImpl.sol:93
• contracts/ethregistrar/BulkRenewal.sol:41,56
• contracts/ethregistrar/ETHRegistrarController.sol:256

Many thanks!