ENS contest - asutorufos's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=%2B%20price.premium))%20%7B-,payable(msg.sender).transfer(,-msg.value#L183 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=payable(msg.sender).transfer(msg.value%20%2D%20price.base)%3B#L204 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#:~:text=registrar.transferFrom(registrant%2C%20address(this)%2C%20tokenId)%3B#L231

Vulnerability details

Impact

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

The claimer smart contract does not implement a payable function. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.## Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. ETHRegistrarController.sol L#211

ETHRegistrarController.sol L#183

ETHRegistrarController.sol L#204

Tools Used

Manual review

Recommended Mitigation Steps

Used call instead.

    (bool success, ) = msg.sender.call{amount}("");
    require(success, "Transfer failed.");

L-1 USE OF FLOATING PRAGMA While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain. ERC1155Fuse.sol L#2 NameWrapper.sol L#2 Controllable.sol L#2

L-2 EVENTS NOT EMITTED FOR IMPORTANT STATE CHANGES When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

ReverseRegistrar.sol L#51

ReverseRegostrar.sol L#112

N-1 Unused named returns Using both named returns and a return statement isn’t necessary. Removing one of those can improve code clarity: [NameWrapper.sol L#825]https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#:~:text=function%20_getDataAndNormaliseExpiry(,%7D

G-1 <ARRAY>.LENGTH SHOULD NOT BE LOOKED UP IN EVERY LOOP OF A FOR-LOOP The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a sload(100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset 2 instances for this issue: ERC1155Fuse.sol L#92 ERC1155Fuse.sol L#205

G-2 REQUIRE()/REVERT() STRINGS LONGER THAN 32 BYTES COST EXTRA GAS Each extra chunk of byetes past the original 32 iincurs an MSTORE which costs 3 gas

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=%3E%200)%20%7B-,require(,)%3B,-%7D

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=(name%2C%20duration)%3B-,require(,)%3B,-_consumeCommitment(

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=%3A36%5D)%3B-,require(,)%3B,-resolver.functionCall

G-3 USING PRIVATE RATHER THAN PUBLIC FOR CONSTANTS, SAVE 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

There is 1 instance of this issue:

ETHRegistrarController.sol L#21

G-4 ABI.ENCODE() IS LESS EFFICIENT THAN ABI.ENCODEPACKED() There is one issue of this: ETHRegistrarController.sol L#106

G-5 USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas. https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=%3E%200)%20%7B-,require(,)%3B,-%7D

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=(name%2C%20duration)%3B-,require(,)%3B,-_consumeCommitment(

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#:~:text=%3A36%5D)%3B-,require(,)%3B,-resolver.functionCall