ENS contest - Deivitto'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: 28/100

Findings: 2

Award: $222.29

🌟 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

177.9856 USDC - $177.99

Labels

bug

QA (Quality Assurance)

old-submission-method

External Links

Link to GitHub issue

Shadowing of variables

summary

Name shadowing where two or more variables/functions share the same name could be confusing to developers and/or reviewers.

details

in setAddr in Resolver.sol there is shadowing with addr variable that in case a contract inherits from Resolver.sol, when implementing setAddr function interface, they can't access to some functions with the same name but only to the local address variable:
- IAddrResolver.addr(bytes32) in IAddrResolver.sol#L15
- IAddressResolver.addr(bytes32,uint256) in IAddressResolver.sol#L10

github permalink

Case addr https://github.com/code-423n4/2022-07-ens/blob/main/contracts/resolvers/Resolver.sol#L43

Case owner variables https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L25 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L33 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L41 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L46 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L60 and the function https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L52

Case resolver variables https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L26 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L34 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L44 and the function https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L54

Case ttl variables https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L27 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L35 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L48 and the function https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol#L56

mitigation

change the variable name addr in setAddr to _addr or another alternative name for avoiding shadowing
change function owner to nodeOwner() or getOwner() or the variables owner passed as parameters to _owner
same with resolver to nodeResolver() or getResolver or resolver as parameter to _resolver
same with ttl to nodeTtl() or getTtl() or ttl as parameter to _ttl

Warning SPDX-License missing

summary

SPDX license should be included for avoiding compiler warnings.

details

Compilation warnings/errors on https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ENS.sol: Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.

github permalink

mitigation

Add a SPDX License to each file

interfaces functions are already virtual

summary

In interfaces functions are already virtual

details

The follow function is expliciting including virtual keyword what is not needed in interfaces:

function verify(bytes calldata key, bytes calldata data, bytes calldata signature) external virtual view returns (bool);

github permalink

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

mitigation

remove the virtual keyword in interfaces

Use of magic numbers is confusing and risky

a. Summary:

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

b. Details:

values are hardcoded and would be more readable and maintainable if declared as a constant Also they are used in operations with assembly, which is error prone

c. Github Permalinks:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L148 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L161 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L187 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L268-270

d. Mitigation:

Replace magic hardcoded numbers with declared constants.

Important functions should include events

a. Summary:

Code that can affect the correct usage of the contract should include events

b. Details:

The setOwner function has no events an it's an important part of the code

c. Github Permalinks:

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

d. Mitigation:

Add an event to the function

Missing indexed event parameters

a. Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

b. Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

event do not use indexed parameters.

c. Github Permalinks:

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

d. Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

visibility of the constructor is ignored

summary

public visibility in constructors are ignored an throws warning by the compiler

Details

Visibility for constructor is ignored. If you want the contract to be non-deployable, making it "abstract" is sufficient.

github permalink

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

mitigation

remove public keyword from constructor

visibility of setController should be declared external

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/Controllable.sol#L11-L14 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L18-L20 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSEC.sol#L18 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L120-L123 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L125 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L210-L212

function should have a way of access control

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

summary

ENS directions can be renewed by anyone, not only by the owner, this can create an direction to be locked so nobody can get it

details

this means that maybe somebody is expecting to get a direction that Alice is holding but left alone or maybe even lost control forever, then the user Mallory that doesn't want to let other users to get the direction pays for it blocking it so Bob can't get control of it

github permalinks

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

mitigation

add some short of user control

Naming convention of constants

a. Summary:

Constant naming convention is all upper case.

b. Details:

base32HexTable is a constant and should be in UPPER_CASE_WITH_UNDERSCORES as per Solidity Style Guide.

c. Github Permalinks:

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

d. Mitigation:

Rename the constant to all uppercase.

No pragma specified

Summary

No pragma specified, this may lead to wrong assumptions and deployments and even audits, since pragma version modifies many things

Details

Interface IBaseRegistrar has no pragma version specified

github permalinks

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

mitigation

Add pragma version, as I could see in the code, I would suggest ^0.8.4

explicit pragma experimental

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

Deprecated pragma ABIEncoderV2

a. Summary:

Since 0.8.0 ABI coder v2 is activated by default

b. Details:

The pragma pragma experimental ABIEncoderV2; is still valid, but it is deprecated and has no effect.

c. Github Permalinks:

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

d. Mitigation:

Consider removing it as it is deprecated and enabled by default. If you want to be explicit, please use pragma abicoder v2; instead.

Local variable shadows built-in symbol

a. Summary:

Name shadowing where two or more variables/functions share the same name could be confusing to developers and/or reviewers

b. Details:

Use of now keyword as a parameter

c. Github Permalinks:

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

d. Mitigation

Replace now variable in the function parameter to _now or a similar substitution

Missing Natspec

a. Summary:

Missing Natspec and regular comments affect readability and maintainability of a codebase.

b. Details:

Contracts has partial or full lack of comments

c. Github Permalinks:

d. mitigation

Add @param descriptors Add @return descriptors Add Natspec comments. Add inline comments. Add comments for what the contract does

Maximum line length exceeded

Summary:

Long lines should be wrapped to conform with Solidity Style guidelines.

Details:

Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Github Permalinks:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L10 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L182 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L201 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L202 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L248 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L449

Mitigation:

Comments and lines of code should be wrapped to a maximum of 79 (or 99) characters to help readers easily parse the comments.

bad order of code

summary

