ENS contest - TomJ'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: 35/100

Findings: 3

Award: $160.83

🌟 Selected for report: 0

🚀 Solo Findings: 0

Awards

5.45 USDC - $5.45

Labels

bug
duplicate
2 (Med Risk)

External Links

Lines of code

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

Vulnerability details

Impact

It is recommended to avoid the usage of payable.transfer, since it can cause the transaction to fail when the user is accessing this function with a smart contract and:

  1. does not have payable function
  2. have a payable function but spends more than 2300 gas
  3. have a payable function and spends less than 2300 gas but is called through proxy which uses over 2300 gas.

Reference: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Also there is no access control for payable.transfer at withdraw function (line 210-212) and anyone can call this function to transfer all the ether in this contract to owner of the contract. I suggest to add access control so that only owner can call this.

Proof of Concept

./ethregistrar/ETHRegistrarController.sol:183:            payable(msg.sender).transfer(
./ethregistrar/ETHRegistrarController.sol:204:            payable(msg.sender).transfer(msg.value - price.base);
./ethregistrar/ETHRegistrarController.sol:211:        payable(owner()).transfer(address(this).balance);

Tools Used

Manual Analysis

I recommend using low-level call() or OpenZeppelin Address.sendValue instead of transfer().

#0 - jefflau

2022-07-22T09:52:16Z

Duplicate of #133

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.

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
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.

 

Table of Contents

  • Should Use Unchecked Block where Over/Underflow is not Possible
  • Minimize the Number of SLOADs by Caching State Variable
  • Defined Variables Used Only Once
  • Redundant Use of Variable
  • Duplicate require() Checks Should be a Modifier or a Function
  • Use require instead of &&
  • Use Calldata instead of Memory for Read Only Function Parameters
  • Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
  • Unnecessary Default Value Initialization
  • ++i Costs Less Gas than i++
  • Keep The Revert Strings of Error Messages within 32 Bytes
  • Use Custom Errors to Save Gas

 

Should Use Unchecked Block where Over/Underflow is not Possible

Issue

Since Solidity 0.8.0, all arithmetic operations revert on overflow and underflow by default. However in places where overflow and underflow is not possible, it is better to use unchecked block to reduce the gas usage. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#checked-or-unchecked-arithmetic

PoC
  1. register function of ETHRegistrarController.sol
Wrap line 184 with unchecked since underflow is not possible due to line 182 check 182: if (msg.value > (price.base + price.premium)) { 184: msg.value - (price.base + price.premium)

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

  1. renew function of ETHRegistrarController.sol
Wrap line 204 with unchecked since underflow is not possible due to line 203 check 203: if (msg.value > price.base) { 204: payable(msg.sender).transfer(msg.value - price.base);

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

Mitigation

Wrap the code with uncheck like described in above PoC.

 

Minimize the Number of SLOADs by Caching State Variable

Issue

SLOADs cost 100 gas where MLOADs/MSTOREs cost only 3 gas. Whenever function reads storage value more than once, it should be cached to save gas.

PoC
  1. Cache digests[digesttype] variable of verifyDSHash() of DNSSECImpl.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L336 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L339

  2. Cache commitments[commitment] variable of _consumeCommitment() of ETHRegistrarController.sol 2 SLOADs to 1 SLOAD, 1 MSTORE and 2 MLOADs 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

  3. Cache upgradeContract variable of setUpgradeContract() of NameWrapper.sol Since there is state change of upgradeContract in middle of the function, we can cache it 2 times like beforeUpgradeContract and afterUpgradeContract. 6 SLOADs to 2 SLOAD, 2 MSTORE and 6 MLOADs (not including the state change in line 137) https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L132-L141

Cache it like the following: INameWrapperUpgrade beforeUpgradeContract = upgradeContract if (address(beforeUpgradeContract) != address(0)) { registrar.setApprovalForAll(address(beforeUpgradeContract), false); ens.setApprovalForAll(address(beforeUpgradeContract), false); } upgradeContract = _upgradeAddress; INameWrapperUpgrade afterUpgradeContract = upgradeContract if (address(afterUpgradeContract) != address(0)) { registrar.setApprovalForAll(address(afterUpgradeContract), true); ens.setApprovalForAll(address(afterUpgradeContract), true);
Mitigation

When certain state variable is read more than once, cache it to local variable to save gas.

 

Defined Variables Used Only Once

Issue

Certain variables is defined even though they are used only once. Remove these unnecessary variables to save gas. For cases where it will reduce the readability, one can use comments to help describe what the code is doing.

PoC
  1. len of readName() RRUtils.sol
return self.substring(offset, nameLength(self, offset));
  1. label of rentPrice() ETHRegistrarController.sol
price = prices.price(name, base.nameExpires(uint256(keccak256(bytes(name)))), duration);
  1. label of available() ETHRegistrarController.sol
return valid(name) && base.available(uint256(keccak256(bytes(name))));
  1. label of makeCommitment() ETHRegistrarController.sol
keccak256(bytes(name)),
  1. nodehash of _setRecords() ETHRegistrarController.sol
txNamehash == keccak256(abi.encodePacked(ETH_NODE, label)),
  1. name of getFuses() NameWrapper.sol
if (names[node].length == 0) {
Mitigation

Don't define variable that is used only once. Details are listed on above PoC.

 

Redundant Use of Variable

Issue

Below has redundant use of variables. Instead of defining a new variable, emit the event first and then update the variable.

PoC
  1. _burn function of ERC1155Fuse.sol https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L267-L272 Change it to
function _burn(uint256 tokenId) internal virtual { emit TransferSingle(msg.sender, ownerOf(tokenId), address(0x0), tokenId, 1); // Clear fuses and set owner to 0 _setData(tokenId, address(0x0), 0, 0); }
Mitigation

Instead of defining a new variable, emit the event first and then update the variable like shown in above PoC.

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC
./wrapper/ERC1155Fuse.sol:176: require(to != address(0), "ERC1155: transfer to the zero address"); ./wrapper/ERC1155Fuse.sol:199: require(to != address(0), "ERC1155: transfer to the zero address");
./wrapper/ERC1155Fuse.sol:215-218: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer"); ./wrapper/ERC1155Fuse.sol:290-293: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer");
Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Use require instead of &&

Issue

When there are multiple conditions in require statement, break down the require statement into multiple require statements instead of using && can save gas.

PoC
./dnssec-oracle/BytesUtils.sol:268: require(char >= 0x30 && char <= 0x7A);
Mitigation

Break down into several require statement. For example,

require(char >= 0x30); require(char <= 0x7A);

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

70 of these issues was found through 10 contracts.

wrapper/NameWrapper.sol:
541:        string memory label,
741:    function _addLabel(string memory label, bytes memory name)
773:        bytes memory name,
786:        string memory label,
886:        string memory label,
wrapper/ERC1155Fuse.sol:
78:    function balanceOfBatch(address[] memory accounts, uint256[] memory ids)
174:        bytes memory data
191:        uint256[] memory ids,
192:        uint256[] memory amounts,
193:        bytes memory data
279:        bytes memory data
307:        bytes memory data
336:        uint256[] memory ids,
337:        uint256[] memory amounts,
338:        bytes memory data
wrapper/BytesUtil.sol:
12:    function keccak(bytes memory self, uint offset, uint len) internal pure returns (bytes32 ret) {
25:    function namehash(bytes memory self, uint offset) internal pure returns(bytes32) {
41:    function readLabel(bytes memory self, uint256 idx) internal pure returns (bytes32 labelhash, uint newIdx) {
ethregistrar/ETHRegistrarController.sol:
67:    function rentPrice(string memory name, uint256 duration)
77:    function valid(string memory name) public pure returns (bool) {
81:    function available(string memory name) public view override returns (bool) {
87:        string memory name,
227:        string memory name,
271:        string memory name,
ethregistrar/StringUtils.sol:
10:    function strlen(string memory s) internal pure returns (uint) {
registry/ReverseRegistrar.sol:
112:    function setName(string memory name) public override returns (bytes32) {
136:        string memory name
dnssec-oracle/DNSSECImpl.sol:
46:    constructor(bytes memory _anchors) {
80:    function verifyRRSet(RRSetWithSignature[] memory input) external virtual view override returns(bytes memory) {
91:    function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
110:    function validateSignedSet(RRSetWithSignature memory input, bytes memory proof, uint256 now) internal view returns(RRUtils.SignedSet memory rrset) {
147:    function validateRRs(RRUtils.SignedSet memory rrset, uint16 typecovered) internal pure returns (bytes memory name) {
183:    function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view {
209:    function verifyWithKnownKey(RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, RRUtils.RRIterator memory proof) internal view {
233:    function verifySignatureWithKey(RRUtils.DNSKEY memory dnskey, bytes memory keyrdata, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data)
273:    function verifyWithDS(RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, RRUtils.RRIterator memory proof) internal view {
299:    function verifyKeyWithDS(bytes memory keyname, RRUtils.RRIterator memory dsrrs, RRUtils.DNSKEY memory dnskey, bytes memory keyrdata)
335:    function verifyDSHash(uint8 digesttype, bytes memory data, bytes memory digest) internal view returns (bool) {
dnssec-oracle/BytesUtils.sol:
11:    function keccak(bytes memory self, uint offset, uint len) internal pure returns (bytes32 ret) {
27:    function compare(bytes memory self, bytes memory other) internal pure returns (int) {
44:    function compare(bytes memory self, uint offset, uint len, bytes memory other, uint otheroffset, uint otherlen) internal pure returns (int) {
91:    function equals(bytes memory self, uint offset, bytes memory other, uint otherOffset, uint len) internal pure returns (bool) {
103:    function equals(bytes memory self, uint offset, bytes memory other, uint otherOffset) internal pure returns (bool) {
115:    function equals(bytes memory self, uint offset, bytes memory other) internal pure returns (bool) {
125:    function equals(bytes memory self, bytes memory other) internal pure returns(bool) {
135:    function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {
145:    function readUint16(bytes memory self, uint idx) internal pure returns (uint16 ret) {
158:    function readUint32(bytes memory self, uint idx) internal pure returns (uint32 ret) {
171:    function readBytes32(bytes memory self, uint idx) internal pure returns (bytes32 ret) {
184:    function readBytes20(bytes memory self, uint idx) internal pure returns (bytes20 ret) {
198:    function readBytesN(bytes memory self, uint idx, uint len) internal pure returns (bytes32 ret) {
234:    function substring(bytes memory self, uint offset, uint len) internal pure returns(bytes memory) {
261:    function base32HexDecodeWord(bytes memory self, uint off, uint len) internal pure returns(bytes32) {
312:    function find(bytes memory self, uint256 off, uint256 len, bytes1 needle) internal pure returns(uint256) {
dnssec-oracle/SHA1.sol:
6:    function sha1(bytes memory data) internal pure returns(bytes20 ret) {
dnssec-oracle/RRUtils.sol:
19:    function nameLength(bytes memory self, uint offset) internal pure returns(uint) {
38:    function readName(bytes memory self, uint offset) internal pure returns(bytes memory ret) {
49:    function labelCount(bytes memory self, uint offset) internal pure returns(uint) {
85:    function readSignedSet(bytes memory data) internal pure returns(SignedSet memory self) {
97:    function rrs(SignedSet memory rrset) internal pure returns(RRIterator memory) {
120:    function iterateRRs(bytes memory self, uint offset) internal pure returns (RRIterator memory ret) {
131:    function done(RRIterator memory iter) internal pure returns(bool) {
139:    function next(RRIterator memory iter) internal pure {
168:    function name(RRIterator memory iter) internal pure returns(bytes memory) {
177:    function rdata(RRIterator memory iter) internal pure returns(bytes memory) {
193:    function readDNSKEY(bytes memory data, uint offset, uint length) internal pure returns(DNSKEY memory self) {
212:    function readDS(bytes memory data, uint offset, uint length) internal pure returns(DS memory self) {
219:    function compareNames(bytes memory self, bytes memory other) internal pure returns (int) {
270:    function progress(bytes memory body, uint off) internal pure returns(uint) {
279:    function computeKeytag(bytes memory data) internal pure returns (uint16) {
Mitigation

Change function arguments from memory to calldata.

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size. Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

111 of these issues was found through 8 contracts.

wrapper/NameWrapper.sol:
25:error InvalidExpiry(bytes32 node, uint64 expiry);
47:    uint64 private constant MAX_EXPIRY = type(uint64).max;
189:        returns (uint32 fuses, uint64 expiry)
213:        uint32 fuses,
214:        uint64 expiry,
216:    ) public override returns (uint64) {
256:        uint32 fuses,
257:        uint64 expiry
274:        uint64 expiry
373:    function setFuses(bytes32 node, uint32 fuses)
377:        returns (uint32)
459:        uint32 fuses,
460:        uint64 expiry
466:        uint64 maxExpiry;
508:        uint32 fuses,
509:        uint64 expiry
544:        uint64 ttl,
545:        uint32 fuses,
546:        uint64 expiry
588:        uint64 ttl
624:    function setTTL(bytes32 node, uint64 ttl)
639:    modifier operationAllowed(bytes32 node, uint32 fuseMask) {
683:    function allFusesBurned(bytes32 node, uint32 fuseMask)
707:            uint32 fuses,
708:            uint64 expiry,
729:    function _canTransfer(uint32 fuses) internal pure override returns (bool) {
758:        uint32 fuses,
759:        uint64 expiry
775:        uint32 fuses,
776:        uint64 expiry
788:        uint32 fuses,
789:        uint64 expiry
797:        returns (uint32 fuses, uint64 expiry)
816:        uint32 fuses,
817:        uint64 expiry
828:        uint64 expiry
834:            uint32 fuses,
835:            uint64
838:        uint64 oldExpiry;
849:        uint64 expiry
855:            uint32 fuses,
856:            uint64
859:        uint64 oldExpiry;
868:        uint64 expiry,
869:        uint64 oldExpiry,
870:        uint64 maxExpiry
871:    ) internal pure returns (uint64) {
888:        uint32 fuses,
889:        uint64 expiry,
891:    ) private returns (uint64) {
937:        uint32 fuses,
938:        uint64 expiry
947:        uint32 fuses,
948:        uint64 expiry
954:    function _canFusesBeBurned(bytes32 node, uint32 fuses) internal pure {
wrapper/ERC1155Fuse.sol:
137:            uint32 fuses,
138:            uint64 expiry
157:        uint32 fuses,
158:        uint64 expiry
238:    function _canTransfer(uint32 fuses) internal virtual returns (bool);
243:        uint32 fuses,
244:        uint64 expiry
ethregistrar/ETHRegistrarController.sol:
94:        uint32 fuses,
95:        uint64 wrapperExpiry
133:        uint32 fuses,
134:        uint64 wrapperExpiry
registry/ENS.sol:
14:    event NewTTL(bytes32 indexed node, uint64 ttl);
27:        uint64 ttl
35:        uint64 ttl
48:    function setTTL(bytes32 node, uint64 ttl) external;
56:    function ttl(bytes32 node) external view returns (uint64);
dnssec-oracle/DNSSECImpl.sol:
21:    uint16 constant DNSCLASS_IN = 1;
23:    uint16 constant DNSTYPE_DS = 43;
24:    uint16 constant DNSTYPE_DNSKEY = 48;
29:    error SignatureNotValidYet(uint32 inception, uint32 now);
30:    error SignatureExpired(uint32 expiration, uint32 now);
31:    error InvalidClass(uint16 class);
33:    error SignatureTypeMismatch(uint16 rrsetType, uint16 sigType);
35:    error InvalidProofType(uint16 proofType);
39:    mapping (uint8 => Algorithm) public algorithms;
40:    mapping (uint8 => Digest) public digests;
58:    function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
69:    function setDigest(uint8 id, Digest digest) public owner_only {
147:    function validateRRs(RRUtils.SignedSet memory rrset, uint16 typecovered) internal pure returns (bytes memory name) {
251:        uint16 computedkeytag = keyrdata.computeKeytag();
302:        uint16 keytag = keyrdata.computeKeytag();
335:    function verifyDSHash(uint8 digesttype, bytes memory data, bytes memory digest) internal view returns (bool) {
dnssec-oracle/DNSSEC.sol:
14:    event AlgorithmUpdated(uint8 id, address addr);
15:    event DigestUpdated(uint8 id, address addr);
dnssec-oracle/BytesUtils.sol:
135:    function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {
145:    function readUint16(bytes memory self, uint idx) internal pure returns (uint16 ret) {
158:    function readUint32(bytes memory self, uint idx) internal pure returns (uint32 ret) {
265:        uint8 decoded;
dnssec-oracle/RRUtils.sol:
73:        uint16 typeCovered;
74:        uint8 algorithm;
75:        uint8 labels;
76:        uint32 ttl;
77:        uint32 expiration;
78:        uint32 inception;
79:        uint16 keytag;
107:        uint16 dnstype;
108:        uint16 class;
109:        uint32 ttl;
187:        uint16 flags;
188:        uint8 protocol;
189:        uint8 algorithm;
206:        uint16 keytag;
207:        uint8 algorithm;
208:        uint8 digestType;
266:    function serialNumberGte(uint32 i1, uint32 i2) internal pure returns(bool) {
279:    function computeKeytag(bytes memory data) internal pure returns (uint16) {
Mitigation

I suggest using uint256 instead of anything smaller or downcast where needed.

 

Unnecessary Default Value Initialization

Issue

When variable is not initialized, it will have its default values. For example, 0 for uint, false for bool and address(0) for address. Reference: https://docs.soliditylang.org/en/v0.8.15/control-structures.html#scoping-and-declarations

PoC

14 of these issues was found.

./wrapper/INameWrapper.sol:16:uint32 constant CAN_DO_EVERYTHING = 0; ./wrapper/ERC1155Fuse.sol:92: for (uint256 i = 0; i < accounts.length; ++i) { ./wrapper/ERC1155Fuse.sol:205: for (uint256 i = 0; i < ids.length; ++i) { ./ethregistrar/ETHRegistrarController.sol:256: for (uint256 i = 0; i < data.length; i++) { ./ethregistrar/StringUtils.sol:12: uint i = 0; ./dnssec-oracle/DNSSECImpl.sol:93: for(uint i = 0; i < input.length; i++) { ./dnssec-oracle/BytesUtils.sol:56: for (uint idx = 0; idx < shortest; idx += 32) { ./dnssec-oracle/BytesUtils.sol:264: uint ret = 0; ./dnssec-oracle/BytesUtils.sol:266: for(uint i = 0; i < len; i++) { ./dnssec-oracle/RRUtils.sol:50: uint count = 0; ./dnssec-oracle/RRUtils.sol:63: uint constant RRSIG_TYPE = 0; ./dnssec-oracle/RRUtils.sol:181: uint constant DNSKEY_FLAGS = 0; ./dnssec-oracle/RRUtils.sol:200: uint constant DS_KEY_TAG = 0; ./dnssec-oracle/RRUtils.sol:310: for(uint i = 0; i < data.length + 31; i += 32) {
Mitigation

I suggest removing default value initialization. For example,

  • uint32 constant CAN_DO_EVERYTHING;
  • for (uint256 i; i < accounts.length; ++i) {

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

7 of these issues was found.

./ethregistrar/ETHRegistrarController.sol:256: for (uint256 i = 0; i < data.length; i++) { ./ethregistrar/StringUtils.sol:14: for(len = 0; i < bytelength; len++) { ./dnssec-oracle/DNSSECImpl.sol:93: for(uint i = 0; i < input.length; i++) { ./dnssec-oracle/BytesUtils.sol:266: for(uint i = 0; i < len; i++) { ./dnssec-oracle/BytesUtils.sol:313: for(uint256 idx = off; idx < off + len; idx++) { ./dnssec-oracle/RRUtils.sol:235: counts--; ./dnssec-oracle/RRUtils.sol:241: othercounts--;
Mitigation

Change it to postfix increments/decrements. For example,

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

 

Keep the revert strings of error messages within 32 bytes

issue

since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

20 of these issues was found.

./wrapper/ERC1155Fuse.sol:60-63: require(account != address(0), "ERC1155: balance query for the zero address"); ./wrapper/ERC1155Fuse.sol:85-88: require(accounts.length == ids.length, "ERC1155: accounts and ids length mismatch"); ./wrapper/ERC1155Fuse.sol:107-110: require(msg.sender != operator, "ERC1155: setting approval status for self"); ./wrapper/ERC1155Fuse.sol:176: require(to != address(0), "ERC1155: transfer to the zero address"); ./wrapper/ERC1155Fuse.sol:177-180: require(from == msg.sender || isApprovedForAll(from, msg.sender), "ERC1155: caller is not owner nor approved"); ./wrapper/ERC1155Fuse.sol:195-198: require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch"); ./wrapper/ERC1155Fuse.sol:199: require(to != address(0), "ERC1155: transfer to the zero address"); ./wrapper/ERC1155Fuse.sol:200-203: require(from == msg.sender || isApprovedForAll(from, msg.sender), "ERC1155: transfer caller is not owner nor approved"); ./wrapper/ERC1155Fuse.sol:215-218: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer"); ./wrapper/ERC1155Fuse.sol:249: require(newOwner != address(0), "ERC1155: mint to the zero address"); ./wrapper/ERC1155Fuse.sol:250-253: require(newOwner != address(this), "ERC1155: newOwner cannot be the NameWrapper contract"); ./wrapper/ERC1155Fuse.sol:290-293: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer"); ./wrapper/Controllable.sol:17: require(controllers[msg.sender], "Controllable: Caller is not a controller"); ./ethregistrar/ETHRegistrarController.sol:99-102: require(resolver != address(0), "ETHRegistrarController: resolver is required when data is supplied"); ./ethregistrar/ETHRegistrarController.sol:137-140: require(msg.value >= (price.base + price.premium), "ETHRegistrarController: Not enough ether provided"); ./ethregistrar/ETHRegistrarController.sol:196-199: require(msg.value >= price.base, "ETHController: Not enough Ether provided for renewal"); ./ethregistrar/ETHRegistrarController.sol:232-235: require(commitments[commitment] + minCommitmentAge <= block.timestamp, "ETHRegistrarController: Commitment is not valid"); ./ethregistrar/ETHRegistrarController.sol:238-241: require(commitments[commitment] + maxCommitmentAge > block.timestamp, "ETHRegistrarController: Commitment has expired"); ./ethregistrar/ETHRegistrarController.sol:242: require(available(name), "ETHRegistrarController: Name is unavailable"); ./ethregistrar/ETHRegistrarController.sol:259-262: require(txNamehash == nodehash, "ETHRegistrarController: Namehash on record do not match the name being registered"); ./registry/ReverseRegistrar.sol:41-47: 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"); ./registry/ReverseRegistrar.sol:52-55: require(address(resolver) != address(0), "ReverseRegistrar: Resolver address must not be 0");
Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

26 of these issues was found.

./wrapper/ERC1155Fuse.sol:60-63: require(account != address(0), "ERC1155: balance query for the zero address"); ./wrapper/ERC1155Fuse.sol:85-88: require(accounts.length == ids.length, "ERC1155: accounts and ids length mismatch"); ./wrapper/ERC1155Fuse.sol:107-110: require(msg.sender != operator, "ERC1155: setting approval status for self"); ./wrapper/ERC1155Fuse.sol:176: require(to != address(0), "ERC1155: transfer to the zero address"); ./wrapper/ERC1155Fuse.sol:177-180: require(from == msg.sender || isApprovedForAll(from, msg.sender), "ERC1155: caller is not owner nor approved"); ./wrapper/ERC1155Fuse.sol:195-198: require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch"); ./wrapper/ERC1155Fuse.sol:199: require(to != address(0), "ERC1155: transfer to the zero address"); ./wrapper/ERC1155Fuse.sol:200-203: require(from == msg.sender || isApprovedForAll(from, msg.sender), "ERC1155: transfer caller is not owner nor approved"); ./wrapper/ERC1155Fuse.sol:215-218: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer"); ./wrapper/ERC1155Fuse.sol:248: require(owner == address(0), "ERC1155: mint of existing token"); ./wrapper/ERC1155Fuse.sol:249: require(newOwner != address(0), "ERC1155: mint to the zero address"); ./wrapper/ERC1155Fuse.sol:250-253: require(newOwner != address(this), "ERC1155: newOwner cannot be the NameWrapper contract"); ./wrapper/ERC1155Fuse.sol:290-293: require(amount == 1 && oldOwner == from, "ERC1155: insufficient balance for transfer"); ./wrapper/BytesUtil.sol:28: require(offset == self.length - 1, "namehash: Junk at end of name"); ./wrapper/BytesUtil.sol:42: require(idx < self.length, "readLabel: Index out of bounds"); ./wrapper/Controllable.sol:17: require(controllers[msg.sender], "Controllable: Caller is not a controller"); ./ethregistrar/ETHRegistrarController.sol:99-102: require(resolver != address(0), "ETHRegistrarController: resolver is required when data is supplied"); ./ethregistrar/ETHRegistrarController.sol:137-140: require(msg.value >= (price.base + price.premium), "ETHRegistrarController: Not enough ether provided"); ./ethregistrar/ETHRegistrarController.sol:196-199: require(msg.value >= price.base, "ETHController: Not enough Ether provided for renewal"); ./ethregistrar/ETHRegistrarController.sol:232-235: require(commitments[commitment] + minCommitmentAge <= block.timestamp, "ETHRegistrarController: Commitment is not valid"); ./ethregistrar/ETHRegistrarController.sol:238-241: require(commitments[commitment] + maxCommitmentAge > block.timestamp, "ETHRegistrarController: Commitment has expired"); ./ethregistrar/ETHRegistrarController.sol:242: require(available(name), "ETHRegistrarController: Name is unavailable"); ./ethregistrar/ETHRegistrarController.sol:259-262: require(txNamehash == nodehash, "ETHRegistrarController: Namehash on record do not match the name being registered"); ./registry/ReverseRegistrar.sol:41-47: 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"); ./registry/ReverseRegistrar.sol:52-55: require(address(resolver) != address(0), "ReverseRegistrar: Resolver address must not be 0"); ./dnssec-oracle/RRUtils.sol:307: require(data.length <= 8192, "Long keys not permitted");
Mitigation

I suggest implementing custom errors to save gas.

 

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter