ENS contest - 0xNineDec's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183 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 contract ETHRegistrarController uses to send ETH payable(address).transfer(amount). This method of sending ETH use a fixed amount of gas (2300) that can revert while executing if, for example the gas amounts change in the future or the whole call uses more than 2300 gas units, preventing users to perform several actions.

Proof of Concept

Currently ETHRegistrarController uses this method to send value while calling register(), renew() and withdraw().

If the caller of those functions is a contract that match any of the following conditions, the execution will be reverted because the amount of gas used:

• It lacks from a payable function.
• Implements a payable fallback which consumes more than 2300 gas units.
• Implements a payable fallback that is called via a proxy (process that raises the gas consumption over 2300 gas units).
• Uses a multisig wallet that requires more than 2300 gas units for the executed transactions.

Any of those scenarios will prevent each call from being executed. A caveat on withdraw(), if the owner() is using a multisig that raises the gas consumption on each call, it may be needed to even change either the multisig provider for one that has a gas consumption within the mentioned range.

It is remarked that in BulkRenewal.sol, it is also used transfer().

Recommended Mitigation Steps

It is advised to avoid using transfer() or send() for sending ether and use call() instead.
For example:

(bool success, ) = _to.call{value: msg.value}("");
require(success, "!success");

Also, as a reminder, it is important to evaluate reentrancy. If that may be an issue, it is advisable using a reentrancy mutex for the function.

QA: MISSING SPDX LICENSE IDENTIFIER

The following contract files do not have a license identifier:

• BytesUtils.sol
• RRUtils.sol
• SHA1.sol
• Algorithm.sol
• Digest.sol
• ETHRegistrarController.sol
• IETHRegistrarController.sol
• StringUtils.sol
• IBaseRegistrar.sol
• ReverseRegistrar.sol
• IReverseRegistrar.sol
• IMetadataService.sol
• ENS.sol

Consider adding the SPDX License Identifier to those files and any other file where it is missing.

L: MISSING NATSPEC & UNRESOLVED TODO's

The majority of the code is well-commented. However, there are some places that are missing NATSPEC:

• ETHRegistrarController, NameWrapper: Those codebases completely lack from NATSPEC.
• ReverseRegistrar: There is no NATSPEC on the beginning explaining what these contracts are meant to do.
• DNSSECImp.sol Line 238: Has an open TODO.

Consider updating/adding the NATSPEC not only from the places mentioned, but also from any other places where it might be a lack of comments / bad placed comments in order to provide a higher quality codebase.

L: MISSING PRAGMA VERSION

The following file do not specify a pragma compiler version / range:

• IBaseRegistrar.sol

Please consider adding missing pragmas to this file and any other files that may have this missing.

G: USING CUSTOM ERRORS

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met). More information about custom errors can be seen here.

Errors are declared using the keyword error and can have parameters within them. They have a selector like any other function and can be declared within interfaces, contracts and libraries.

G: SHORTENING REVERT STRINGS SIZE

Revert strings that use up to 32 bytes will occupy only one storage slot and will subsequently increase the storage slots used as integer divisor of 32 bytes. For example, a 64 bytes string will take 2 slots whereas a 65 bytes string will require using 3.

There are some revert strings within the contracts that use more than 32 bytes. Some of them even use 67 bytes like: "ETHRegistrarController: resolver is required when data is supplied" (which is indeed using 32 + 32 + 3 bytes, 3 slots).

If error strings are decided to be used, it is recommended to shorten the revert strings to fit them in 32 bytes in order to decrease both deployment and runtime (if there is a reversal) gas usage.

G: FOR LOOPS: CACHING ARRAY SIZE

Some of the loops where this can be applied:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L93 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L256

Calculating the array length on each iteration within a for loop consumes around 6 gas units. If the array.length is cached before looping, around 3 gas units will be saved on each iteration.

It is recommended to apply this also in any other for loop that uses a fixed length of iterations.

G: FOR LOOPS: USING ++i INSTEAD OF i++

Some of the loops where this can be applied:

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L93 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L256

Replacing i++ with unchecked{++i;} (whenever there is no overflow risk) or even only ++i as the strategy to increase the counter saves gas on each iteration. Using ++i saves around 4 gas units per iteration against i++ and unchecking the counter saves considerably more.

Whenever there is no risk of overflowing the counter it is advised to increment the counter using the unchecked way. If overflow is a risk, it is allowed to use ++i instead.