ENS contest - joestakey's results

QA Report

Table of Contents

LOW

• avoid using the native transfer method
• hash collision with abi.encodePacked
• Immutable addresses lack zero-address check
• Setters should check the input value

NON - CRITICAL

• Constructor visibility
• Events emitted early
• Events indexing
• Event should be emitted in setters
• Function missing comments
• Function order
• Interface functions are implicitly virtual
• Local variable shadowing
• Non-library files should use fixed compiler versions
• open TODOs
• Public functions can be external
• Revert strings missing
• SPDX missing
• Transfer of ownership can be two-steps

avoid using the native transfer method

PROBLEM

In ETHRegistrarController, the .transfer() method is used to refund the caller if they sent more ETH than the price required.

The transfer() call requires that the recipient has a payable callback, only providing 2300 gas for its operation. This means the following cases can cause the transfer to fail if register or renew are called by a smart contract:

• The contract does not have a payable callback
• The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)
• The contract is called through a proxy which itself uses up the 2300 gas

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

Staking.sol

payable(msg.sender).transfer(msg.value - (price.base + price.premium))
payable(msg.sender).transfer(msg.value - price.base)
payable(owner()).transfer(address(this).balance)

TOOLS USED

Manual Analysis

MITIGATION

Use call instead of transfer.

hash collision with abi.encodePacked

IMPACT

strings and bytes are encoded with padding when using abi.encodePacked. This can lead to hash collision when passing the result to keccak256

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ETHRegistrarController.sol

bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label))

ReverseRegistrar.sol

bytes32 reverseNode = keccak256(abi.encodePacked(ADDR_REVERSE_NODE, labelHash))
return keccak256(abi.encodePacked(ADDR_REVERSE_NODE, sha3HexAddress(addr)))

NameWrapper.sol

return keccak256(abi.encodePacked(node, labelhash))

TOOLS USED

Manual Analysis

MITIGATION

Use abi.encode() instead.

Immutable addresses lack zero-address check

IMPACT

constructors should check the address written in an immutable address variable is not the zero address. This is also valid for implementation addresses.

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

ETHRegistrarController.sol

base = _base
prices = _prices
reverseRegistrar = _reverseRegistrar
nameWrapper = _nameWrapper

ReverseRegistrar.sol

ens = ensAddr

NameWrapper.sol

ens = _ens
registrar = _registrar

TOOLS USED

Manual Analysis

MITIGATION

Add a zero address check for the immutable variables aforementioned.

Setters should check the input value

PROBLEM

Setters and initializers should check the input value - ie make revert if it is the zero address

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

DNSSECImpl.sol

function setAlgorithm
function setDigest

TOOLS USED

Manual Analysis

MITIGATION

Add non-zero address checks.

Constructor visibility

PROBLEM

Visibility (public / external) is not needed for constructors anymore since Solidity 0.7.0, see here

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Owned.sol

constructor() public

TOOLS USED

Manual Analysis

MITIGATION

Remove the public modifier.

Events emitted early

PROBLEM

Events should be emitted after all external calls, to prevent the risk of a function reverting while the event had been emitted.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ETHRegistrarController.sol

• NameRegistered() is emitted before the function refunds ETH to the caller

ReverseRegistrar.sol

• ReverseClaimed() is emitted before an external function call to ens

TOOLS USED

Manual Analysis

MITIGATION

emit these events at the end of the functions they are in.

Events indexing

PROBLEM

Events should use indexed fields, to make them easier to filter in logs. This can be interesting for fields such as address indicating the new owner of a token, or filter through which controllers have been made active.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

DNSSEC.sol

event AlgorithmUpdated(uint8 id, address addr)
event DigestUpdated(uint8 id, address addr)

ETHRegistrarController.sol

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)

INameWrapper.sol

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

Controllable.sol

event ControllerChanged(address indexed controller, bool active)

ENS.sol

event Transfer(bytes32 indexed node, address owner)
event NewResolver(bytes32 indexed node, address resolver)
event NewTTL(bytes32 indexed node, uint64 ttl)
event ApprovalForAll(address indexed owner,address indexed operator,bool approved)

TOOLS USED

Manual Analysis

MITIGATION

Add indexed fields to these events so that they have the maximum number of indexed fields possible.

Event should be emitted in setters

PROBLEM

Setters should emit an event so that Dapps can detect important changes to storage

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ReverseRegistrar.sol

function setDefaultResolver

NameWrapper.sol

function setMetadataService
function setUpgradeContract

TOOLS USED

Manual Analysis

MITIGATION

emit an event in all setters

Function missing comments

PROBLEM

Some functions are missing Natspec comments

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

RRUtils.sol

function readSignedSet
function rrs
function readDNSKEY
function readDS
function compareNames
function progress

SHA1.sol

function sha1

ETHRegistrarController.sol

All the functions are missing comments.

ReverseRegistrar.sol

function setDefaultResolver
function ownsContract

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Function order

PROBLEM

Functions should be ordered following the Solidity conventions. This means ordering functions based on their visibility.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Several contracts have external functions defined after public functions:

• DNSSECImpl.sol

• ETHRegistrarController.sol

Some have internal functions defined after private functions:

• ReverseRegistrar.sol

And this contract has both:

• NameWrapper.sol

TOOLS USED

Manual Analysis

MITIGATION

Order the functions as per the Solidity conventions: external, public, internal, private.

Interface functions are implicitly virtual

PROBLEM

All interface functions are implicitly virtual.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Algorithm.sol

function verify(bytes calldata key, bytes calldata data, bytes calldata signature) external virtual

Digest.sol

function verify(bytes calldata data, bytes calldata hash) external virtual

TOOLS USED

Compiler warnings

MITIGATION

Remove the virtual modifier from these functions.

Local variable shadowing

IMPACT

In NameWrapper.sol, there is local variable shadowing: several functions declare a local variable owner, which is the name of a state variable - inherited from Ownable. This will not lead to any error but can be confusing.

SEVERITY

Non-critical

PROOF OF CONCEPT

Instances include:

NameWrapper.sol

