ENS contest - JohnSmith's results

Decentralised naming for wallets, websites, & more.

General Information

Platform: Code4rena

Start Date: 12/07/2022

Pot Size: $75,000 USDC

Total HM: 16

Participants: 100

Period: 7 days

Judge: LSDan

Total Solo HM: 7

Id: 145

League: ETH

ENS

Findings Distribution

Researcher Performance

Rank: 55/100

Findings: 2

Award: $119.40

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository ENS

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 8olidity, Aussie_Battlers, Bnke0x0, Ch_301, Critical, Deivitto, Dravee, ElKu, Funen, GimelSec, JC, JohnSmith, Lambda, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, RustyRabbit, Sm4rty, TomJ, Waze, _Adam, __141345__, alan724, asutorufos, benbaessler, berndartmueller, bin2chen, brgltd, bulej93, c3phas, cRat1st0s, cryptonue, cryptphi, csanuragjain, delfin454000, dxdv, exd0tpy, fatherOfBlocks, gogo, hake, hyh, joestakey, kyteg, lcfr_eth, minhtrng, p_crypt0, pashov, pedr02b2, philogy, rajatbeladiya, rbserver, rishabh, robee, rokinot, sach1r0, sashik_eth, seyni, simon135, svskaushik, zuhaibmohd, zzzitron

Awards

78.9385 USDC - $78.94

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

1). Transfer ownership should be more secure

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call claimOwnership() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

2). Event is missing indexed fields

Problem

Each event should use three indexed fields if there are three or more fields

Proof of Concept

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

event NameRegistered(
string name,
bytes32 indexed label,
address indexed owner,
uint256 baseCost,
uint256 premium,
uint256 expires
);
event NameRenewed(
string name,
bytes32 indexed label,
uint256 cost,
uint256 expires
);

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/IBaseRegistrar.sol#L9-L18

event NameMigrated(
        uint256 indexed id,
        address indexed owner,
        uint256 expires
    );
    event NameRegistered(
        uint256 indexed id,
        address indexed owner,
        uint256 expires
    );

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/INameWrapper.sol#L19-L25

event NameWrapped(
bytes32 indexed node,
bytes name,
address owner,
uint32 fuses,
uint64 expiry
);

Mitigation

Make more fields as indexed

3). Use of floating pragma

Problem

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Proof of Concept

All contracts in scope use floating pragma solidity ^0.8.4;

Mitigation

Please use fixed latest recent version like pragma solidity 0.8.13;

4). Missing event for critical parameter change

Problem

Events should be emitted when important values change

Proof of Concept

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

function setOwner(address newOwner) public owner_only {
        owner = newOwner;
    }

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L105-L110

function setMetadataService(IMetadataService _newMetadataService)
public
onlyOwner
{
metadataService = _newMetadataService;
}

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L137 upgradeContract = _upgradeAddress;

Mitigation

Emit an event on value change.

5). Shadowing State Variables

Problem

https://swcregistry.io/docs/SWC-119 In future it could go unnoticed and subsequently lead to security issues.

Proof of Concept

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L29-L30 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L91 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L110 now was a keyword and an alias to block.timestamp, some editors still recognize it, and here it is used like an another function argument.

Mitigation

Rename owner and now variables.

Findings Information

🌟 Selected for report: 0xKitsune

Also found by: 0x040, 0x1f8b, 0x29A, 0xNazgul, 0xNineDec, 0xsam, 8olidity, Aussie_Battlers, Aymen0909, Bnke0x0, CRYP70, Ch_301, Chom, Deivitto, Dravee, ElKu, Fitraldys, Funen, GimelSec, IllIllI, JC, JohnSmith, Lambda, MiloTruck, Noah3o6, RedOneN, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Tomio, Waze, _Adam, __141345__, ajtra, ak1, arcoun, asutorufos, benbaessler, brgltd, bulej93, c3phas, cRat1st0s, cryptonue, delfin454000, durianSausage, fatherOfBlocks, gogo, hake, hyh, joestakey, karanctf, kyteg, lcfr_eth, lucacez, m_Rassska, rajatbeladiya, rbserver, robee, rokinot, sach1r0, sahar, samruna, sashik_eth, seyni, simon135, zuhaibmohd

Awards

40.4596 USDC - $40.46

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

1). Caching storage variables in memory to save gas

Problem

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost from 100 to 2100 gas, while MLOAD and MSTORE cost 3 gas.

Proof of Concept

Instances include:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L336-L339 digests[digesttype] is read two times

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L233 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L239 commitments[commitment] is read two times

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L132-L141 upgradeContract is read six times

Mitigation

