ENS contest - m_Rassska's results

Table of contents

• [0x0] Disclaimer
• [G-01] Try ++i instead of i++
• [G-02] Try unchecked{++i} instead of i++ in loops
• [G-03] Consider a = a + b instead of a += b
• [G-04] Consider marking onlyOwner functions as payable
• [G-05] Use binary shifting instead of a / 2^x, x > 0
• [G-06] Cache state variables, MLOAD << SLOAD
• [G-07] Add require() before some computations, if it makes sense
• [G-08] Internal functions can be inlined
• [G-09] Use private/internal for constants/immutable variables instead of public
• [G-10] Use const values instead of type(uint256).max
• [G-11] Use uint instead of uint8, uint64, if it's possible
• [G-12] Mark functions as external instead of public, if there are no internal calls
• [G-13] Use calldataload instead of mload
• [G-14] Use revert or require instead of assert

Disclaimer<a name="0x0"></a>

• Please, consider everything described below as a general recommendation. These notes will represent potential possibilities to optimize gas consumption. It's okay, if something is not suitable in your case. Just let me know the reason in the comments section. Enjoy!

[G-01] Try ++i instead of i++<a name="G-01"></a>

Description:

• In case of i++, the compiler has to to create a temp variable to return i (if there's a need) and then i gets incremented.
• In case of ++i, the compiler just simply returns already incremented value.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/BytesUtils.sol |
    -------------------------------
  
    // Initial version:
      // Lines: [266-266]
        for(uint i = 0; i < len; i++) {} 
  
    // Proposed by m_Rassska:
      // Lines: [266-266]
        for(uint i = 0; i < len;) {
          // some code
  
          unchecked{
            ++i;
          }
        } 
  
    // Also occured in the following files: 
        src/**/DNSSECImpl.sol 
        src/**/RRUtils.sol 
        src/**/StringUtils.sol 
        src/**/ETHRegistrarController.sol 
        src/**/ERC1155Fuse.sol 
  
  
    -------------------------------
    | file: src/**/RRUtils.sol |
    -------------------------------
  
    // Initial version:
      // Lines: [58-58]
        count += 1;
  
    // Proposed by m_Rassska:
      // Lines: [58-58]
        unchecked{
          ++i;
        };
  
    // Also occured in the following files: 
        src/**/StringUtils.sol 
  
  

[G-02] try unchecked{++i}; instead of i++;/++i; in loops<a name="G-02"></a>

Description:

• Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default Therefore, unchecked box can be used to prevent all unnecessary checks, if it's no a way to get a reversion.

• There are ~1e80 atoms in the universe, so 2^256 is closed to that number, therefore it's no a way to be overflowed, because of the gas limit as well.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/BytesUtils.sol |
    -------------------------------
    
    // Initial versions:
      // Lines: [56-56]
        for (uint idx = 0; idx < shortest; idx += 32) {}
  
    // Proposed by m_Rassska:
      // Lines: [266-266]
        for (uint idx = 0; idx < shortest;) {  
          // some code
  
          unchecked{
            idx = idx + 32;
          }
        } 
  
    // Also occured in the following files: 
        src/**/RRUtils.sol 
  
  
    -------------------------------
    | file: src/**/RRUtils.sol |
    -------------------------------
  
    // Initial versions:
      // Lines: [24-24]
        idx += labelLen + 1; 
  
      // Lines: [54-54]
        offset += labelLen + 1;
  
      // Lines: [58-58]
        count += 1;
  
  
    // Proposed by m_Rassska:
      // Lines: [24-24]
        idx = idx + ++labelLen
  
      // Lines: [54-54]
        unchecked {++labelLen;}
        offset = offset + labelLen;
  
      // Lines: [58-58]
        unchecked {++count};
  
    // Also occured in the following files: 
  
  

[G-03] Consider a = a + b instead of a += b<a name="G-03"></a>

Description:

• It has an impact on the deployment cost and the cost for distinct transaction as well.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/RRUtils.sol |
    -------------------------------
  
    // Initial versions:
      // Lines: [75-76]
          selfptr += 32;
          otherptr += 32;
  
    // Proposed by m_Rassska:
      // Lines: [75-76]
          selfptr = selfptr + 32;
          otherptr = otherptr + 32;
  
    // Also occured in the following files: 
        src/**/RRUtils.sol 
        src/**/StringUtils.sol 
        src/**/SHA1.sol 
  

[G-04] Consider marking onlyOwner functions as payable<a name="G-04"></a>

Description:

• This one is a bit questionable, but you can try that out. So, the compiler adds some extra conditions in case of non-payable, but we know that onlyOwner modifier will be reverted, if the user invoke following methods.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
  
    // Initial versions:
      // Lines: [58-61]
          function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
              algorithms[id] = algo;
              emit AlgorithmUpdated(id, address(algo));
          }
  
    // Proposed by m_Rassska:
      // Lines: [58-61]
          function setAlgorithm(uint8 id, Algorithm algo) public payable owner_only {
              algorithms[id] = algo;
              emit AlgorithmUpdated(id, address(algo));
          }
    
    // Also occured in the following files: 
        src/**/Owned.sol 
        src/**/ReverseRegistrar.sol 
        src/**/NameWrapper.sol 
        src/**/Controllable.sol 
  
      ```

[G-05] Use binary shifting instead of a / 2^x, x > 0 or a * 2^x, x > 0<a name="G-05"></a>

Description:

• It's also pretty impactful one, especially in loops.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
    // Initial versions:
      // Lines: [316-316]
        uint unused = 256 - (data.length - i) * 8;
  
    // Proposed by m_Rassska:
      // Lines: [316-316]
        uint unused = 256 - (data.length - i) << 3;
  
    // Also occured in the following files: 
  
    ```

[G-06] Cache state variables, MLOAD << SLOAD<a name="G-06"></a>

Description:

• MLOAD costs only 3 units of gas, SLOAD(warm access) is about 100 units. Therefore, cache, when it's possible.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/NameWrapper.sol |
    -------------------------------
    // Initial versions:
      // Lines: [128-143]
          function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
              public
              onlyOwner
          {
              if (address(upgradeContract) != address(0)) {
                  registrar.setApprovalForAll(address(upgradeContract), false);
                  ens.setApprovalForAll(address(upgradeContract), false);
              }
  
              upgradeContract = _upgradeAddress;
  
              if (address(upgradeContract) != address(0)) {
                  registrar.setApprovalForAll(address(upgradeContract), true);
                  ens.setApprovalForAll(address(upgradeContract), true);
              }
          }
  
    // Proposed by m_Rassska:
      // Lines: [128-143]
          function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
              public
              onlyOwner
          {
              address upgradeAddressPrev = address(upgradeContract);
  
              if (upgradeAddressPrev != address(0)) {
                  registrar.setApprovalForAll(upgradeAddressPrev , false);
                  ens.setApprovalForAll(upgradeAddressPrev , false);
              }
  
              upgradeContract = _upgradeAddress;
              address upgradeAddressCurr = address(_upgradeAddress);
  
              if (upgradeAddressCurr != address(0)) {
                  registrar.setApprovalForAll(upgradeAddressCurr , true);
                  ens.setApprovalForAll(upgradeAddressCurr , true);
              }
          }
  
    // Also occured in the following files: 
        src/**/ETHRegistrarController.sol 
  
      ```

[G-07] Add require() before some computations, if it makes sense<a name="G-07"></a>

Description:

• Everyting above require() takes some gas for execution. Sometimes it's usefull to rearrange require statements or place them before any computations.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/ETHRegistrarController.sol |
    -------------------------------
    // Initial versions:
      // Lines: [232-242]
        function _consumeCommitment(
            string memory name,
            uint256 duration,
            bytes32 commitment
        ) internal {
            // Require a valid commitment (is old enough and is committed)
            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");
  
            delete (commitments[commitment]);
  
            require(duration >= MIN_REGISTRATION_DURATION);
        }
  
    // Proposed by m_Rassska:
      // Lines: [232-242]
        function _consumeCommitment(
            string memory name,
            uint256 duration,
            bytes32 commitment
        ) internal {
            // Require a valid commitment (is old enough and is committed)
            require(available(name), "ETHRegistrarController: Name is unavailable");
  
            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"
            );
  
            delete (commitments[commitment]);
  
            require(duration >= MIN_REGISTRATION_DURATION);
        }
  
    // Also occured in the following files: 
        src/**/BytesUtil.sol 
        src/**/ERC1155Fuse.sol 
        src/**/NameWrapper.sol 
  

[G-08] Internal functions can be inlined<a name="G-08"></a>

Description:

• It takes some extra JUMPs. Especially, if it takes as an arg bytes or arrays, it would be expensive. There is a tradeoff here between: code readability and optimization. Try to evalute it.

All occurances:

• Contracts:

    -------------------------------------
    | file: src/**/ReverseRegistrar.sol |
    -------------------------------------
    // Initial versions:
      // Lines: [181-187]
       function ownsContract(address addr) internal view returns (bool) {
            try Ownable(addr).owner() returns (address owner) {
                return owner == msg.sender;
            } catch {
                return false;
            }
        } 
  
    // Proposed by m_Rassska:
      // Lines: [181-187]
        // Comment: Remove the internal function with a simple logic.
  
    // Also occured in the following files: 
        src/**/DNSSECImpl.sol 
        src/**/RRUtils.sol 
        src/**/NameWrapper.sol 
        src/**/ETHRegistrarController.sol 
        src/**/BytesUtils.sol 
  

[G-09] Use private/internal for constants/immutable variables instead of public<a name="G-09"></a>

Description:

• Optimization comes from not creating a getter function for each public instance.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
    // Initial versions:
      // Lines: [21-26]
        uint16 constant DNSCLASS_IN = 1;
        uint16 constant DNSTYPE_DS = 43;
        uint16 constant DNSTYPE_DNSKEY = 48;
        uint constant DNSKEY_FLAG_ZONEKEY = 0x100;
  
    // Proposed by m_Rassska:
      // Lines: [21-26]
        uint16 constant private DNSCLASS_IN = 1;
        uint16 constant private DNSTYPE_DS = 43;
        uint16 constant private DNSTYPE_DNSKEY = 48;
        uint constant private DNSKEY_FLAG_ZONEKEY = 0x100;
  
    // Also occured in the following files: 
        src/**/RRUtils.sol 
        src/**/DNSSEC.sol 
        src/**/ReverseRegistrar.sol 
        src/**/NameWrapper.sol 
        src/**/ETHRegistrarController.sol 
        src/**/Controllable.sol 
  

[G-10] Use const values instead of type(uint256).max<a name="G-10"></a>

Description:

• Not sure about readability, but it might be tangible in loops.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/BytesUtils.sol |
    -------------------------------
    // Proposed by m_Rassska:
      // Lines: [67-67]
        mask = type(uint256).max;
  
    // Proposed by m_Rassska:
      // Lines: [67-67]
        mask = 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; // type(uint).max
  
    // Also occured in the following files: 
        src/**/NameWrapper.sol 
  

[G-11] Use uint instead of uint8, uint64, if it's possible<a name="G-11"></a>

Description:

• Stack slot is 32bytes, therefore using explicit casts like uint8, uint64 leads to overheads.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
    // Initial versions:
      // Lines: [21-26]
        uint16 constant DNSCLASS_IN = 1;
        uint16 constant DNSTYPE_DS = 43;
        uint16 constant DNSTYPE_DNSKEY = 48;
        uint constant DNSKEY_FLAG_ZONEKEY = 0x100;
  
    // Proposed by m_Rassska:
      // Lines: [21-26]
        uint constant DNSCLASS_IN = 1;
        uint constant DNSTYPE_DS = 43;
        uint constant DNSTYPE_DNSKEY = 48;
        uint constant DNSKEY_FLAG_ZONEKEY = 0x100;
  
    // Also occured in the following files: 
        src/**/RRUtils.sol 
        src/**/DNSSEC.sol 
        src/**/ReverseRegistrar.sol 
        src/**/NameWrapper.sol 
        src/**/StringUtils.sol 
        src/**/Controllable.sol 
        src/**/ENS.sol 
        src/**/Resolver.sol 
  
    ```

[G-12] Mark functions as external instead of public, if there are no internal calls<a name="G-12"></a>

Description:

• Functions marked by external use call data to read arguments, where public will first allocate in local memory and then load them.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
    // Initial versions:
      // Lines: [58-58]
        function setAlgorithm(uint8 id, Algorithm algo) public owner_only {} 
      // Lines: [69-69]
        function setDigest(uint8 id, Digest digest) public owner_only {}
  
    // Proposed by m_Rassska:
      // Lines: [58-58]
        function setAlgorithm(uint8 id, Algorithm algo) external owner_only {} 
      // Lines: [69-69]
        function setDigest(uint8 id, Digest digest) external owner_only {}
  
    // Also occured in the following files: 
        src/**/Owned.sol 
        src/**/ReverseRegistrar.sol 
        src/**/ERC1155Fuse.sol 
        src/**/Controllable.sol 
        src/**/NameWrapper.sol 
  
    ```

[G-13] Use calldataload instead of mload<a name="G-13"></a>

Description:

• In EIP-2029, gas consumption for non-zero byte read from calldata was dicreased from 68 to ~16, therefore, try to use it to save gas per each transaction and on the deployment stage as well. Here you need to explicitly specify calldataload, or replace memory with calldata, if args are not supposed to be modified.

All occurances:

• Contracts:

    -------------------------------
    | file: src/**/DNSSECImpl.sol |
    -------------------------------
    // Initial versions:
      // Lines: [183-183]
        function verifySignature(bytes memory name, RRUtils.SignedSet memory rrset, RRSetWithSignature memory data, bytes memory proof) internal view {}
  
    // Proposed by m_Rassska:
      // Lines: [183-183]
        function verifySignature(bytes calldata name, RRUtils.SignedSet calldata rrset, RRSetWithSignature memory data, bytes memory proof) internal view {}
  
    // Also occured in the following files: 
        src/**/BytesUtils.sol 
        src/**/RRUtils.sol 
        src/**/ETHRegistrarController.sol 
        src/**/ModexpPrecompile.sol 
        src/**/ReverseRegistrar.sol 
        src/**/NameWrapper.sol 
        src/**/Resolver.sol  // looks nice, thanks!
  
    ```

[G-14] Use revert or require instead of assert<a name="G-14"></a>

Description:

• Assert doesn't refund gas in case of failure.

All occurances:

• Contracts:

    ----------------------------
    | file: src/**/RRUtils.sol |
    ----------------------------
    // Initial versions:
      // Lines: [22-22]
        assert(idx < self.length);
      // Lines: [52-52]
          assert(offset < self.length);
  
    // Proposed by m_Rassska:
      // Lines: [22-22]
        require(idx < self.length);
      // Lines: [52-52]
          require(offset < self.length);
  
    // Also occured in the following files: 
  
    ```
  

Kudos for the quality of the code! It's pretty easy to explore