• address owner = ownerOf(uint256(node))
• (address owner, uint32 fuses, uint64 oldExpiry) = getData(uint256(node))
• address owner = ens.owner(node)
• (address owner, uint32 oldFuses, uint64 expiry) = getData(uint256(node))
• (address owner, uint32 oldFuses, uint64 oldExpiry) = getData(uint256(node))
• function setRecord(bytes32 node,address owner
• address owner = ens.owner(subnode)
• (owner, fuses, oldExpiry) = getData(uint256(node))
• (owner, fuses, oldExpiry) = getData(uint256(node))

TOOLS USED

Manual Analysis

MITIGATION

Add an underscore (_owner) to avoid shadowing.

Non-library files should use fixed compiler versions

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

None of the contracts in scope have fixed pragmas.

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

open TODOs

PROBLEM

There are open TODOs in the code. Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

DNSSECImpl.sol

// TODO: Check key isn't expired, unless updating key itself

TOOLS USED

Manual Analysis

MITIGATION

Remove the TODOs

Public functions can be external

PROBLEM

It is good practice to mark functions as external instead of public if they are not called by the contract where they are defined.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

DNSSECImpl.sol

function setAlgorithm()
function setDigest()

ETHRegistrarController.sol

function available()
function commit()
function register()
function withdraw()

ReverseRegistrar.sol

function setDefaultResolver()
function claimWithResolver()
function node()

NameWrapper.sol

function setUpgradeContract()
function setFuses()
function upgradeETH2LD()
function upgrade()
function setChildFuses()
function setSubnodeOwner()
function setSubnodeRecord()

TOOLS USED

Manual Analysis

MITIGATION

Declare these functions as external instead of public

Revert strings missing

PROBLEM

Require statements should have descriptive error strings, to make it easier to debug when a function reverts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

BytesUtils.sol

• require(offset + len <= self.length)
• require(idx + 2 <= self.length)
• require(idx + 4 <= self.length)
• require(idx + 32 <= self.length)
• require(idx + 20 <= self.length)
• require(len <= 32)
• require(idx + len <= self.length)
• require(offset + len <= self.length)
• require(len <= 52)
• require(char >= 0x30 && char <= 0x7A)
• require(decoded <= 0x20)

ETHRegistrarController.sol

• require(_maxCommitmentAge > _minCommitmentAge);
• require(commitments- [commitment] + maxCommitmentAge < block.timestamp)
• require(duration >= MIN_REGISTRATION_DURATION)

BytesUtil.sol

• require(offset + len <= self.length)

TOOLS USED

Manual Analysis

MITIGATION

Add descriptive revert strings

SPDX missing

PROBLEM

Several contracts are missing SPDX identifiers which correctly license the contract for open source development

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

• BytesUtils.sol
• RRUtils.sol
• SHA1.sol
• Owned.sol
• Algorithms.sol
• Digests.sol
• ETHRegistrarController.sol
• IETHRegistrarController.sol
• StringUtils.sol
• IBaseRegistrar.sol
• ENS.sol
• ReverseRegistrar.sol
• IReverseRegistrar.sol
• IMetadataService.sol

TOOLS USED

Compiler warnings

Transfer of ownership can be two-steps

PROBLEM

The oracle contract DNSSECImpl uses an implementation of Owned to determine its owner. Consider using a two-steps ownership transfer method in Owned, where the new owner needs to approve the ownership. This mitigates the risk of transferring ownership to a random address.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

Owned.sol

function setOwner

TOOLS USED

Manual Analysis

Gas Report

Table of Contents

• Array length should not be looked up in every iteration
• Bools for storage are expensive
• Caching storage variables in local variables to save gas
• Custom errors
• Functions with access control cheaper if payable
• Inline functions
• Mathematical optimizations
• Modifier instead of duplicate require
• Prefix increments
• Revert strings length
• unchecked arithmetic
• upgrade Solidity compiler version
• use Assembly for simple setters

Array length should not be looked up in every iteration

IMPACT

It wastes gas to read an array's length in every iteration of a for loop, even if it is a memory or calldata array: 3 gas per read.

PROOF OF CONCEPT

5 instances:

DNSSECImpl.sol

• for(uint i = 0; i < input.length; i++)

RRUtils.sol

• for(uint i = 0; i < data.length + 31; i += 32)

ETHRegistrarController.sol

• for (uint256 i = 0; i < data.length; i++)

ERC1155Fuse.sol

• for (uint256 i = 0; i < accounts.length; ++i)
• for (uint256 i = 0; i < ids.length; ++i)

TOOLS USED

Manual Analysis

MITIGATION

Caching the length in a variable before the for loop

Bools for storage are expensive

IMPACT

Booleans are more expensive than uint256: each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. Compared to using uint256, this costs an extra 100 gas, even one Gsset 20000 gas when changing from 'false' to 'true' - if this is not the first time setting it to true.

PROOF OF CONCEPT

2 instances:

ERC1155Fuse.sol

• mapping(address => mapping(address => bool)) private _operatorApprovals

Controllable.sol

• mapping(address=>bool) public controllers

TOOLS USED

Manual Analysis

MITIGATION

Replace bool with uint256. Use uint256(1) and uint256(2) instead of true/false

Caching storage variables in local variables to save gas

IMPACT

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

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length can yield significant gas savings if the array length is high

PROOF OF CONCEPT

2 instances:

ETHRegistrarController.sol

scope: _consumeCommitment:

• commitments- [commitment] is read twice, it can be cached with a local variable

• commitments- [commitment] + minCommitmentAge <= block.timestamp

• commitments- [commitment] + maxCommitmentAge > block.timestamp

NameWrapper.sol

scope: setUpgradeContract:

• upgradeContract is read 3 times, while it could be cached with the function argument _upgradeAddress:

• if (address(upgradeContract) != address(0))

• registrar.setApprovalForAll(address(upgradeContract), true)

• ens.setApprovalForAll(address(upgradeContract), true)

TOOLS USED

Manual Analysis

Constants can be private

IMPACT

Marking constants as private save gas upon deployment, as the compiler does not have to create getter functions for these variables. It is worth noting that a private variable can still be read using either the verified contract source code or the bytecode.

PROOF OF CONCEPT

2 instances:

ETHRegistrarController.sol

• uint256 public immutable minCommitmentAge
• uint256 public immutable maxCommitmentAge

TOOLS USED

Manual Analysis

MITIGATION

Make the constants private instead of public

• gas cost before change:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 2010419 · 6.4 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

• gas cost after change:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 1974221 · 6.4 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

This saves 36,198 gas upon deployment.

Custom Errors

IMPACT

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

Custom errors are defined using the error statement

PROOF OF CONCEPT

41 instances:

BytesUtils.sol

• require(offset + len <= self.length)
• require(idx + 2 <= self.length)
• require(idx + 4 <= self.length)
• require(idx + 32 <= self.length)
• require(idx + 20 <= self.length)
• require(len <= 32)
• require(idx + len <= self.length)
• require(offset + len <= self.length)
• require(len <= 52)
• require(char >= 0x30 && char <= 0x7A)
• require(decoded <= 0x20)

RRUtils.sol

• require(data.length <= 8192, "Long keys not permitted")

ETHRegistrarController.sol

• require(_maxCommitmentAge > _minCommitmentAge);
• require(resolver != address(0),"ETHRegistrarController: resolver is required when data is supplied")
• require(commitments- [commitment] + maxCommitmentAge < block.timestamp)
• require(msg.value >= (price.base + price.premium),"ETHRegistrarController: Not enough ether provided")
• require(msg.value >= price.base,"ETHController: Not enough Ether provided for renewal")
• require(commitments- [commitment] + minCommitmentAge <= block.timestamp,"ETHRegistrarController: Commitment is not valid")
• require(commitments- [commitment] + maxCommitmentAge > block.timestamp,"ETHRegistrarController: Commitment has expired")
• require(available(name), "ETHRegistrarController: Name is unavailable")
• require(duration >= MIN_REGISTRATION_DURATION)
• require(txNamehash == nodehash,"ETHRegistrarController: Namehash on record do not match the name being registered")

ReverseRegistrar.sol

• require(addr == msg.sender || controllers- [msg.sender] ||ens.isApprovedForAll(addr, msg.sender) || ownsContract(addr), "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself")
• require(address(resolver) != address(0),"ReverseRegistrar: Resolver address must not be 0")

BytesUtil.sol

• require(offset + len <= self.length)
• require(offset == self.length - 1, "namehash: Junk at end of name")
• require(idx < self.length, "readLabel: Index out of bounds")

ERC1155Fuse.sol

• require(account != address(0),"ERC1155: balance query for the zero address")
• require(accounts.length == ids.length,"ERC1155: accounts and ids length mismatch")
• require(msg.sender != operator,"ERC1155: setting approval status for self")
• require(to != address(0), "ERC1155: transfer to the zero address")
• require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")
• require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch")
• require(to != address(0), "ERC1155: transfer to the zero address")
• require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")
• require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")
• require(owner == address(0), "ERC1155: mint of existing token")
• require(newOwner != address(0), "ERC1155: mint to the zero address")
• require(newOwner != address(this),"ERC1155: newOwner cannot be the NameWrapper contract")
• require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")

Controllable.sol

• require(controllers- [msg.sender], "Controllable: Caller is not a controller")

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in ETHRegistrarController.sol:

-require(resolver != address(0),"ETHRegistrarController: resolver is required when data is supplied")
+if (resolver == address(0)) {
+		revert ResolverRequired();
+}

and define the custom error in the contract

+error ResolverRequired();

Functions with access control cheaper if payable

PROBLEM

A function with access control marked as payable will lbe cheaper for legitimate callers: the compiler removes checks for msg.value, saving approximately 20 gas per function call.

PROOF OF CONCEPT

6 instances:

DNSSECImpl.sol

• function setAlgorithm(uint8 id, Algorithm algo) public owner_only
• function setDigest(uint8 id, Digest digest) public owner_only

Owned.sol

• function setOwner(address newOwner) public owner_only

ReverseRegistrar.sol

• function claimForAddr(address addr,address owner,address resolver) public override authorised(addr)

NameWrapper.sol

• function setMetadataService(IMetadataService _newMetadataService) public onlyOwner
• function setUpgradeContract(INameWrapperUpgrade _upgradeAddress) public onlyOwner

TOOLS USED

Manual Analysis

MITIGATION

Mark these functions as payable

Inline functions

PROBLEM

When we define internal functions to perform computation:

• The contract’s code size gets bigger
• the function call requires additional stack operations, making it consume more gas than if it was inlined.

When it does not affect readability, it is recommended to inline internal functions that are called only once.

PROOF OF CONCEPT

3 instances:

DNNSECImpl.sol

• function verifyDSHash(uint8 digesttype, bytes memory data, bytes memory digest) internal

ETHRegistrarController.sol

• function _setReverseRecord(string memory name,address resolver,address owner) internal

ReverseRegistrar.sol

• function ownsContract(address addr) internal view returns (bool)

TOOLS USED

Manual Analysis

MITIGATION

Inline these functions where they are called. Another method is to use the viaIR compiler optimization flag, which helps with inline optimizations.

Mathematical optimizations

PROBLEM

X += Y costs 22 more gas than X = X + Y. This can mean a lot of gas wasted in a function call when the computation is repeated n times (loops)

PROOF OF CONCEPT

9 instances include:

RRUtils.sol

• idx += labelLen + 1
• offset += labelLen + 1
• count += 1
• off += 2
• off += 2
• off += 4
• off += 2
• counts -= 1
• i += 32

TOOLS USED

Manual Analysis

MITIGATION

use X = X + Y instead of X += Y (same with -). If Y ==1, use ++Y or --Y.

Modifier instead of duplicate require

PROBLEM

When a require statement is used multiple times, it is cheaper in deployment costs to use a modifier instead.

PROOF OF CONCEPT

4 instances include:

BytesUtils.sol

• require(offset + len <= self.length)
  require(offset + len <= self.length)

ERC1155Fuse.sol

• require(to != address(0), "ERC1155: transfer to the zero address")
  require(to != address(0), "ERC1155: transfer to the zero address")

• require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")
  require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")

• require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")
  require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")

TOOLS USED

Manual Analysis

MITIGATION

Use modifiers for these repeated statements

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments - 6 gas. This can mean interesting savings in for loops.

PROOF OF CONCEPT

5 instances:

BytesUtils.sol

• i++
• id++

DNSSECImpl.sol

• i++

ETHRegistrarController.sol

• i++

StringUtils.sol

• len++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Revert strings length

IMPACT

Revert strings cost more gas to deploy if the string is larger than 32 bytes.

PROOF OF CONCEPT

22 instances:

ETHRegistrarController.sol

• require(resolver != address(0),"ETHRegistrarController: resolver is required when data is supplied")
• require(msg.value >= (price.base + price.premium),"ETHRegistrarController: Not enough ether provided")
• require(msg.value >= price.base,"ETHController: Not enough Ether provided for renewal")
• require(commitments- [commitment] + minCommitmentAge <= block.timestamp,"ETHRegistrarController: Commitment is not valid")
• require(commitments- [commitment] + maxCommitmentAge > block.timestamp,"ETHRegistrarController: Commitment has expired")
• require(available(name), "ETHRegistrarController: Name is unavailable")
• require(txNamehash == nodehash,"ETHRegistrarController: Namehash on record do not match the name being registered")

ReverseRegistrar.sol

• require(addr == msg.sender || controllers- [msg.sender] ||ens.isApprovedForAll(addr, msg.sender) || ownsContract(addr), "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself")
• require(address(resolver) != address(0),"ReverseRegistrar: Resolver address must not be 0")

ERC1155Fuse.sol

• require(account != address(0),"ERC1155: balance query for the zero address")
• require(accounts.length == ids.length,"ERC1155: accounts and ids length mismatch")
• require(msg.sender != operator,"ERC1155: setting approval status for self")
• require(to != address(0), "ERC1155: transfer to the zero address")
• require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")
• require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch")
• require(to != address(0), "ERC1155: transfer to the zero address")
• require(from == msg.sender || isApprovedForAll(from, msg.sender),"ERC1155: caller is not owner nor approved")
• require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")
• require(newOwner != address(0), "ERC1155: mint to the zero address")
• require(newOwner != address(this),"ERC1155: newOwner cannot be the NameWrapper contract")
• require(amount == 1 && oldOwner == from,"ERC1155: insufficient balance for transfer")

Controllable.sol

• require(controllers- [msg.sender], "Controllable: Caller is not a controller")

TOOLS USED

Manual Analysis

MITIGATION

Write the error strings so that they do not exceed 32 bytes. For further gas savings, consider also using - custom errors.

For instance, changing all strings in ETHRegistrarController.sol to strings shorter than 32 bytes:

• gas cost before change:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 2010419 · 6.4 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

• gas cost after change:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 1932761 · 6.4 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

This saves 77,658 gas upon deployment, an average of 11,094 gas saved per revert string.

Unchecked arithmetic

IMPACT

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

9 instances:

BytesUtils.sol

• i++
• id++

DNSSECImpl.sol

• i++

ETHRegistrarController.sol

• msg.value - price.base cannot underflow because of the check - line 182
• msg.value - (price.base + price.premium) cannot underflow because of the check - line 203
• i++

StringUtils.sol

• len++

ERC1155Fuse.sol

• ++i
• ++i

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block.

For instance, doing it for the 3 instances mentioned in ETHRegistrarController:

• gas cost before changes:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Methods │ ································|·····················|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ································|·····················|·············|·············|···············|···············|·············· | ETHRegistrarController · register · 233372 · 377120 · 293714 · 15 · - │ ································|·····················|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 2010419 · 6.7 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

• gas cost after changes:

·-----------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ······················································|···························|···············|······························ | Methods │ ································|·····················|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ································|·····················|·············|·············|···············|···············|·············· | ETHRegistrarController · register · 233244 · 376921 · 293558 · 15 · - │ ································|·····················|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ······················································|·············|·············|···············|···············|·············· | ETHRegistrarController · - · - · 1997901 · 6.7 % · - │ ·-----------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

It saves 12,518 upon deployment, and an average of 156 gas per register call - here for data.length == 15, so an average of 10 gas per byte of data.

Upgrade Solidity compiler version

IMPACT

• 0.8.10 removes contract existence checks if the external call has a return value - 700 gas

PROOF OF CONCEPT

All the contracts in scope have the pragma set to ^0.8.4

TOOLS USED

Manual Analysis

MITIGATION

Upgrade these contracts compiler versions.

use Assembly for simple setters

IMPACT

Where it does not affect readability, using assembly allows to save gas not only on deployment, but also on function calls. This is the case for instance for simple admin setters.

PROOF OF CONCEPT

2 instances:

Owned.sol

• owner = newOwner

NameWrapper.sol

• metadataService = _newMetadataService

MITIGATION

Use assembly to write the new value in storage. For instance in NameWrapper.sol:

-  metadataService = _newMetadataService;
+  assembly {
+    sstore(metadataService.slot, _newMetadataService)
+  }

• gas cost before changes:

·---------------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ··························································|···························|···············|······························ | Methods │ ································|·························|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ································|·························|·············|·············|···············|···············|·············· | NameWrapper · setMetadataService · - · - · 29013 · 1 · - │ ································|·························|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ··························································|·············|·············|···············|···············|·············· | NameWrapper · - · - · 5250093 · 17.5 % · - │ ·---------------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

• gas cost after changes:

·---------------------------------------------------------|---------------------------|---------------|-----------------------------· | Solc version: 0.8.13 · Optimizer enabled: true · Runs: 10000 · Block limit: 30000000 gas │ ··························································|···························|···············|······························ | Methods │ ································|·························|·············|·············|···············|···············|·············· | Contract · Method · Min · Max · Avg · # calls · eur (avg) │ ································|·························|·············|·············|···············|···············|·············· | NameWrapper · setMetadataService · - · - · 28962 · 1 · - │ ································|·························|·············|·············|···············|···············|·············· | Deployments · · % of limit · │ ··························································|·············|·············|···············|···············|·············· | NameWrapper · - · - · 5238900 · 17.5 % · - │ ·---------------------------------------------------------|-------------|-------------|---------------|---------------|-------------·

It saves 11,193 gas upon deployment, and an average of 51 gas per function call.