ENS contest - Sm4rty'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 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L223 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L341-L345

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances:

contracts/ethregistrar/ETHRegistrarController.sol:183: contracts/ethregistrar/ETHRegistrarController.sol:204: contracts/ethregistrar/ETHRegistrarController.sol:211: contracts/wrapper/NameWrapper.sol:231: contracts/wrapper/NameWrapper.sol:341:

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

contracts/wrapper/NameWrapper.sol:231:        registrar.transferFrom(registrant, address(this), tokenId);
contracts/wrapper/NameWrapper.sol:341:        registrar.transferFrom(

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

1. Remove commented function or code of no use.

There is 1 instance of this issue: File: RRUtils.sol

 /* This function probably deserves some explanation.
         * The DNSSEC keytag function is a checksum that relies on summing up individual bytes
         * from the input string, with some mild bitshifting. Here's a Naive solidity implementation:
         *
         *     function computeKeytag(bytes memory data) internal pure returns (uint16) {
         *         uint ac;
         *         for (uint i = 0; i < data.length; i++) {
         *             ac += i & 1 == 0 ? uint16(data.readUint8(i)) << 8 : data.readUint8(i);
         *         }
         *         return uint16(ac + (ac >> 16));
         *     }
         *

References:

https://code4rena.com/reports/2022-05-sturdy#n-12-remove-commented-out-code

---

2. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances

contracts/dnssec-oracle/DNSSECImpl.sol:238:

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

---

3. Avoid use of floating pragma

Impact

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

contracts/dnssec-oracle/BytesUtils.sol contracts/dnssec-oracle/Owned.sol contracts/dnssec-oracle/DNSSEC.sol contracts/dnssec-oracle/algorithms/Algorithm.sol contracts/dnssec-oracle/DNSSECImpl.sol contracts/dnssec-oracle/RRUtils.sol contracts/dnssec-oracle/digests/Digest.sol contracts/wrapper/Controllable.sol contracts/wrapper/INameWrapper.sol contracts/wrapper/NameWrapper.sol contracts/wrapper/ERC1155Fuse.sol

Instances:

contracts/registry/IReverseRegistrar.sol:1	pragma solidity >=0.8.4;
contracts/registry/ENS.sol:1			pragma solidity >=0.8.4;
contracts/registry/ReverseRegistrar.sol:1	pragma solidity >=0.8.4;
contracts/resolvers/Resolver.sol:2		pragma solidity >=0.8.4;
contracts/ethregistrar/ETHRegistrarController.sol:1	pragma solidity >=0.8.4;
contracts/ethregistrar/StringUtils.sol:1	pragma solidity >=0.8.4;
contracts/ethregistrar/IETHRegistrarController.sol:1	pragma solidity >=0.8.4;
contracts/dnssec-oracle/BytesUtils.sol:1	pragma solidity ^0.8.4;
contracts/dnssec-oracle/SHA1.sol:1		pragma solidity >=0.8.4;
contracts/dnssec-oracle/Owned.sol:1		pragma solidity ^0.8.4;
contracts/dnssec-oracle/DNSSEC.sol:2		pragma solidity ^0.8.4;
contracts/dnssec-oracle/algorithms/Algorithm.sol:1	pragma solidity ^0.8.4;
contracts/dnssec-oracle/DNSSECImpl.sol:2	pragma solidity ^0.8.4;
contracts/dnssec-oracle/RRUtils.sol:1		pragma solidity ^0.8.4;
contracts/dnssec-oracle/digests/Digest.sol:1	pragma solidity ^0.8.4;
contracts/wrapper/Controllable.sol:2		pragma solidity ^0.8.4;
contracts/wrapper/INameWrapper.sol:2		pragma solidity ^0.8.4;
contracts/wrapper/IMetadataService.sol:1	pragma solidity >=0.8.4;
contracts/wrapper/NameWrapper.sol:2		pragma solidity ^0.8.4;
contracts/wrapper/ERC1155Fuse.sol:2		pragma solidity ^0.8.4;
contracts/wrapper/BytesUtil.sol:2		pragma solidity >=0.8.4;

Reference:

https://code4rena.com/reports/2022-04-phuture#g-20-use-a-more-recent-version-of-solidity)

---

5. File does not contain an SPDX Identifier

There are 13 instance of where the smart contract files does not contain spdx identifier.

contracts/registry/IReverseRegistrar.sol contracts/registry/ENS.sol contracts/ethregistrar/ETHRegistrarController.sol contracts/ethregistrar/StringUtils.sol contracts/ethregistrar/IETHRegistrarController.sol contracts/dnssec-oracle/BytesUtils.sol contracts/dnssec-oracle/SHA1.sol contracts/dnssec-oracle/Owned.sol contracts/dnssec-oracle/DNSSEC.sol contracts/dnssec-oracle/algorithms/Algorithm.sol contracts/dnssec-oracle/RRUtils.sol contracts/dnssec-oracle/digests/Digest.sol contracts/wrapper/IMetadataService.sol

---

---

1. An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Instances:

contracts/ethregistrar/ETHRegistrarController.sol:256: contracts/dnssec-oracle/DNSSECImpl.sol:93: contracts/dnssec-oracle/RRUtils.sol:310: contracts/wrapper/ERC1155Fuse.sol:205: contracts/wrapper/ERC1155Fuse.sol:92:

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

Remediation:

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead.

---

2. Require instead of &&

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

Instances include:

contracts/wrapper/ERC1155Fuse.sol::215 contracts/wrapper/ERC1155Fuse.sol:290 contracts/dnssec-oracle/BytesUtils.sol:268

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

Mitigation:

Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)

---

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

Saves 6 gas PER LOOP

Instances:

contracts/ethregistrar/ETHRegistrarController.sol:256: contracts/ethregistrar/StringUtils.sol:14: contracts/dnssec-oracle/BytesUtils.sol:266: contracts/dnssec-oracle/BytesUtils.sol:313: contracts/dnssec-oracle/DNSSECImpl.sol:93:

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

Reference:

https://code4rena.com/reports/2022-05-cally#g-19-i-costs-less-gas-than-i-especially-when-its-used-in-for-loops---ii---too

---

4. Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because 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. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

There are 2 instances of this issue: contracts/wrapper/Controllable.sol:7: contracts/wrapper/ERC1155Fuse.sol:19:

contracts/wrapper/Controllable.sol:7:    mapping(address=>bool) public controllers;
contracts/wrapper/ERC1155Fuse.sol:19:    mapping(address => mapping(address => bool)) private _operatorApprovals;

---

5. abi.encode() is less efficient than abi.encodePacked()

There are 1 instances of this issue: contracts/ethregistrar/ETHRegistrarController.sol:106:

contracts/ethregistrar/ETHRegistrarController.sol:106:                abi.encode(

Reference:

https://code4rena.com/reports/2022-06-notional-coop#15-abiencode-is-less-efficient-than-abiencodepacked

---

6. Custom Errors instead of Revert Strings to save Gas

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

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

Instances:

RRUtils.sol: 307

ETHRegistrarController.sol: 99-102 137-140 196-199 232-235 238-241 242 259-262

ReverseRegistrar.sol 41-47 52-56

ByteUtil.sol 28 42

Controllable.sol 17

ERC1155Fuse.sol 60-63 85-88 107-110 176 177-180 195-198 199 200-203 215-218 248 249 250-253 290-293

References:

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

Remediation:

I suggest replacing revert strings with custom errors.

---

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

Instances:

ETHRegistrarController.sol: 99-102 137-140 196-199 232-235 238-241 242 259-262

ReverseRegistrar.sol 41-47 52-56

Controllable.sol 17

ERC1155Fuse.sol 60-63 85-88 107-110 176 177-180 195-198 199 200-203 215-218 249 250-253 290-293

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.****

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 Custom Errors in Solidity:

---

8. Variables: No need to explicitly initialize variables with default values

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.

We can use uint number; instead of uint number = 0;

Instances:

contracts/ethregistrar/ETHRegistrarController.sol:256 contracts/ethregistrar/StringUtils.sol:12: contracts/dnssec-oracle/BytesUtils.sol:56:
contracts/dnssec-oracle/BytesUtils.sol:264:
contracts/dnssec-oracle/BytesUtils.sol:266: contracts/dnssec-oracle/DNSSECImpl.sol:93: contracts/dnssec-oracle/RRUtils.sol:50:
contracts/dnssec-oracle/RRUtils.sol:63: contracts/dnssec-oracle/RRUtils.sol:181: contracts/dnssec-oracle/RRUtils.sol:200: contracts/dnssec-oracle/RRUtils.sol:310: contracts/wrapper/INameWrapper.sol:16: contracts/wrapper/ERC1155Fuse.sol:92:
contracts/wrapper/ERC1155Fuse.sol:205:

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

---

---