ENS contest - Ch_301's results

Lines of code

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

Vulnerability details

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 that 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 a proxy, raising the call’s gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multi-sig wallets.

File:  main/contracts/ethregistrar/ETHRegistrarController.sol

183:     payable(msg.sender).transfer
204:     payable(msg.sender).transfer(msg.value - price.base);
211:     payable(owner()).transfer(address(this).balance);

Recommended Mitigation Steps

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

Missing event for critical parameter change

Findings

File:  main/contracts/ethregistrar/ETHRegistrarController.sol

210:    function withdraw() public {
211:    payable(owner()).transfer(address(this).balance);
212:    }

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

Missing checks for address(0x0) when assigning values to register a new name

Findings

File:  main/contracts/ethregistrar/ETHRegistrarController.sol

88:  address owner
127:  address owner

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

Misleading comment

Findings

File:     /main/contracts/registry/ReverseRegistrar.sol

16:       // namehash('addr.reverse')

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L16

Use complex pragmas

Description Complex pragmas are also possible using ‘>’,’>=‘,’<‘ and ‘<=‘ symbols to combine multiple versions e.g. “pragma solidity >=0.8.0 <0.8.3;”

Findings SHA1.sol ETHRegistrarController.sol IETHRegistrarController.sol StringUtils.sol ReverseRegistrar.sol IReverseRegistrar.sol BytesUtil.sol IMetadataService.sol ENS.sol Resolver.sol

Use of floating pragma

Description A ‘^’ symbol prefixed to x.y.z in the pragma indicates that the source file may be compiled only from versions starting with x.y.z until x.(y+1).z. For e.g., “pragma solidity ^0.8.3;” indicates that source file may be compiled with compiler version starting from 0.8.3 until any 0.8.z but not 0.9.z. This is known as a “floating pragma.”.

Findings BytesUtils.sol DNSSECImpl.sol RRUtils.sol Owned.sol DNSSEC.sol Algorithm.sol Digest.sol ERC1155Fuse.sol NameWrapper.sol INameWrapper.sol Controllable.sol INameWrapperUpgrade.sol

public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public.

Findings

File: /main/contracts/dnssec-oracle/DNSSECImpl.sol

58:  function setAlgorithm(uint8 id, Algorithm algo) public owner_only 
69:  function setDigest(uint8 id, Digest digest) public owner_only 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L58

File:  /main/contracts/ethregistrar/ETHRegistrarController.sol

120:   function commit(bytes32 commitment) public override 
125:   function register
210:   function withdraw() public

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

File: /main/contracts/registry/ReverseRegistrar.sol

51:    function setDefaultResolver(address resolver) public override onlyOwner
97:     function claimWithResolver(address owner, address resolver)

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L51

emitting event after set all the changes

Findings

File:  main/contracts/ethregistrar/ETHRegistrarController.sol


173:        emit NameRegistered(
174:     name,
175:      keccak256(bytes(name)),
176:      owner,
177:      price.base,
178:      price.premium,
179:      expires
180:  );
181:
182:
183:  if (msg.value > (price.base + price.premium)) {
184:     payable(msg.sender).transfer(
185:         msg.value - (price.base + price.premium)
186:      );
187:  }

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

Calldata Instead Of Memory For Ro Function Parameters

Description

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

It is better to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

Findings

File: /main/contracts/dnssec-oracle/DNSSECImpl.sol

80:  function verifyRRSet(RRSetWithSignature[] memory input) external virtual view override returns(bytes memory)

91:      function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {

110:      function validateSignedSet(RRSetWithSignature memory input, bytes memory proof, uint256 now) internal view returns(RRUtils.SignedSet memory rrset)

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L80

File:  main/contracts/ethregistrar/ETHRegistrarController.sol

67:    function rentPrice(string memory name, uint256 duration)
77:    function valid(string memory name) public pure returns (bool)
81:    function available(string memory name) public view override returns (bool)
87:    string memory name
227:  string memory name
271:  string memory name

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

It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Findings

File:  /main/contracts/dnssec-oracle/BytesUtils.sol

56:        for (uint idx = 0; idx < shortest; idx += 32)
256:     for(uint i = 0; i < len; i++)

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L56

File: /main/contracts/dnssec-oracle/DNSSECImpl.sol

93:       for(uint i = 0; i < input.length; i++) 

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

File: /main/contracts/ethregistrar/ETHRegistrarController.sol

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

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

Using Prefix Increments Save Gas

Description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

Findings

File:  /main/contracts/dnssec-oracle/BytesUtils.sol

256:     for(uint i = 0; i < len; i++)

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L266

File: /main/contracts/dnssec-oracle/DNSSECImpl.sol

93:       for(uint i = 0; i < input.length; i++) 

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

File: /main/contracts/ethregistrar/ETHRegistrarController.sol

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

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

<array>.length Should Not Be Looked Up In Every Loop Of A For-loop

description The code can be optimized by minimizing the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

Findings

File: /main/contracts/dnssec-oracle/DNSSECImpl.sol

93:       for(uint i = 0; i < input.length; i++) 

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

File: /main/contracts/ethregistrar/ETHRegistrarController.sol

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

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

require()/revert() strings longer than 32 bytes cost extra gas

description Each extra chunk of byetes past the original 32 incurs an MSTORE which costs 3 gas

Findings

File: /main/contracts/ethregistrar/ETHRegistrarController.sol

101:       "ETHRegistrarController: resolver is required when data is supplied"
139:       "ETHRegistrarController: Not enough ether provided"
198:       "ETHController: Not enough Ether provided for renewal"
234:       "ETHRegistrarController: Commitment is not valid"
240:       "ETHRegistrarController: Commitment has expired"
242:        require(available(name), "ETHRegistrarController: Name is unavailable");
261:        "ETHRegistrarController: Namehash on record do not match the name being registered"

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

File: /main/contracts/registry/ReverseRegistrar.sol

46:     "ReverseRegistrar: Caller is not a controller or authorized by address or the address itself"
54:     "ReverseRegistrar: Resolver address must not be 0"

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L46

Use custom errors rather than revert()/require() strings to save deployment gas

description Custom errors are available from solidity version 0.8.4. The instances below match or exceed that version

Findings

File: /main/contracts/ethregistrar/ETHRegistrarController.sol

101:       "ETHRegistrarController: resolver is required when data is supplied"
139:       "ETHRegistrarController: Not enough ether provided"
198:       "ETHController: Not enough Ether provided for renewal"
234:       "ETHRegistrarController: Commitment is not valid"
240:       "ETHRegistrarController: Commitment has expired"
242:        require(available(name), "ETHRegistrarController: Name is unavailable");
261:        "ETHRegistrarController: Namehash on record do not match the name being registered"

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

File: /main/contracts/registry/ReverseRegistrar.sol

46:     "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
54:     "ReverseRegistrar: Resolver address must not be 0"

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L46