search

code-423n4 / 2022-07-ens-findings

0 stars 0 forks source link

QA Report #131

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

QA Report

Documentation fixes

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

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

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

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

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.

jefflau commented 2 years ago

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

BaseRegistrarImplementation is out of scope and was deployed before 0.8