ENS contest - RedOneN's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L204 https://github.com/code-423n4/2022-07-ens/blob/main/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 claimer smart contract does not implement a payable function. 2 The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. 3 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.

Proof of Concept

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

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

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L211

Tools Used

Manual audit

Recommended Mitigation Steps

I recommend using call() instead of transfer() :

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

Lines of code

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

Vulnerability details

Impact

In NameWrapper.sol the wrapETH2LD() and unwrapETH2LD() functions call transferFrom() on a ERC721 token. This does not ensure that the token is not sent to an address that is not able to properly support it which could result in the loss of the token.

This is especially true for the unwrapETH2LD() function where the recipient is not the contract itself.

Note as well that openzepellin’s documentation discourages the use of transferFrom. Indeed, it is highly suggested to use safeTransferFrom() whenever possible.

Proof of Concept

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

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

Tools Used

Manual audit

Recommended Mitigation Steps

Call the safeTransferFrom() method instead of transferFrom(). Note that the contract should inherit the ERC721TokenReceiver contract as a consequence (which is already the case).

[G-01] It costs more gas to initialize variables to zero than to let the default of zero be applied.

This is mainly true for storage variables but also applies to memory variables.   8 instances over 5 files.

File: BytesUtils.sol BytesUtils.sol:56 BytesUtils.sol:266     File: DNSSECImpl.sol DNSSECImpl.sol:93   File: ETHRegistrarController.sol ETHRegistrarController.sol:256   File: StringUtils.sol StringUtils.sol:12 StringUtils.sol:14   File: ERC1155Fuse.sol ERC1155Fuse.sol:92 ERC1155Fuse.sol:205

[G-02] ++i/i++ should be UNCHECKED{++i}/UNCHECKED{i++} when it is not possible for them to overflow (eg. when used in for and while loops).

  8 instances over 5 files.

File: BytesUtils.sol BytesUtils.sol:56 BytesUtils.sol:266 BytesUtils.sol:313   File: DNSSECImpl.sol DNSSECImpl.sol:93   File: ETHRegistrarController.sol ETHRegistrarController.sol:256   File: StringUtils.sol StringUtils.sol:14   File: ERC1155Fuse.sol ERC1155Fuse.sol:92 ERC1155Fuse.sol:205

[G-03] ++i costs less gas than i++, especially when it's use in for loops

  10 instances over 6 files

File: BytesUtils.sol BytesUtils.sol:266 BytesUtils.sol:313   File: DNSSECImpl.sol DNSSECImpl.sol:93   File: RRUtils.sol RRUtils.sol:58 -> use ++counts instead of count +=1 RRUtils.sol:235 RRUtils.sol:241 RRUtils.sol:250 -> use --counts instead of counts -=1   File: ETHRegistrarController.sol ETHRegistrarController.sol:256   File: StringUtils.sol StringUtils.sol:14     File: ERC1155Fuse.sol ERC1155Fuse.sol:92

[G-04] - Splitting require() statements that use « && » saves gas

  3 instances over 2 files.

File: BytesUtils.sol BytesUtils.sol:268   File: ERC1155Fuse.sol ERC1155Fuse.sol:215 ERC1155Fuse.sol:290  

 

[G-05] - Duplicated require()/revert() checks should be refactored to a modifier or function

  7 instances in 1 file.

File: BytesUtils.sol expression : require(idx/offset + n <= self.length) BytesUtils.sol:12 BytesUtils.sol:146 BytesUtils.sol:159 BytesUtils.sol:172 BytesUtils.sol:185 BytesUtils.sol:200 BytesUtils.sol:235      

[G-06] - Consider a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

 

[G-07] - Functions guaranteed to revert when called by normal users can be marked payable

  8 instances over 3 files.

File: BytesUtils.sol BytesUtils.sol:58 BytesUtils.sol:69   File: ReverseRegistrar.sol ReverseRegistrar.sol:51 ReverseRegistrar.sol:76   File: NameWrapper.sol NameWrapper.sol:106 NameWrapper.sol:128 NameWrapper.sol:251 NameWrapper.sol:271          

[G-08] - Require() should be used instead of assert() to save gas left during a transaction.

  2 instances.

File: RRUtils.sol RRUtils.sol:22 RRUtils.sol:52          

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

  25 instances over 5 files.

File: RRUtils.sol RRUtils.sol:307   File: ETHRegistrarController.sol ETHRegistrarController.sol:99 ETHRegistrarController.sol:139 ETHRegistrarController.sol:196 ETHRegistrarController.sol:232 ETHRegistrarController.sol:238 ETHRegistrarController.sol:242 ETHRegistrarController.sol:259   File: ReverseRegistrar.sol ReverseRegistrar.sol:41 ReverseRegistrar.sol:52   File: BytesUtil.sol BytesUtil.sol:13 BytesUtil.sol:42   File: ERC1155Fuse.sol ERC1155Fuse.sol:60 ERC1155Fuse.sol:85 ERC1155Fuse.sol:107 ERC1155Fuse.sol:176 ERC1155Fuse.sol:177 ERC1155Fuse.sol:195 ERC1155Fuse.sol:199 ERC1155Fuse.sol:200 ERC1155Fuse.sol:215 ERC1155Fuse.sol:248 ERC1155Fuse.sol:249 ERC1155Fuse.sol:250 ERC1155Fuse.sol:290        

[G-10] require()/revert() strings longer than 32 bytes cost extra gas

  21 instances over 3 files

File: ETHRegistrarController.sol ETHRegistrarController.sol:101 ETHRegistrarController.sol:139 ETHRegistrarController.sol:198 ETHRegistrarController.sol:234 ETHRegistrarController.sol:240 ETHRegistrarController.sol:242 ETHRegistrarController.sol:261   File: ReverseRegistrar.sol ReverseRegistrar.sol:41 ReverseRegistrar.sol:54   File: ERC1155Fuse.sol ERC1155Fuse.sol:62 ERC1155Fuse.sol:87 ERC1155Fuse.sol:109 ERC1155Fuse.sol:176 ERC1155Fuse.sol:179 ERC1155Fuse.sol:197 ERC1155Fuse.sol:199 ERC1155Fuse.sol:202 ERC1155Fuse.sol:215 ERC1155Fuse.sol:249 ERC1155Fuse.sol:252 ERC1155Fuse.sol:292          

[G-11] Avoid redundant check of condition

  File: ETHRegistrarController.sol function : renew() ->  if(msg.value>price.base) could be met in the require statement at the begining of the funciton. ETHRegistrarController.sol:203   function : renew() ->  if(msg.value>price.base+price.premium) could met in the require statement at the begining of the funciton. ETHRegistrarController.sol:182          

[G-12] Require() statement should be place at the top of a function to save gas.

  File: ETHRegistrarController.sol ETHRegistrarController.sol:246 -> should be at least before the delete statement of line 244          

[G-13] Caching storage variable in memory to save gas :

  File: ETHRegistrarController.sol function: _consumeCommitment(), variable commitments[commitment] --> 2 sloads ETHRegistrarController.sol:233 ETHRegistrarController.sol:239   File: NameWrapper.sol function: setUpgradeContract(), variable upgradeContract --> 3 sloads NameWrapper.sol