ENS contest - Aussie_Battlers's results

Lines of code

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

Vulnerability details

Impact

Function NameWrapper.setSubnodeOwner can be used to transfer ownership of a sub-domain to a new owner and, at the same time, burn fuses. A possible use-case could be that a domain owner wants to transfer ownership of the sub-domain but burn fuses in order to restrict what the sub-domain owner can do.

Unfortunately, the sub-domain owner -- through a malicious contract -- can use the onERC1155Received hook to call back into the NameWrapper contract to perform actions that the fuses would prohibit. This is made possible because of the way _transferAndBurnFuses is written. A _transfer, which transfers control to the malicious smart contract, is done before setFuses is called.

The burning of fuses can prohibit:

• further burning of fuses
• setting the resolver
• setting the TTL (time to live)
• creating a sub-domain
• transferring the sub-domain

Oh all the things a domain holder might want to prohibit in sub-domains probably the most likely would be the setting of a resolver. The domain holder could, quite legitimately, want to prohibit a sub-domain holder to set a resolver that would cause a web browser to a dangerous IP address for the purpose of scamming.

Another thing a domain owner might want to prohibit is the creating of further sub-domains. Unfortunately, using this exploit the malicious sub-domain owner could create as many sub-sub-domains as they wanted.

In all cases, the situation is made much worse by the fact that once setSubnodeOwner has finished executing the domain owner can no longer perform actions on the sub-domain! This is because PARENT_CANNOT_CONTROL must have been burned. A parent cannot burn sub-domain fuses unless both PARENT_CANNOT_CONTROL and CANNOT_UNWRAP are also burned. See lines 954-962.

This exploit works equally well with NameWrapper.setSubnodeRecord because it also calls _transferAndBurnFuses.

Proof of Concept

• A domain owner calls setSubnodeOwner with parameter newOwner set to themselves in order to create a sub-domain which they still own.
• The domain owner then calls setSubnodeOwner again but with a newOwner set to the new sub-domain owner. They also use a value for fuses that locks down its functionality somehow. e.g. PARENT_CANNOT_CONTROL | CANNOT_UNWRAP | CANNOT_SET_RESOLVER.
• As part of the execution _transferAndBurnFuses is called which eventually executes line 820.
• The sub-domain owner is a smart contract that has set up its onERC1155Received hook to call back into NameWrapper to perform some action. e.g. setSubnodeOwner so it can create a new sub-domain or setRecord to set the resolver and the TTL. These calls succeed since
  • the NFT representing the ownership of the sub-domain has already been transferred to the smart contract, and
  • the fuses that the domain owner wanted burned have not been burned yet
• Finally line 821 is executed which burns the fuses that the domain owner wanted burned in the first place.

A proof of concept of the a) resolver setting and b) sub-domain creating scenarios can be executed by applying the diff in this gist and then running

$ npx hardhat test test/wrapper/BugsSetSubnodeOwner.js

Tools used

Manual inspection + hardhat

Recommended Mitigation Steps

The root cause of this issue is the lack of re-entrancy protection on many of the NameWrapper functions. Although costly in terms of gas, re-entrancy protection should be added perhaps using Open Zeppelin's ReentrancyGuard contract.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L182-L185 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#L210-L212

Vulnerability details

Impact

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

1. The receiver smart contract does not implement a payable function.

2. The receiver smart contract does implement a payable fallback which uses more than 2300 gas unit.

3. The receiver 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.

Recommended Mitigation Steps

Use call() instead of transfer() to send ETH. And return value must be checked to see if the call is successful. Sending ETH with the transfer is no longer recommended.

QA Report

Documentation fixes

• line 893. PARENT_CANNOT_REPLACE --> PARENT_CANNOT_CONTROL

---

Low Risk: [L-01] Unspecific Compiler Version Pragma

Impact

Avoid floating pragmas for non-library contracts.

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.

It is recommended to pin to a concrete compiler version.

Findings

• BytesUtils.sol:1
• DNSSECImpl.sol:2
• RRUtils.sol:1
• SHA1.sol:1
• Owned.sol:1
• DNSSEC.sol:2
• Algorithm.sol:1
• Digest.sol:1
• ETHRegistrarController.sol:1
• IETHRegistrarController.sol:1
• StringUtils.sol:1
• ReverseRegistrar.sol:1
• INameWrapper.sol:2
• Controllable.sol:2
• IMetadataService.sol:1
• ENS.sol:1
• Resolver.sol:2
• IReverseRegistrar.sol:1
• BytesUtil.sol:2
• ERC1155Fuse.sol:2
• NameWrapper.sol:2

---

Low Risk: [L-02] Missing checks for zero address

Impact

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

Findings

• DNSSECImpl.sol:59
• DNSSECImpl.sol:70
• ETHRegistrarController.sol:59-60
• ETHRegistrarController.sol:63-64
• ReverseRegistrar.sol:29
• NameWrapper.sol:54-56
• NameWrapper.sol:109
• NameWrapper.sol:137

---

Low Risk: [L-03] No Transfer Ownership Pattern

Description

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

Findings

• Owned.sol:18-20

---

Low Risk: [L-04] Fuses can be burned that don't exist (yet)

INameWrapper.sol defines 7 fuses but there is space for up to 32 in the uint32 type. Fuses appear to be one-way in the sense that you can burn them and they only get reset when the expiry runs out. setFuses allows arbitrary fuses to be burned because it accepts an arbitrary uint32 fuses parameter.

What happens when/if new fuses are added in the future? Would the old fuses get migrated? If so, could the fact that some have been set cause problems?

Recommended Mitigation Steps

Add a check to see if the fuses being burned are valid fuses in setFuses and revert otherwise.

---

Low Risk: [L-05] Use of block.timestamp

Description

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommendation

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Findings

• ETHRegistrarController.sol:L121-L122
• ETHRegistrarController.sol:L233
• ETHRegistrarController.sol:L239
• ERC1155Fuse.sol:L144

Low Risk: [L-06] Commitments can get very large

Description

A user can call commit() a large number of times to fill up the commitments mapping with expired or invalid data.

Commitments are only removed from the mapping when a valid commit is consumed

ETHRegistrarController.sol:L244

Recommend adding a function to periodically clear expired commitments.

Findings

ETHRegistrarController.sol:L122

---

Non-critical: [N-01] Use bytes.concat instead of abi.encodePacked

Function bytes.concat can be used in place of abi.encodePacked in many locations. e.g. NameWrapper.sol:733-753.

There are known issues with abi.encodePacked. Even those abi.encodePacked is safe as it is used in the ENS code-base there have been proposals to remove it from Solidity.

---

Non-critical: [N-02] Upgrade version Open Zeppelin contract dependency

The solution uses:

"@openzeppelin/contracts": "^4.1.0",

there has been multiple security patches released for openzeppelin/contracts since this version

Openzeppelin Security Advisories

Recommended Mitigation Steps

Upgrade @openzeppelin/contracts to 4.4.2 or higher

---

Non-critical: [N-03] Overflow checks in BaseRegistrarImplementation not quite right

Since Solidity 0.8 arithmetic overflow is checked and leads to a revert unless you use an unchecked block.

Line 122 and Line 141 both check whatever the new expiries[id] will be set to it will not overflow when GRACE_PERIOD is added to it.

However, in the case that you are trying to protect against, the expression inside the require will overflow itself before evaluating to true or false.

The function will revert, which is the intended effect, but for the wrong reason. Consider rewriting the logic as follows (just for clarity).

For _register

require(type(uint256.max) - (block.timestamp + duration) >= GRACE_PERIOD);

and for renew

require(type(uint256).max - (expiries[id] + duration) >= GRACE_PERIOD);

You might even want to add a user-friendly reason as to why the require failed.

Gas Optimization Report

Gas Optimizations: [G-01] For loop optimisations

Description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecessary gas.

Suggest not initializing the for loop counter to 0.

An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Suggest storing the array’s length in a variable before the for-loop, and use it instead:

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

taking all of the above, the recommended format for gas savings is

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

Findings

• DNSSECImpl.sol:L93
• RRUtils.sol:L310
• ETHRegistrarController.sol:L256
• StringUtils.sol:L14
• ERC1155Fuse.sol:L92
• ERC1155Fuse.sol:L205

Gas Optimizations: [G-02] Use calldata instead of memory

Description

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

Findings

• DNSSECImpl.sol:L80
• DNSSECImpl.sol:L110
• RRUtils.sol:L85
• RRUtils.sol:L97
• RRUtils.sol:L120
• ETHRegistrarController.sol:L67
• ETHRegistrarController.sol:L77
• ETHRegistrarController.sol:L81
• ETHRegistrarController.sol:L87
• ETHRegistrarController.sol:L126
• ReverseRegistrar.sol:L112
• ERC1155Fuse.sol:L78
• ERC1155Fuse.sol:L279
• ERC1155Fuse.sol:L307
• ERC1155Fuse.sol:L336-L337

Gas Optimizations: [G-03] Don't Initialize Variables with Default Value

Description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

Findings

• RRUtils.sol:L63
• RRUtils.sol:L181
• RRUtils.sol:L200
• StringUtils.sol:L12
• INameWrapper.sol:L16

Gas Optimizations: [G-04] Require statements should be checked first

Description

require statements can be placed earlier to reduce gas usage on revert ie move require to the top of the function if possible

Findings

• ETHRegistrarController.sol:L246