Clearness of the code is important for the readiability and manteniability. As Solidity guidelines says about declaration order: 1.Type declarations 2.State variables 3.Events 4.Modifiers 5.Functions

details

In this case, a constant is declared in the middle of diferent functions

github permalink

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtil#L252 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L104-L112 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L181-L191 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L200-L210 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L63-83

mitigation

Follow solidity style guidelines https://docs.soliditylang.org/en/v0.8.15/style-guide.html

Wrong function ordering assumption

summary

Comments says where the private and internal function starts, ordering this is a good practice as improves readiability and manteniability

Details

There is a function that should be included within this block and was forget to move there and can confuse any reader

github permalinks

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

mitigation

Move function _setData with the other internal functions

Deprecated code

a. Summary:

code that is deprecated may lead to wrong assumptions and should be removed for avoiding future problems of missasumptions

b. Details:

If code is deprecated, it should be remvoed from the code as it is replaced but something else or not used anymore

c. Github Permalinks:

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/resolvers/Resolver.sol#L35-L36 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/resolvers/Resolver.sol#L79-L86

d. Mitigation:

Remove code that is deprecated

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

44.3038 USDC - $44.30

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

Returns variable ignored

Summary

Not using the named return variables when a function returns wastes deployment gas

Details

There are functions that includes returns(type variableName) and then doesn't use that variableName that had been initialised. This consumes gas.

Github permalinks

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L94-L96 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L744-L752

Mitigation

returns variable ignored wastes extra gas, remove them or assigned them

use of custom errors rather than revert() / require()

summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

github permalinks

mitigation

replace each require by an error

use of > 0 costs more than != 0 on uints

summary

using > 0 costs 6 more gas than != 0 when used on a require() statement as negative numbers are not allowed in uint values

details

uint variables can't go negative so it can be improved the gas

github permalinks

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/BytesUtil.sol#L44 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L245 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L98

mitigation

replace > 0 to != 0 for extra gas savings by each time is called the condition

duplicated require() check should be refactored

summary

duplicated require() / revert() checks should be refactored to a modifier or function to save gas

details

Event appears twice and can be reduced

github permalinks

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

mitigation

refactor this checks to different functions to save gas

Index initialised in for loop

summary

for loops doens't need to initialize loop indexes to 0 as it is the default uint value this saves gas

details

Multiple initializations on loop's index to 0

github permalinks

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 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L56 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L266 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L313 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L310 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256

mitigation

Don't initialise the index

variable initialised without need

summary

The default value of the uints is 0

details

Not initializing to 0 explicitly saves gas on variable declarations

github permalinks

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L12 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/INameWrapper.sol#L16 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L264 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L50 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L63 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L181 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L200

mitigation

Don't initialise uint variables to 0

variables in a struct should be ordered by same data type to improve gas and storage

summary

the order in structs affects memory and gas usage

details

there are several structs with no order on the variable types

github permalinks

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

mitigation

put together all the same data types

use of counter++ in loop rather than ++counter

summary

++i costs less gas than i++, especially when it's used in for loops

details

using ++i doesn't affect the flow of regular for loops and improves gas cost

github permalinks

mitigation

Substitute to ++variableName when posible

use of var-- where it can be substitute by --var

summary

--i costs less gas than i--

details

i-- is used within a while loop and can be optimised to --i

github permalinks

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

mitigation

replace i-- to --i

unchecked index in loop

summary

unchecked operations as the ++i on for loops are cheaper than checked one. If they are not supposed to reach the maximum value, they don't need to check for overflow

details

Gas can be optimised

github permalinks

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L92-L94 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L205-L220 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/StringUtils.sol#L14-L31 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L266-L275 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L256-L267

mitigation

add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

loop length stored in local variable for performance

summary

loop length variables assigned to a local variable improves gas usage

details

loop length is used without a uint variable

github permalinks

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

mitigation

assign lengths to local variable for loops or when they are used several times

variable should be declared constant

summary

variables that can be constant should be constant for gas usage

details

variable is only assigned 1 so should be constant

github permalinks

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

mitigation

assigned it to constant

Functions guaranteed to revert when called by normal users can be marked payable

Summary

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.

Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

Details

The extra opcodes avoided are: CALLVALUE (2), DUP1 (3), ISZERO (3), PUSH2 (3), JUMPI (10), PUSH1 (3), DUP1 (3), REVERT(0), JUMPDEST (1), POP (2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

Github permalinks

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

Mitigation

It's suggested to add payable to functions guaranteed to revert when called by normal users to improve gas costs

ENS contest - Deivitto's results

Decentralised naming for wallets, websites, & more.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-32] QA Report - $177.99

Findings Information

Awards

Labels

External Links

Shadowing of variables

summary

details

github permalink

mitigation

Warning SPDX-License missing

summary

details

github permalink

mitigation

interfaces functions are already virtual

summary

details

github permalink

mitigation

Use of magic numbers is confusing and risky

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation:

Important functions should include events

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation:

Missing indexed event parameters

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation:

visibility of the constructor is ignored

summary

Details

github permalink

mitigation

visibility of setController should be declared external

function should have a way of access control

summary

details

github permalinks

mitigation

Naming convention of constants

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation:

No pragma specified

Summary

Details

github permalinks

mitigation

explicit pragma experimental

Deprecated pragma ABIEncoderV2

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation:

Local variable shadows built-in symbol

a. Summary:

b. Details:

c. Github Permalinks:

d. Mitigation

Missing Natspec

a. Summary:

b. Details:

c. Github Permalinks:

d. mitigation

Maximum line length exceeded

Summary: