ENS contest - __141345__'s results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185 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#L210-L212

Vulnerability details

Use of transfer might render ETH impossible to withdraw

Impact

Proof of Concept

When withdrawing and refund extra ETH, the ETHRegistrarController contract uses Solidity’s transfer() function.

Using Solidity's transfer() function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-effects-interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. 

Proof of Concept

contracts/ethregistrar/ETHRegistrarController.sol

183-185:
        payable(msg.sender).transfer(
            msg.value - (price.base + price.premium)
        );

204:
        payable(msg.sender).transfer(msg.value - price.base);        

210-212:
    function withdraw() public {
        payable(owner()).transfer(address(this).balance);
    }

References:

The issues with transfer() are outlined here

For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles.

Tools Used

Mannual analysis.

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised, see reference.

Remove deprecated events and functions

contracts/resolvers/Resolver.sol

34-35:
    /* Deprecated events */
    event ContentChanged(bytes32 indexed node, bytes32 hash);

79-86:
    /* Deprecated functions */
    function content(bytes32 node) external view returns (bytes32);

    function multihash(bytes32 node) external view returns (bytes memory);

    function setContent(bytes32 node, bytes32 hash) external;

    function setMultihash(bytes32 node, bytes calldata hash) external;

SafeMath can be removed

SafeMath is not used in these 2 files: contracts/ethregistrar/ExponentialPremiumPriceOracle.sol contracts/ethregistrar/StablePriceOracle.sol

solidity >=0.8.4 has built-in arithmic check, SafeMath can be replaced with regular arithmic operation. contracts/ethregistrar/LinearPremiumPriceOracle.sol

NATSPEC not complete

contracts/resolvers/Multicallable.sol
contracts/resolvers/PublicResolver.sol
contracts/resolvers/Resolver.sol
contracts/wrapper/NameWrapper.sol
contracts/ethregistrar/ETHRegistrarController.sol

Suggestion: Follow NATSPEC.

Typo

contracts/wrapper/NameWrapper.sol

370:     * @param fuses fuses to burn (cannot burn PARENT_CANOT_CONTROL)

should be PARENT_CANNOT_CONTROL .

FOR-LOOPS GAS-SAVING

There are several for loops can be improved to save gas

contracts/dnssec-oracle/BytesUtils.sol:

56-77:
        for (uint idx = 0; idx < shortest; idx += 32) {
            // ...
            selfptr += 32;
            otherptr += 32;
        }

207-215:
    function memcpy(uint dest, uint src, uint len) private pure {
        // Copy word-length chunks while possible
        for (; len >= 32; len -= 32) {
            assembly {
                mstore(dest, mload(src))
            }
            dest += 32;
            src += 32;
        }

266:    for(uint i = 0; i < len; i++) {

313:    for(uint256 idx = off; idx < off + len; idx++) {

contracts/ethregistrar/BulkRenewal.sol:

41-47:
        for (uint256 i = 0; i < names.length; i++) {

56-65:
        for (uint256 i = 0; i < names.length; i++) {

contracts/ethregistrar/BulkRenewal.sol:

56-65:
        for (uint256 i = 0; i < data.length; i++) {

contracts/ethregistrar/ETHRegistrarController.sol:

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

contracts/wrapper/ERC1155Fuse.sol:

92-94:
        for (uint256 i = 0; i < accounts.length; ++i) {
            batchBalances[i] = balanceOf(accounts[i], ids[i]);
        }

205:    for (uint256 i = 0; i < ids.length; ++i) {

contracts/ethregistrar/StringUtils.sol:

        for(len = 0; i < bytelength; len++) {

contracts/utils/NameEncoder.sol:

27:     for (uint256 i = length - 1; i >= 0; 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/dnssec-oracle/algorithms/EllipticCurve.sol:

304:    for(uint i = 0; i < exp; i++) {

contracts/ethregistrar/StringUtils.sol:

11-14:
        uint len;
        uint i = 0;
        uint bytelength = bytes(s).length;
        for(len = 0; i < bytelength; len++) {

contracts/resolvers/Multicallable.sol:

10:     for(uint i = 0; i < data.length; i++) {

suggestion: cache the for loop length to memory before the loop.

1. LOOP LENGTH COULD BE CACHED TO MEMORY BEFORE THE LOOP

    uint length = names.length;

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

3. ++I COSTS LESS GAS COMPARED TO I++ OR I += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked.

Summary: Take the following as one example

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

can be written as

    uint length = names.length;
    unchecked {
        for (uint256 i; i < length; ++i) {
            treeLength += IModule(_modules[i]).getLeafNodes().length;
        }
    }

And the optimizer need to be turned on.

The demo of the loop gas comparsion can be seen here.

X = X + Y IS CHEAPER THAN X += Y

Consider use X = X + Y to save gas

contracts/dnssec-oracle/RRUtils.sol:

24:         idx += labelLen + 1;
54:         offset += labelLen + 1;
58:         count += 1;
150:        off += 2;
152:        off += 2;
154:        off += 4;
250:        counts -= 1;

contracts/ethregistrar/BulkRenewal.sol:

46:         total += (price.base + price.premium);

contracts/utils/NameEncoder.sol:

38:         labelLength += 1;

contracts/ethregistrar/StringUtils.sol:

            if(b < 0x80) {
                i += 1;
            } else if (b < 0xE0) {
                i += 2;
            } else if (b < 0xF0) {
                i += 3;
            } else if (b < 0xF8) {
                i += 4;
            } else if (b < 0xFC) {
                i += 5;
            } else {
                i += 6;
            }

contracts/dnssec-oracle/BytesUtils.sol:

284:         bitlen -= 2;
288:         bitlen -= 4;
292:         bitlen -= 1;
296:         bitlen -= 3;                                    

The demo of the gas comparsion can be seen here.

SafeMath can be removed

solidity >=0.8.4 has built-in arithmic check, SafeMath can be replaced with regular arithmic operation. contracts/ethregistrar/LinearPremiumPriceOracle.sol:

USE CALLDATA INSTEAD OF MEMORY

When arguments are read-only on external functions, the data location can be calldata.

contracts/dnssec-oracle/DNSSECImpl.sol:

80-82:
    function verifyRRSet(RRSetWithSignature[] memory input) external virtual view override returns(bytes memory) {
        return verifyRRSet(input, block.timestamp);
    }

The demo of the gas comparsion can be seen here.

FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as owner_only() 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.

contracts/dnssec-oracle/Owned.sol:
18:     function setOwner(address newOwner) public owner_only {

contracts/dnssec-oracle/DNSSECImpl.sol:
58:     function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
69:     function setDigest(uint8 id, Digest digest) public owner_only {

120:    function _register(uint256 id, address owner, uint duration, bool updateRegistry) internal live onlyController returns(uint) {
139:    function renew(uint256 id, uint duration) external override live onlyController returns(uint) {
151:    function reclaim(uint256 id, address owner) external override live {    

contracts/registry/ReverseRegistrar.sol:
51:     function setDefaultResolver(address resolver) public override onlyOwner {

contracts/root/Controllable.sol:
18:     function setController(address controller, bool enabled) public onlyOwner {

contracts/wrapper/NameWrapper.sol:
105:    function setMetadataService(IMetadataService _newMetadataService) public onlyOwner
128:    function setUpgradeContract(INameWrapperUpgrade _upgradeAddress) public onlyOwner

contracts/registry/ENSRegistry.sol:
63:     function setOwner(bytes32 node, address owner) public virtual override authorised(node) {
74:     function setSubnodeOwner(bytes32 node, bytes32 label, address owner) public virtual override authorised(node) returns(bytes32) {
86:     function setResolver(bytes32 node, address resolver) public virtual override authorised(node) {
96:     function setTTL(bytes32 node, uint64 ttl) public virtual override authorised(node) {

contracts/registry/ReverseRegistrar.sol:
76:     function claimForAddr() public override authorised(addr) returns (bytes32) {

contracts/registry/FIFSRegistrar.sol:
33:     function register(bytes32 label, address owner) public only_owner(label) {

Use bit shift instead of power operation

compare() and memcpy() seem to be frequently called functions.

contracts/dnssec-oracle/BytesUtils.sol:

69:         mask = ~(2 ** (8 * (32 - shortest + idx)) - 1);
219:        uint mask = (256 ** (32 - len)) - 1;

Suggestion: Change the above to

69:         mask = ~(2 << ((8 * (32 - shortest + idx)) - 1 ) - 1);
219:        uint mask = (2 << ( 8 * (32 - len) - 1 )) - 1;

The demo of the gas comparsion can be seen here.

Guaranteed non overflow/underflow can be unchecked

contracts/dnssec-oracle/RRUtils.sol: contain multiple arithmic operations won't overflow/underflow, and some are likely to be called oftren, so they can be unchecked to save gas.

20-29:
        uint idx = offset;
        while (true) {
            assert(idx < self.length);
            uint labelLen = self.readUint8(idx);
            idx += labelLen + 1;
            if (labelLen == 0) {
                break;
            }
        }
        return idx - offset;

Here

        return unchecked { idx - offset; }

94:         self.data = data.substring(RRSIG_SIGNER_NAME + self.signerName.length, data.length - RRSIG_SIGNER_NAME - self.signerName.length);
197:        self.publicKey = data.substring(offset + DNSKEY_PUBKEY, length - DNSKEY_PUBKEY);
216:        self.digest = data.substring(offset + DS_DIGEST, length - DS_DIGEST);

    function next(RRIterator memory iter) internal pure {
        iter.offset = iter.nextOffset;
        if (iter.offset >= iter.data.length) {
            return;
        }

        // Skip the name
        uint off = iter.offset + nameLength(iter.data, iter.offset);

        // Read type, class, and ttl
        iter.dnstype = iter.data.readUint16(off);
        off += 2;
        iter.class = iter.data.readUint16(off);
        off += 2;
        iter.ttl = iter.data.readUint32(off);
        off += 4;

        // Read the rdata
        uint rdataLength = iter.data.readUint16(off);
        off += 2;
        iter.rdataOffset = off;
        iter.nextOffset = off + rdataLength;
    }

    function progress(bytes memory body, uint off) internal pure returns(uint) {
        return off + 1 + body.readUint8(off);
    }

contracts\wrapper\BytesUtil.sol

13:         require(offset + len <= self.length);
45:         labelhash = keccak(self, idx + 1, len);
49:         newIdx = idx + len + 1;

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

contracts/dnssec-oracle/RRUtils.sol:

50:        uint count = 0;

--x COSTS LESS GAS COMPARED TO x--

232-242:
        while (counts > othercounts) {
            prevoff = off;
            off = progress(self, off);
            counts--;
        }

        while (othercounts > counts) {
            otherprevoff = otheroff;
            otheroff = progress(other, otheroff);
            othercounts--;
        }

Suggestion: Since neither counts and othercounts will underflow, they can be unchecked.

    unchecked {
        while (counts > othercounts) {
            prevoff = off;
            off = progress(self, off);
            --counts;
        }

        while (othercounts > counts) {
            otherprevoff = otheroff;
            otheroff = progress(other, otheroff);
            --othercounts;
        }

    }

And also turn on the optimizer.

The demo of the loop gas comparsion can be seen here.

REDUCE THE SIZE OF ERROR MESSAGES

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.

contracts/ethregistrar/ETHRegistrarController.sol:

99-102:
        require(
            resolver != address(0),
            "ETHRegistrarController: resolver is required when data is supplied"
        );

137-140:
        require(
            msg.value >= (price.base + price.premium),
            "ETHRegistrarController: Not enough ether provided"
        );

196-199:
        require(
            msg.value >= price.base,
            "ETHController: Not enough Ether provided for renewal"
        );

232-242:
        require(
            commitments[commitment] + minCommitmentAge <= block.timestamp,
            "ETHRegistrarController: Commitment is not valid"
        );

        // If the commitment is too old, or the name is registered, stop
        require(
            commitments[commitment] + maxCommitmentAge > block.timestamp,
            "ETHRegistrarController: Commitment has expired"
        );
        require(available(name), "ETHRegistrarController: Name is unavailable");

259-266:
        require(
            txNamehash == nodehash,
            "ETHRegistrarController: Namehash on record do not match the name being registered"
        );
        resolver.functionCall(
            data[i],
            "ETHRegistrarController: Failed to set Record"
        );

contracts\wrapper\ERC1155Fuse.sol

107-110:
        require(
            msg.sender != operator,
            "ERC1155: setting approval status for self"
        );

176-180:
        require(to != address(0), "ERC1155: transfer to the zero address");
        require(
            from == msg.sender || isApprovedForAll(from, msg.sender),
            "ERC1155: caller is not owner nor approved"
        );

195-203:
        require(
            ids.length == amounts.length,
            "ERC1155: ids and amounts length mismatch"
        );
        require(to != address(0), "ERC1155: transfer to the zero address");
        require(
            from == msg.sender || isApprovedForAll(from, msg.sender),
            "ERC1155: transfer caller is not owner nor approved"
        );

215-218:
        require(
            amount == 1 && oldOwner == from,
            "ERC1155: insufficient balance for transfer"
        );

248-253:
        require(owner == address(0), "ERC1155: mint of existing token");
        require(newOwner != address(0), "ERC1155: mint to the zero address");
        require(
            newOwner != address(this),
            "ERC1155: newOwner cannot be the NameWrapper contract"
        );

290-293:
        require(
            amount == 1 && oldOwner == from,
            "ERC1155: insufficient balance for transfer"
        );

322:
        revert("ERC1155: ERC1155Receiver rejected tokens");
328:
        revert("ERC1155: transfer to non ERC1155Receiver implementer");
354:
        revert("ERC1155: ERC1155Receiver rejected tokens");
359:
        revert("ERC1155: transfer to non ERC1155Receiver implementer");

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) reference

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

These 2 files contain multiple require() and revert statements can use custom errors: contracts/ethregistrar/ETHRegistrarController.sol contracts\wrapper\ERC1155Fuse.sol

block.timestamp can be cached

contracts\ethregistrar\ETHRegistrarController.sol

    function commit(bytes32 commitment) public override {
        require(commitments[commitment] + maxCommitmentAge < block.timestamp);
        commitments[commitment] = block.timestamp;
    }