QA Report

[code423n4]

Table of Contents

Low Risk Issues

• Admin Address Change should be a 2-Step Process
• Immutable addresses should 0-Check
• Require should be used instead of Assert

Non-critical Issues

• Require Statements without Descriptive Revert Strings
• Best Practices of Source File Layout
• Use fixed compiler versions instead of floating version
• Unnecessary use of named returns
• Event is Missing Indexed Fields

 

Low Risk Issues

Admin Address Change should be a 2-Step Process

Issue

High privilege account such as admin / owner is changed with only single process.
This can be a concern since an admin / owner role has a high privilege in the contract and
mistakenly setting a new admin to an incorrect address will end up losing that privilege.

PoC

Owned.sol
18:    function setOwner(address newOwner) public owner_only {
19:        owner = newOwner;
20:    }

Mitigation

This can be fixed by implementing 2-step process. We can do this by following.
First make the setAdmin function approve a new address as a pending admin.
Next that pending admin has to claim the ownership in a separate transaction to be a new admin.

 

Immutable addresses should 0-Check

Issue

I recommend adding check of 0-address for immutable addresses.
Not doing so might lead to non-functional contract when it is updated to 0-address accidentally.

PoC

Total of 7 issues found through 3 contract.

• ETHRegistrarController.sol: 4 immutable addresses of "base", "prices", "reverseRegistrar" and "nameWrapper"
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L49-L65
• ReverseRegistrar.sol: 1 immutable addresses of "ens"
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L28-L29
• NameWrapper.sol: 2 immutable addresses of "ens" and "registrar"
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L49-L56

Mitigation

Add 0-address check for above immutable addresses.

 

Require should be used instead of Assert

Issue

Solidity documents mention that properly functioning code should never reach a failing assert statement
and if this happens there is a bug in the contract which should be fixed.
Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#panic-via-assert-and-error-via-require

PoC

2 of these issues was found

19:    function nameLength(bytes memory self, uint offset) internal pure returns(uint) {
20:        uint idx = offset;
21:        while (true) {
22:            assert(idx < self.length);
23:            uint labelLen = self.readUint8(idx);
24:            idx += labelLen + 1;
25:            if (labelLen == 0) {
26:                break;
27:            }
28:        }
29:        return idx - offset;
30:    }

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

49:    function labelCount(bytes memory self, uint offset) internal pure returns(uint) {
50:        uint count = 0;
51:        while (true) {
52:            assert(offset < self.length);
53:            uint labelLen = self.readUint8(offset);
54:            offset += labelLen + 1;
55:            if (labelLen == 0) {
56:                break;
57:            }
58:            count += 1;
59:        }
60:        return count;
61:    }

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

Mitigation

Replace assert by require.

 

Non-critical Issues

Require Statements without Descriptive Revert Strings

Issue

It is best practice to include descriptive revert strings for require statement for readability and auditing.

PoC

./wrapper/BytesUtil.sol:13:        require(offset + len <= self.length);
./ethregistrar/ETHRegistrarController.sol:57:        require(_maxCommitmentAge > _minCommitmentAge);
./ethregistrar/ETHRegistrarController.sol:121:        require(commitments[commitment] + maxCommitmentAge < block.timestamp);
./ethregistrar/ETHRegistrarController.sol:246:        require(duration >= MIN_REGISTRATION_DURATION);
./dnssec-oracle/Owned.sol:10:        require(msg.sender == owner);
./dnssec-oracle/BytesUtils.sol:12:        require(offset + len <= self.length);
./dnssec-oracle/BytesUtils.sol:146:        require(idx + 2 <= self.length);
./dnssec-oracle/BytesUtils.sol:159:        require(idx + 4 <= self.length);
./dnssec-oracle/BytesUtils.sol:172:        require(idx + 32 <= self.length);
./dnssec-oracle/BytesUtils.sol:185:        require(idx + 20 <= self.length);
./dnssec-oracle/BytesUtils.sol:199:        require(len <= 32);
./dnssec-oracle/BytesUtils.sol:200:        require(idx + len <= self.length);
./dnssec-oracle/BytesUtils.sol:235:        require(offset + len <= self.length);
./dnssec-oracle/BytesUtils.sol:262:        require(len <= 52);
./dnssec-oracle/BytesUtils.sol:268:            require(char >= 0x30 && char <= 0x7A);
./dnssec-oracle/BytesUtils.sol:270:            require(decoded <= 0x20);

Mitigation

Add descriptive revert strings to easier understand what the code is trying to do.

 

Best Practices of Source File Layout

Issue

I recommend following best practices of solidity source file layout for readability.
Reference: https://docs.soliditylang.org/en/v0.8.15/style-guide.html#order-of-layout

This best practices is to layout a contract elements in following order:
Pragma statements, Import statements, Interfaces, Libraries, Contracts

Inside each contract, library or interface, use the following order:
Type declarations, State variables, Events, Modifiers, Functions

PoC

1. NameWrapper.sol

• Modifier are not placed before functions
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L150
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L639
  https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L657

Mitigation

I recommend to follow best practice source file layout

 

Use fixed compiler versions instead of floating version

Issue

It is best practice to lock your pragma instead of using floating pragma.
The use of floating pragma has a risk of accidentally get deployed using latest complier
which may have higher risk of undiscovered bugs.
Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

./wrapper/INameWrapper.sol:2:pragma solidity ^0.8.4;
./wrapper/NameWrapper.sol:2:pragma solidity ^0.8.4;
./wrapper/IMetadataService.sol:1:pragma solidity >=0.8.4;
./wrapper/ERC1155Fuse.sol:2:pragma solidity ^0.8.4;
./wrapper/BytesUtil.sol:2:pragma solidity >=0.8.4;
./wrapper/Controllable.sol:2:pragma solidity ^0.8.4;
./ethregistrar/IETHRegistrarController.sol:1:pragma solidity >=0.8.4;
./ethregistrar/ETHRegistrarController.sol:1:pragma solidity >=0.8.4;
./ethregistrar/StringUtils.sol:1:pragma solidity >=0.8.4;
./resolvers/Resolver.sol:2:pragma solidity >=0.8.4;
./registry/ReverseRegistrar.sol:1:pragma solidity >=0.8.4;
./registry/ENS.sol:1:pragma solidity >=0.8.4;
./registry/IReverseRegistrar.sol:1:pragma solidity >=0.8.4;
./dnssec-oracle/DNSSECImpl.sol:2:pragma solidity ^0.8.4;
./dnssec-oracle/Owned.sol:1:pragma solidity ^0.8.4;
./dnssec-oracle/DNSSEC.sol:2:pragma solidity ^0.8.4;
./dnssec-oracle/BytesUtils.sol:1:pragma solidity ^0.8.4;
./dnssec-oracle/algorithms/Algorithm.sol:1:pragma solidity ^0.8.4;
./dnssec-oracle/SHA1.sol:1:pragma solidity >=0.8.4;
./dnssec-oracle/digests/Digest.sol:1:pragma solidity ^0.8.4;
./dnssec-oracle/RRUtils.sol:1:pragma solidity ^0.8.4;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Unnecessary use of named returns

Issue

Several function adds return statement even thought named returns variable are used.
Remove unnecessary named returns variable to improve code readability.
Also keeping the use of named returns or return statement consistent through out the whole project
if possible is recommended.

PoC

1. Remove returns variable "ret" of readUint8() BytesUtils.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L135-L136
2. Remove returns variable "rrset" of validateSignedSet() DNSSECImpl.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L110-L140
3. Remove returns variable "ret" of readName() RRUtils.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L38-L41
4. Remove returns variable "owner" of ownerOf() NameWrapper.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L90-L97
5. Remove returns variable "ret" of _addLabel() NameWrapper.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L741-L753
6. Remove returns variable "owner" and "fuses" of _getDataAndNormaliseExpiry() NameWrapper.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L825-L844
7. Remove returns variable "owner" and "fuses" of _getETH2LDDataAndNormaliseExpiry() NameWrapper.sol
   https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L846-L865

Mitigation

Remove unused named returns variable and keep the use of named returns or return statement consistent
through out the whole project if possible.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

./wrapper/INameWrapper.sol:19:    event NameWrapped(bytes32 indexed node, bytes name, address owner, uint32 fuses, uint64 expiry);
./wrapper/INameWrapper.sol:27:    event NameUnwrapped(bytes32 indexed node, address owner);
./wrapper/INameWrapper.sol:29:    event FusesSet(bytes32 indexed node, uint32 fuses, uint64 expiry);
./wrapper/Controllable.sol:9:    event ControllerChanged(address indexed controller, bool active);
./ethregistrar/ETHRegistrarController.sol:34:    event NameRegistered(string name, bytes32 indexed label, address indexed owner, uint256 baseCost, uint256 premium, uint256 expires);
./ethregistrar/ETHRegistrarController.sol:42:    event NameRenewed(string name, bytes32 indexed label, uint256 cost, uint256 expires);
./ethregistrar/IBaseRegistrar.sol:9:    event NameMigrated(uint256 indexed id, address indexed owner, uint256 expires);
./ethregistrar/IBaseRegistrar.sol:14:    event NameRegistered(uint256 indexed id, address indexed owner, uint256 expires);
./ethregistrar/IBaseRegistrar.sol:19:    event NameRenewed(uint256 indexed id, uint256 expires);
./resolvers/Resolver.sol:35:    event ContentChanged(bytes32 indexed node, bytes32 hash);
./registry/ENS.sol:5:    event NewOwner(bytes32 indexed node, bytes32 indexed label, address owner);
./registry/ENS.sol:8:    event Transfer(bytes32 indexed node, address owner);
./registry/ENS.sol:11:    event NewResolver(bytes32 indexed node, address resolver);
./registry/ENS.sol:14:    event NewTTL(bytes32 indexed node, uint64 ttl);
./registry/ENS.sol:17:    event ApprovalForAll(address indexed owner, address indexed operator, bool approved);
./dnssec-oracle/DNSSEC.sol:14:    event AlgorithmUpdated(uint8 id, address addr);
./dnssec-oracle/DNSSEC.sol:15:    event DigestUpdated(uint8 id, address addr);
./dnssec-oracle/SHA1.sol:4:    event Debug(bytes32 x);

Mitigation

Add up to 3 indexed fields when possible.