ENS contest - Dravee's results

Lines of code

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

Vulnerability details

This is a classic Code4rena issue:

• https://github.com/code-423n4/2021-04-meebits-findings/issues/2
• https://github.com/code-423n4/2021-10-tally-findings/issues/20
• https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

Impact

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Impacted lines:

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);

Recommended Mitigation

Use call() instead of transfer(), but be sure to respect the CEI pattern and/or add re-entrancy guards, as several hacks already happened in the past due to this recommendation not being fully understood.

Relevant links: https://twitter.com/hacxyk/status/1520715516490379264?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/hacxyk/status/1520715536325218304?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/hacxyk/status/1520370441705037824?s=21&t=fnhDkcC3KpE_kJE8eLiE2A https://twitter.com/Hacxyk/status/1521949933380595712

Overview

Table of Contents

• 1. Low Risk Issues
  • 1.1. Critical known vulnerabilities exist in currently used @openzeppelin/contracts version
  • 1.2. pragma experimental ABIEncoderV2 is deprecated
  • 1.3. Unsafe casting may overflow
  • 1.4. Missing address(0) checks
  • 1.5. _safeMint() should be used rather than _mint() wherever possible
  • 1.6. Use a 2-step ownership transfer pattern
  • 1.7. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking
• 2. Non-Critical Issues
  • 2.1. Avoid using deprecated keywords (now) as variable names
  • 2.2. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()
  • 2.3. It's better to emit after all processing is done
  • 2.4. Typos
  • 2.5. Use a constant instead of duplicating the same string
  • 2.6. Open TODOS
  • 2.7. Use string.concat() or bytes.concat()
  • 2.8. Adding a return statement when the function defines a named return variable, is redundant
  • 2.9. Non-library/interface files should use fixed compiler versions, not floating ones

1. Low Risk Issues

1.1. Critical known vulnerabilities exist in currently used @openzeppelin/contracts version

As several known critical vulnerabilities exist in the current @openzeppelin/contracts version, consider updating package.json with at least @openzeppelin/contracts@4.4.1 here:

• package.json

File: package.json
53:     "@openzeppelin/contracts": "^4.1.0",

While vulnerabilities are known, the current scope isn't affected (this might not hold true for the whole solution or a future update)

1.2. pragma experimental ABIEncoderV2 is deprecated

Use pragma abicoder v2 instead

dnssec-oracle/DNSSEC.sol:3:pragma experimental ABIEncoderV2;
dnssec-oracle/DNSSECImpl.sol:3:pragma experimental ABIEncoderV2;

1.3. Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting. Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

dnssec-oracle/BytesUtils.sol:269:            decoded = uint8(base32HexTable[uint(uint8(char)) - 0x30]);
dnssec-oracle/DNSSECImpl.sol:126:        if(!RRUtils.serialNumberGte(rrset.expiration, uint32(now))) {
dnssec-oracle/DNSSECImpl.sol:127:            revert SignatureExpired(rrset.expiration, uint32(now));
dnssec-oracle/DNSSECImpl.sol:132:        if(!RRUtils.serialNumberGte(uint32(now), rrset.inception)) {
dnssec-oracle/DNSSECImpl.sol:133:            revert SignatureNotValidYet(rrset.inception, uint32(now));
dnssec-oracle/RRUtils.sol:267:        return int32(i1) - int32(i2) >= 0;
dnssec-oracle/RRUtils.sol:334:            return uint16(ac1);
wrapper/BytesUtil.sol:43:        uint len = uint(uint8(self[idx]));
wrapper/ERC1155Fuse.sol:143:        expiry = uint64(t >> 192);
wrapper/ERC1155Fuse.sol:147:            fuses = uint32(t >> 160);
wrapper/NameWrapper.sol:63:            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
wrapper/NameWrapper.sol:69:            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
wrapper/NameWrapper.sol:282:        expiry = _normaliseExpiry(expiry, oldExpiry, uint64(expires));
wrapper/NameWrapper.sol:472:            maxExpiry = uint64(registrar.nameExpires(uint256(labelhash)));
wrapper/NameWrapper.sol:752:        return abi.encodePacked(uint8(bytes(label).length), label, name);
wrapper/NameWrapper.sol:861:        uint64 maxExpiry = uint64(registrar.nameExpires(uint256(labelhash)));

1.4. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

• File: ETHRegistrarController.sol

25:     BaseRegistrarImplementation immutable base;
26:     IPriceOracle public immutable prices;
27:     uint256 public immutable minCommitmentAge;
28:     uint256 public immutable maxCommitmentAge;
29:     ReverseRegistrar public immutable reverseRegistrar;
30:     INameWrapper public immutable nameWrapper;
...
49:     constructor(
50:         BaseRegistrarImplementation _base,
51:         IPriceOracle _prices,
52:         uint256 _minCommitmentAge,
53:         uint256 _maxCommitmentAge,
54:         ReverseRegistrar _reverseRegistrar,
55:         INameWrapper _nameWrapper
56:     ) {
57:         require(_maxCommitmentAge > _minCommitmentAge);
+ 58:         require(_base != address(0));
+ 58:         require(_prices != address(0));
+ 58:         require(_reverseRegistrar != address(0));
+ 58:         require(_nameWrapper != address(0));
58: 
59:         base = _base;
60:         prices = _prices;
61:         minCommitmentAge = _minCommitmentAge;
62:         maxCommitmentAge = _maxCommitmentAge;
63:         reverseRegistrar = _reverseRegistrar;
64:         nameWrapper = _nameWrapper;
65:     }

• File: NameWrapper.sol

35:     ENS public immutable override ens;
36:     IBaseRegistrar public immutable override registrar;
...
49:     constructor(
50:         ENS _ens,
51:         IBaseRegistrar _registrar,
52:         IMetadataService _metadataService
53:     ) {
+ 54:         require(_ens != address(0));
+ 54:         require(_registrar != address(0));
54:         ens = _ens;
55:         registrar = _registrar;

1.5. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements ERC1155TokenReceiver's onERC1155Received.

File: NameWrapper.sol
771:     function _wrap(
772:         bytes32 node,
773:         bytes memory name,
774:         address wrappedOwner,
775:         uint32 fuses,
776:         uint64 expiry
777:     ) internal {
778:         names[node] = name;
779:         _mint(node, wrappedOwner, fuses, expiry);
780:         emit NameWrapped(node, name, wrappedOwner, fuses, expiry);
781:     }

Be careful however to respect the CEI pattern or add a re-entrancy guard as _safeMint adds a callback-check and a malicious onERC1155Received could be exploited if not careful (the CEIP is respected here).

Reading material:

• https://blocksecteam.medium.com/when-safemint-becomes-unsafe-lessons-from-the-hypebears-security-incident-2965209bda2a
• https://samczsun.com/the-dangers-of-surprising-code/
• https://github.com/KadenZipfel/smart-contract-attack-vectors/blob/master/vulnerabilities/unprotected-callback.md

1.6. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

• ETHRegistrarController.sol

ethregistrar/ETHRegistrarController.sol:9:import "@openzeppelin/contracts/access/Ownable.sol";
ethregistrar/ETHRegistrarController.sol:17:contract ETHRegistrarController is Ownable, IETHRegistrarController {

• ReverseRegistrar.sol

registry/ReverseRegistrar.sol:5:import "@openzeppelin/contracts/access/Ownable.sol";
registry/ReverseRegistrar.sol:18:contract ReverseRegistrar is Ownable, Controllable, IReverseRegistrar {

• Controllable.sol

wrapper/Controllable.sol:4:import "@openzeppelin/contracts/access/Ownable.sol";
wrapper/Controllable.sol:6:contract Controllable is Ownable {

• NameWrapper.sol

wrapper/NameWrapper.sol:12:import "@openzeppelin/contracts/access/Ownable.sol";
wrapper/NameWrapper.sol:28:    Ownable,

1.7. require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking

Properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix. Here, I believe the assert should be a require or a revert:

dnssec-oracle/RRUtils.sol:22:            assert(idx < self.length);
dnssec-oracle/RRUtils.sol:52:            assert(offset < self.length);

As the Solidity version is > 0.8.* the remaining gas would still be refunded in case of failure.

2. Non-Critical Issues

2.1. Avoid using deprecated keywords (now) as variable names

now is a deprecated keyword that was used before block.timestamp. Here, a variable is named as "now", which introduces some misunderstanding from the IDE:

contracts/dnssec-oracle/DNSSEC.sol:
  18:     function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public view virtual returns(bytes memory);

contracts/dnssec-oracle/DNSSECImpl.sol:
   29:     error SignatureNotValidYet(uint32 inception, uint32 now);
   30:     error SignatureExpired(uint32 expiration, uint32 now);
   91:     function verifyRRSet(RRSetWithSignature[] memory input, uint256 now) public virtual view override returns(bytes memory) {
   94:             RRUtils.SignedSet memory rrset = validateSignedSet(input[i], proof, now);
  110:     function validateSignedSet(RRSetWithSignature memory input, bytes memory proof, uint256 now) internal view returns(RRUtils.SignedSet memory rrset) {
  126:         if(!RRUtils.serialNumberGte(rrset.expiration, uint32(now))) {
  127:             revert SignatureExpired(rrset.expiration, uint32(now));
  132:         if(!RRUtils.serialNumberGte(uint32(now), rrset.inception)) {
  133:             revert SignatureNotValidYet(rrset.inception, uint32(now));

Consider changing the variable's name.

2.2. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here Original issue: Hash collisions when using abi.encodePacked() with multiple variable length arguments

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

ethregistrar/ETHRegistrarController.sol:255:        bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label));
registry/ReverseRegistrar.sol:83:            abi.encodePacked(ADDR_REVERSE_NODE, labelHash)
registry/ReverseRegistrar.sol:151:                abi.encodePacked(ADDR_REVERSE_NODE, sha3HexAddress(addr))
wrapper/BytesUtil.sol:31:        return keccak256(abi.encodePacked(namehash(self, newOffset), labelhash));
wrapper/NameWrapper.sol:738:        return keccak256(abi.encodePacked(node, labelhash));
wrapper/NameWrapper.sol:752:        return abi.encodePacked(uint8(bytes(label).length), label, name);

Here, no attack vector can be thought of. It's a simple mispractice, hence the NC severity.

2.3. It's better to emit after all processing is done

• contracts/ethregistrar/ETHRegistrarController.sol:

  173:         emit NameRegistered(
  174              name,
  175              keccak256(bytes(name)),
  176              owner,
  177              price.base,
  178              price.premium,
  179              expires
  180          );
  181  
  182          if (msg.value > (price.base + price.premium)) {
  183              payable(msg.sender).transfer(

• contracts/registry/ReverseRegistrar.sol:

  85:         emit ReverseClaimed(addr, reverseNode);
  86          ens.setSubnodeRecord(ADDR_REVERSE_NODE, labelHash, owner, resolver, 0);

• contracts/wrapper/ERC1155Fuse.sol:

  222:         emit TransferBatch(msg.sender, from, to, ids, amounts);
  223  
  224          _doSafeBatchTransferAcceptanceCheck(
  225              msg.sender,
  226              from,
  227              to,
  228              ids,
  229              amounts,
  230              data
  231          );

  256:         emit TransferSingle(msg.sender, address(0x0), newOwner, tokenId, 1);
  257          _doSafeTransferAcceptanceCheck(
  258              msg.sender,
  259              address(0),
  260              newOwner,
  261              tokenId,
  262              1,
  263              ""
  264          );

  296:         emit TransferSingle(msg.sender, from, to, id, amount);
  297  
  298          _doSafeTransferAcceptanceCheck(msg.sender, from, to, id, amount, data);
  299      }

• contracts/wrapper/NameWrapper.sol:

  766:             emit NameUnwrapped(node, address(0));
  767          }
  768          super._mint(node, wrappedOwner, fuses, expiry);
  769      }

2.4. Typos

• canonicalised vs canonicalized

dnssec-oracle/DNSSECImpl.sol:105:     *        data, followed by a series of canonicalised RR records that the signature

• optimised vs optimized

registry/ReverseRegistrar.sol:156:     * @dev An optimised function to compute the sha3 of the lower-case

• Authorises vs Authorizes

ethregistrar/IBaseRegistrar.sol:20:    // Authorises a controller, who can register and renew domains.

• authorised vs authorized

registry/ReverseRegistrar.sol:46:            "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
registry/ReverseRegistrar.sol:126:     * Only callable by controllers and authorised users
wrapper/NameWrapper.sol:202:     * @dev Can be called by the owner of the name on the .eth registrar or an authorised caller on the registrar
wrapper/NameWrapper.sol:241:     *      Only callable by authorised controllers.
wrapper/NameWrapper.sol:266:     *      Only callable by authorised controllers.

2.5. Use a constant instead of duplicating the same string

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:322:                    revert("ERC1155: ERC1155Receiver rejected tokens");
wrapper/ERC1155Fuse.sol:354:                    revert("ERC1155: ERC1155Receiver rejected tokens");

wrapper/ERC1155Fuse.sol:327:                revert("ERC1155: transfer to non ERC1155Receiver implementer");
wrapper/ERC1155Fuse.sol:359:                revert("ERC1155: transfer to non ERC1155Receiver implementer");

2.6. Open TODOS

Consider resolving the TODOs before deploying.

dnssec-oracle/DNSSECImpl.sol:238:        // TODO: Check key isn't expired, unless updating key itself

2.7. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

• ETHRegistrarController.sol

ethregistrar/ETHRegistrarController.sol:1:pragma solidity >=0.8.4;
ethregistrar/ETHRegistrarController.sol:255:        bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label));

• ReverseRegistrar.sol

registry/ReverseRegistrar.sol:1:pragma solidity >=0.8.4;
registry/ReverseRegistrar.sol:83:            abi.encodePacked(ADDR_REVERSE_NODE, labelHash)
registry/ReverseRegistrar.sol:151:                abi.encodePacked(ADDR_REVERSE_NODE, sha3HexAddress(addr))

• BytesUtil.sol

wrapper/BytesUtil.sol:2:pragma solidity >=0.8.4;
wrapper/BytesUtil.sol:31:        return keccak256(abi.encodePacked(namehash(self, newOffset), labelhash));

• NameWrapper.sol

wrapper/NameWrapper.sol:2:pragma solidity ^0.8.4;
wrapper/NameWrapper.sol:738:        return keccak256(abi.encodePacked(node, labelhash));
wrapper/NameWrapper.sol:752:        return abi.encodePacked(uint8(bytes(label).length), label, name);

2.8. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

• BytesUtils.sol#readUint8()

135:     function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {
136:         return uint8(self[idx]);
137:     }

• ReverseRegistrar.sol#ownsContract()

181:     function ownsContract(address addr) internal view returns (bool) {
182:         try Ownable(addr).owner() returns (address owner) {
183:             return owner == msg.sender;
184:         } catch {
185:             return false;
186:         }
187:     }

• NameWrapper.sol#ownerOf()

90:     function ownerOf(uint256 id)
91:         public
92:         view
93:         override(ERC1155Fuse, INameWrapper)
94:         returns (address owner)
95:     {
96:         return super.ownerOf(id);
97:     }

• NameWrapper.sol#_addLabel()

741:     function _addLabel(string memory label, bytes memory name)
742:         internal
743:         pure
744:         returns (bytes memory ret)
745:     {
...
752:         return abi.encodePacked(uint8(bytes(label).length), label, name);
753:     }

2.9. Non-library/interface files should use fixed compiler versions, not floating ones

dnssec-oracle/algorithms/Algorithm.sol:1:pragma solidity ^0.8.4;
dnssec-oracle/digests/Digest.sol:1:pragma solidity ^0.8.4;
dnssec-oracle/BytesUtils.sol:1:pragma solidity ^0.8.4;
dnssec-oracle/DNSSEC.sol:2: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/RRUtils.sol:1:pragma solidity ^0.8.4;
dnssec-oracle/SHA1.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;
registry/ENS.sol:1:pragma solidity >=0.8.4;
registry/ReverseRegistrar.sol:1:pragma solidity >=0.8.4;
resolvers/Resolver.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;
wrapper/ERC1155Fuse.sol:2:pragma solidity ^0.8.4;
wrapper/NameWrapper.sol:2:pragma solidity ^0.8.4;

Overview

Table of Contents:

• 1. Caching storage values in memory
• 2. Unchecking arithmetics operations that can't underflow/overflow
• 3. Reduce the size of error messages (Long revert Strings)
• 4. Splitting require() statements that use && saves gas
• 5. Using private rather than public for constants saves gas
• 6. Use shift right/left instead of division/multiplication if possible
• 7. <array>.length should not be looked up in every loop of a for-loop
• 8. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)
• 9. Increments/decrements can be unchecked in for-loops
• 10. It costs more gas to initialize variables with their default value than letting the default value be applied
• 11. Use Custom Errors instead of Revert Strings to save Gas
• 12. Functions guaranteed to revert when called by normal users can be marked payable

1. Caching storage values in memory

The code can be optimized by minimizing the number of SLOADs.

SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.

• Saving around 500 gas: NameWrapper.sol#L128-L143

128:     function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
129:         public
130:         onlyOwner
131:     {
+ 132:         address _upgradeContractAddress = address(upgradeContract);
- 132:         if (address(upgradeContract) != address(0)) { //@audit gas SLOAD 1 (upgradeContract)
+ 132:         if (_upgradeContractAddress != address(0)) {
- 133:             registrar.setApprovalForAll(address(upgradeContract), false); //@audit gas SLOAD 2 (upgradeContract)
+ 133:             registrar.setApprovalForAll(_upgradeContractAddress, false);
- 134:             ens.setApprovalForAll(address(upgradeContract), false); //@audit gas SLOAD 3 (upgradeContract)
+ 134:             ens.setApprovalForAll(_upgradeContractAddress, false);
135:         }
136: 
137:         upgradeContract = _upgradeAddress;
138: 
- 139:         if (address(upgradeContract) != address(0)) { //@audit gas SLOAD 4 (upgradeContract)
+ 139:         if (address(_upgradeAddress) != address(0)) {
- 140:             registrar.setApprovalForAll(address(upgradeContract), true); //@audit gas SLOAD 5 (upgradeContract)
+ 140:             registrar.setApprovalForAll(address(_upgradeAddress), true);
- 141:             ens.setApprovalForAll(address(upgradeContract), true); //@audit gas SLOAD 6 (upgradeContract)
+ 141:             ens.setApprovalForAll(address(_upgradeAddress), true);
142:         }
143:     }

• Saving around 100 gas: DNSSECImpl.sol#L335-L340

+ 336:         Digest _digest = digests[digesttype];
- 336:         if (address(digests[digesttype]) == address(0)) { //@audit SLOAD 1
+ 336:         if (address(_digest) == address(0)) {
337:             return false;
338:         }
- 339:         return digests[digesttype].verify(data, digest); //@audit SLOAD 2
+ 339:         return _digest.verify(data, digest);

• Saving around 100 gas: ETHRegistrarController.sol#L232-L241

+ 232:         uint256 _commitment = commitments[commitment];
232:         require(
- 233:             commitments[commitment] + minCommitmentAge <= block.timestamp, //@audit gas: SLOAD 1 (commitments[commitment])
+ 233:             _commitment + minCommitmentAge <= block.timestamp,
234:             "ETHRegistrarController: Commitment is not valid"
235:         );
236: 
237:         // If the commitment is too old, or the name is registered, stop
238:         require(
- 239:             commitments[commitment] + maxCommitmentAge > block.timestamp, //@audit gas: SLOAD 2 (commitments[commitment)
+ 239:             _commitment + maxCommitmentAge > block.timestamp,
240:             "ETHRegistrarController: Commitment has expired"
241:         );

2. Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

• ETHRegistrarController.sol#L184

182:         if (msg.value > (price.base + price.premium)) {
- 183:             payable(msg.sender).transfer(
- 184:                 msg.value - (price.base + price.premium)  //@audit should be unchecked due to L182
- 185:             );
+ 183:             unchecked { payable(msg.sender).transfer(
+ 184:                 msg.value - (price.base + price.premium)
+ 185:             ) };
186:         }

• ETHRegistrarController.sol#L204

203:         if (msg.value > price.base) {
- 204:             payable(msg.sender).transfer(msg.value - price.base); //@audit should be unchecked due to L203
+ 204:             unchecked { payable(msg.sender).transfer(msg.value - price.base); }
205:         }

• RRUtils.sol#L29

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;//@audit should be unchecked due to L20 + idx only getting increased
+ 29:         unchecked { return idx - offset; }

• RRUtils.sol#L250

File: RRUtils.sol
245:         while (counts > 0 && !self.equals(off, other, otheroff)) {
...
- 250:             counts -= 1; //@audit should be unchecked due to L245
+ 250:             unchecked { --counts; } //@audit "--counts" costs 16 gas less than "counts -= 1"
251:         }

3. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

ethregistrar/ETHRegistrarController.sol:101:                "ETHRegistrarController: resolver is required when data is supplied"
ethregistrar/ETHRegistrarController.sol:139:            "ETHRegistrarController: Not enough ether provided"
ethregistrar/ETHRegistrarController.sol:198:            "ETHController: Not enough Ether provided for renewal"
ethregistrar/ETHRegistrarController.sol:234:            "ETHRegistrarController: Commitment is not valid"
ethregistrar/ETHRegistrarController.sol:240:            "ETHRegistrarController: Commitment has expired"
ethregistrar/ETHRegistrarController.sol:242:        require(available(name), "ETHRegistrarController: Name is unavailable");
ethregistrar/ETHRegistrarController.sol:261:                "ETHRegistrarController: Namehash on record do not match the name being registered"
ethregistrar/ETHRegistrarController.sol:265:                "ETHRegistrarController: Failed to set Record"
registry/ReverseRegistrar.sol:46:            "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
registry/ReverseRegistrar.sol:54:            "ReverseRegistrar: Resolver address must not be 0"
wrapper/Controllable.sol:17:        require(controllers[msg.sender], "Controllable: Caller is not a controller");
wrapper/ERC1155Fuse.sol:62:            "ERC1155: balance query for the zero address"
wrapper/ERC1155Fuse.sol:87:            "ERC1155: accounts and ids length mismatch"
wrapper/ERC1155Fuse.sol:109:            "ERC1155: setting approval status for self"
wrapper/ERC1155Fuse.sol:176:        require(to != address(0), "ERC1155: transfer to the zero address");
wrapper/ERC1155Fuse.sol:179:            "ERC1155: caller is not owner nor approved"
wrapper/ERC1155Fuse.sol:197:            "ERC1155: ids and amounts length mismatch"
wrapper/ERC1155Fuse.sol:199:        require(to != address(0), "ERC1155: transfer to the zero address");
wrapper/ERC1155Fuse.sol:202:            "ERC1155: transfer caller is not owner nor approved"
wrapper/ERC1155Fuse.sol:217:                "ERC1155: insufficient balance for transfer"
wrapper/ERC1155Fuse.sol:249:        require(newOwner != address(0), "ERC1155: mint to the zero address");
wrapper/ERC1155Fuse.sol:252:            "ERC1155: newOwner cannot be the NameWrapper contract"
wrapper/ERC1155Fuse.sol:292:            "ERC1155: insufficient balance for transfer"
wrapper/ERC1155Fuse.sol:322:                    revert("ERC1155: ERC1155Receiver rejected tokens");
wrapper/ERC1155Fuse.sol:327:                revert("ERC1155: transfer to non ERC1155Receiver implementer");
wrapper/ERC1155Fuse.sol:354:                    revert("ERC1155: ERC1155Receiver rejected tokens");
wrapper/ERC1155Fuse.sol:359:                revert("ERC1155: transfer to non ERC1155Receiver implementer");

Consider shortening the revert strings to fit in 32 bytes.

4. Splitting require() statements that use && saves gas

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper

Affected code (saving around 3 gas per instance):

dnssec-oracle/BytesUtils.sol:268:            require(char >= 0x30 && char <= 0x7A);
wrapper/ERC1155Fuse.sol:216:                amount == 1 && oldOwner == from,
wrapper/ERC1155Fuse.sol:291:            amount == 1 && oldOwner == from,

5. Using private rather than public for constants saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

ethregistrar/ETHRegistrarController.sol:21:    uint256 public constant MIN_REGISTRATION_DURATION = 28 days; 

6. Use shift right/left instead of division/multiplication if possible

While the DIV / MUL opcode uses 5 gas, the SHR / SHL opcode only uses 3 gas. Furthermore, beware that Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting. Eventually, overflow checks are never performed for shift operations as they are done for arithmetic operations. Instead, the result is always truncated, so the calculation can be unchecked in Solidity version 0.8+

• Use >> 1 instead of / 2
• Use >> 2 instead of / 4
• Use << 3 instead of * 8

Affected code (saves around 2 gas + 20 for unchecked per instance):

dnssec-oracle/RRUtils.sol:316:                    uint unused = 256 - (data.length - i) * 8;

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

Reading array length at each iteration of the loop consumes more gas than necessary.

In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.

Here, consider storing the array's length in a variable before the for-loop, and use this new variable instead:

dnssec-oracle/DNSSECImpl.sol:93:        for(uint i = 0; i < input.length; i++) {
dnssec-oracle/RRUtils.sol:310:            for(uint i = 0; i < data.length + 31; i += 32) {
ethregistrar/ETHRegistrarController.sol:256:        for (uint256 i = 0; i < data.length; i++) {
wrapper/ERC1155Fuse.sol:92:        for (uint256 i = 0; i < accounts.length; ++i) {
wrapper/ERC1155Fuse.sol:205:        for (uint256 i = 0; i < ids.length; ++i) {

8. ++i costs less gas compared to i++ or i += 1 (same for --i vs i-- or i -= 1)

Pre-increments and pre-decrements are cheaper.

For a uint256 i variable, the following is true with the Optimizer enabled at 10k:

Increment:

• i += 1 is the most expensive form
• i++ costs 6 gas less than i += 1
• ++i costs 5 gas less than i++ (11 gas less than i += 1)

Decrement:

• i -= 1 is the most expensive form
• i-- costs 11 gas less than i -= 1
• --i costs 5 gas less than i-- (16 gas less than i -= 1)

Note that post-increments (or post-decrements) return the old value before incrementing or decrementing, hence the name post-increment:

uint i = 1;  
uint j = 2;
require(j == i++, "This will be false as i is incremented after the comparison");

However, pre-increments (or pre-decrements) return the new value:

uint i = 1;  
uint j = 2;
require(j == ++i, "This will be true as i is incremented before the comparison");

In the pre-increment case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2.

Affected code:

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

Consider using pre-increments and pre-decrements where they are relevant (meaning: not where post-increments/decrements logic are relevant).

9. Increments/decrements can be unchecked in for-loops

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Consider wrapping with an unchecked block here (around 25 gas saved per instance):

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/DNSSECImpl.sol:93:        for(uint i = 0; i < input.length; i++) {
dnssec-oracle/RRUtils.sol:286:         *         for (uint i = 0; i < data.length; i++) {
ethregistrar/ETHRegistrarController.sol:256:        for (uint256 i = 0; i < data.length; i++) {
ethregistrar/StringUtils.sol:14:        for(len = 0; i < bytelength; len++) {
wrapper/ERC1155Fuse.sol:92:        for (uint256 i = 0; i < accounts.length; ++i) {
wrapper/ERC1155Fuse.sol:205:        for (uint256 i = 0; i < ids.length; ++i) {

The change would be:

- for (uint256 i; i < numIterations; i++) {
+ for (uint256 i; i < numIterations;) {
 // ...  
+   unchecked { ++i; }
}  

The same can be applied with decrements (which should use break when i == 0).

The risk of overflow is non-existant for uint256 here.

10. It costs more gas to initialize variables with their default value than letting the default value be applied

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas (around 3 gas per instance).

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Affected code:

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/DNSSECImpl.sol:93:        for(uint i = 0; i < input.length; i++) {
dnssec-oracle/RRUtils.sol:50:        uint count = 0;
dnssec-oracle/RRUtils.sol:310:            for(uint i = 0; i < data.length + 31; i += 32) {
ethregistrar/ETHRegistrarController.sol:256:        for (uint256 i = 0; i < data.length; i++) {
ethregistrar/StringUtils.sol:12:        uint i = 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) {

Consider removing explicit initializations for default values.

11. Use Custom Errors instead of Revert Strings to save Gas

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Consider replacing all revert strings with custom errors in the solution, and particularly those that have multiple occurrences:

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:322:                    revert("ERC1155: ERC1155Receiver rejected tokens");
wrapper/ERC1155Fuse.sol:354:                    revert("ERC1155: ERC1155Receiver rejected tokens");

wrapper/ERC1155Fuse.sol:327:                revert("ERC1155: transfer to non ERC1155Receiver implementer");
wrapper/ERC1155Fuse.sol:359:                revert("ERC1155: transfer to non ERC1155Receiver implementer");

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

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.

dnssec-oracle/DNSSECImpl.sol:58:    function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
dnssec-oracle/DNSSECImpl.sol:69:    function setDigest(uint8 id, Digest digest) public owner_only {
dnssec-oracle/Owned.sol:18:    function setOwner(address newOwner) public owner_only {
registry/ReverseRegistrar.sol:51:    function setDefaultResolver(address resolver) public override onlyOwner {
wrapper/Controllable.sol:11:    function setController(address controller, bool active) onlyOwner() public {