Assign storage value to memory variable and use this variable instead of loading from storage multipple times.

2). Custom Errors

Problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here https://blog.soliditylang.org/2021/04/21/custom-errors/

Custom errors are defined using the error statement

Proof of Concept

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

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

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

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L13 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L28 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L42

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/Controllable.sol#L17

Mitigation

Replace require statements with custom errors.

3). `<array>.length` should not be looked up in every loop of a for-loop

Problem

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas)
memory arrays use `MLOAD` (3 gas)
calldata arrays use `CALLDATALOAD` (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP <n>needed to store the stack offset</n>

Proof of Concept

Instances include:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256 for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L92 for (uint256 i = 0; i < accounts.length; ++i) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L205 for (uint256 i = 0; i < ids.length; ++i) {

Mitigation

Create a variable for array length:

uint length = nameOfArray.length;
for(uint i; i < length; ) { ... }

4). ++variable costs less gas than variable++, especially when it’s used in for-loops (--variable/variable-- too)

Problem

Prefix increments are cheaper than postfix increments. Saves 6 gas PER LOOP

Proof of Concept

Instances include:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L266 for(uint i = 0; i < len; i++) { https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L312 for(uint256 idx = off; idx < off + len; idx++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L93 for(uint i = 0; i < input.length; i++) {

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

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256 for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14 for(len = 0; i < bytelength; len++) {

Mitigation

change i++ to ++i

5). Default value initialization

Problem

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include: https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L56 for (uint idx = 0; idx < shortest; idx += 32) { https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L264 uint ret = 0; https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L264 for(uint i = 0; i < len; i++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L93 for(uint i = 0; i < input.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L50 uint count = 0; https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L310 for(uint i = 0; i < data.length + 31; i += 32) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256 for (uint256 i = 0; i < data.length; i++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L12 uint i = 0; https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14 for(len = 0; i < bytelength; len++) {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L92 for (uint256 i = 0; i < accounts.length; ++i) { https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L92 for (uint256 i = 0; i < ids.length; ++i) {

Mitigation

Remove explicit initialization for default values.

6). Unchecked arithmetic

Problem

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas.

Proof of Concept

Instances include:

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

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14-L27

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L13 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L45 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L49

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L92 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L205

Mitigation

Place the arithmetic operations in an unchecked block

    for (uint i; i < length;) {
        ...
        unchecked{ ++i; }
    }

7). Checks before effects to save gas

Problem

Doing some actions before a check that can revert whole transaction wastes gas.

Proof of Concept

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

function _consumeCommitment(
        string memory name,
        uint256 duration,
        bytes32 commitment
    ) internal {
        // Require a valid commitment (is old enough and is committed)
        require(
            commitments[commitment] + minCommitmentAge <= block.timestamp,
            "ETHRegistrarController: Commitment is not valid"
        );

        // If the commitment is too old, or the name is registered, stop
        require(
            commitments[commitment] + maxCommitmentAge > block.timestamp,
            "ETHRegistrarController: Commitment has expired"
        );
        require(available(name), "ETHRegistrarController: Name is unavailable");

        delete (commitments[commitment]);

        require(duration >= MIN_REGISTRATION_DURATION);
    }

There are two SLOADs and a SSTORE + other actions before check, that compares input to the constant and can revert transaction.

Mitigation

Put require(duration >= MIN_REGISTRATION_DURATION); on top.

ENS contest - JohnSmith's results

Decentralised naming for wallets, websites, & more.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-56] QA Report - $78.94

Findings Information

Awards

Labels

External Links

1). Transfer ownership should be more secure

2). Event is missing indexed fields

Problem

Proof of Concept

Mitigation

3). Use of floating pragma

Problem

Proof of Concept

Mitigation

4). Missing event for critical parameter change

Problem

Proof of Concept

Mitigation

5). Shadowing State Variables

Problem

Proof of Concept

Mitigation

[G-42] Gas Report - $40.46

Findings Information

Awards

Labels

External Links

1). Caching storage variables in memory to save gas

Problem

Proof of Concept

Mitigation

2). Custom Errors

Problem

Proof of Concept

Mitigation

3). <array>.length should not be looked up in every loop of a for-loop

Problem

Proof of Concept

Mitigation

4). ++variable costs less gas than variable++, especially when it’s used in for-loops (--variable/variable-- too)

Problem

Proof of Concept

Mitigation

5). Default value initialization

Problem

Proof of Concept

Mitigation

6). Unchecked arithmetic

Problem

Proof of Concept

Mitigation

7). Checks before effects to save gas

Problem

Proof of Concept

Mitigation

3). `<array>.length` should not be looked up in every loop of a for-loop