ENS contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L341-L345

Vulnerability details

Impact

When the unwrapETH2LD function is called, the registrar ERC721 token is transferred from the NameWrapper contract to the newRegistrant address. If newRegistrant is a contract that does not support the ERC721 protocol, the token will be locked and cannot be retrieved.

For reference, OpenZeppelin's documentation for transferFrom states: "Usage of this method is discouraged, use safeTransferFrom whenever possible."

Proof of Concept

The following steps can occur.

1. The NameWrapper contract is deployed with the deployed BaseRegistrarImplementation contract as the registrar state variable.
2. In the NameWrapper contract, the unwrapETH2LD function is called to run registrar.transferFrom(address(this), newRegistrant, uint256(labelhash).
3. The transferFrom function of the BaseRegistrarImplementation contract, which inherits from the OpenZeppelin ERC721 contract, is called.
4. After the OpenZeppelin ERC721's transferFrom function finishes, the _checkOnERC721Received function, which would be called if running the safeTransferFrom function, is not triggered.
5. If newRegistrant is a contract, it is not guaranteed to inherit from the IERC721Receiver interface or have the onERC721Received function because of Step 4. The corresponding ERC721 token can be locked if this contract does not support the ERC721 protocol.

Tools Used

VSCode

Recommended Mitigation Steps

In the unwrapETH2LD function of the NameWrapper contract where registrar.transferFrom(address(this), newRegistrant, uint256(labelhash) is called, transferFrom can be changed to safeTransferFrom.

[L-01] UNRESOLVED TODO COMMENT

Comments regarding todos indicate that there are unresolved action items for implementation, which need to be addressed before protocol deployment.

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

[L-02] MISSING ZERO-ADDRESS CHECKS FOR CRITICAL ADDRESSES

In the following functions, _transfer is used instead of safeTransferFrom. To resemble safeTransferFrom's behavior and to prevent unintended actions, unexpected loss of assets, etc., the address inputs should be checked against address(0).

wrapper\NameWrapper.sol
    584-589:
        function setRecord(
            bytes32 node,
            address owner,
            address resolver,
            uint64 ttl
        )
    813-818:
        function _transferAndBurnFuses(
            bytes32 node,
            address newOwner,
            uint32 fuses,
            uint64 expiry
        ) internal {

[L-03] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers in the following code with constants.

ethregistrar\ETHRegistrarController.sol
    78: return name.strlen() >= 3;  
	
registry\ReverseRegistrar.sol
    165: let i := 40  
    170: mstore8(i, byte(and(addr, 0xf), lookup))  
    171: addr := div(addr, 0x10)  
    173: mstore8(i, byte(and(addr, 0xf), lookup))  
    174: addr := div(addr, 0x10)  
    177: ret := keccak256(0, 40)  
	
wrapper\ERC1155Fuse.sol
    162: (uint256(fuses) << 160) |  
    163: (uint256(expiry) << 192);  

[N-01] ABI CODER V2 IS NO LONGER EXPERIMENTAL SINCE SOLIDITY V0.8.0

The release announcement of Solidity v0.8.0 states: "The second change that is very visible is that the ABI coder v2 is activated by default. You can activate the old coder using pragma abicoder v1, or explicitly select v2 using pragma abicoder v2 - which has the same effect as pragma experimental ABIEncoderV2 had. ABI coder v2 is more complex than v1 but also performs additional checks on the input and supports a larger set of types than v1." Hence, pragma abicoder v2 can be explicitly used instead of pragma experimental ABIEncoderV2.

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

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

[N-02] OperationProhibited ERROR CAN BE MORE SPECIFIC

The OperationProhibited error will be thrown under different revert conditions. Just recording node might not be specific enough. Please consider include other fields, such as the fuses that are involved.

wrapper\ERC1155Fuse.sol
    12: error OperationProhibited(bytes32 node);

[N-03] MISSING REASON STRINGS IN REQUIRE AND REVERT STATEMENTS

When the reason strings are missing in the require and revert statements, it is unclear about why certain conditions revert. Please add descriptive reason strings for the following require and revert statements.

dnssec-oracle\BytesUtils.sol
    12: require(offset + len <= self.length);
    146: require(idx + 2 <= self.length);
    159: require(idx + 4 <= self.length);
    172: require(idx + 32 <= self.length);
    185: require(idx + 20 <= self.length);
    199: require(len <= 32);
    200: require(idx + len <= self.length);
    235: require(offset + len <= self.length);
    262: require(len <= 52);
    268: require(char >= 0x30 && char <= 0x7A);
    270: require(decoded <= 0x20);
	
dnssec-oracle\Owned.sol
    10: require(msg.sender == owner);
	
ethregistrar\ETHRegistrarController.sol
    57: require(_maxCommitmentAge > _minCommitmentAge);
    121: require(commitments[commitment] + maxCommitmentAge < block.timestamp);
    246: require(duration >= MIN_REGISTRATION_DURATION);
	
wrapper\BytesUtil.sol
    13:  require(offset + len <= self.length);

[N-04] REDUNDANT NAMED RETURNS

When a function has unused named returns and used return statement(s), these named returns become redundant. To improve readability and maintainability, named returns need to be used and return statement(s) need to be removed or vice versa.

dnssec-oracle\BytesUtils.sol
    135: function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {

wrapper\NameWrapper.sol
    94: returns (address owner)
    744: returns (bytes memory ret)

[N-05] REDUNDANT CAST

The resolver input is already an address type so it does not need to be cast to address again in the following function.

registry\ReverseRegistrar.sol
    51-57:
        function setDefaultResolver(address resolver) public override onlyOwner {
            require(
                address(resolver) != address(0),
                "ReverseRegistrar: Resolver address must not be 0"
            );
            defaultResolver = NameResolver(resolver);
        }

[N-06] PUBLIC FUNCTIONS NOT CALLED BY OWN CONTRACT CAN BE EXTERNAL

For public functions that are not called by their own contract, their visibilities can be set to external for readability and maintainability.

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

ethregistrar\ETHRegistrarController.sol
    120: function commit(bytes32 commitment) public override { 
    210: function withdraw() public { 

registry\ReverseRegistrar.sol
    51: function setDefaultResolver(address resolver) public override onlyOwner { 
    65: function claim(address owner) public override returns (bytes32) { 
    148: function node(address addr) public pure override returns (bytes32) { 

wrapper\Controllable.sol
    11: function setController(address controller, bool active) onlyOwner() public { 

wrapper\NameWrapper.sol
    117: function uri(uint256 tokenId) public view override returns (string memory) { 

[N-07] LOWER CASE LETTERS SHOULD NOT BE USED FOR CONSTANTS

As a best practice, only capital letters should be used for constants.

contracts\dnssec-oracle\BytesUtils.sol
    252: bytes constant base32HexTable = hex'00010203040506070809FFFFFFFFFFFFFF0A0B0C0D0E0F101112131415161718191A1B1C1D1E1FFFFFFFFFFFFFFFFFFFFF0A0B0C0D0E0F101112131415161718191A1B1C1D1E1F';  

registry\ReverseRegistrar.sol
    12: bytes32 constant lookup = 0x3031323334353637383961626364656600000000000000000000000000000000;  

[N-08] MISSING INDEXED EVENT FIELDS

Querying events can be optimized with index. Please consider adding index to fields of the following events.

dnssec-oracle\DNSSEC.sol
    14: event AlgorithmUpdated(uint8 id, address addr);  
    15: event DigestUpdated(uint8 id, address addr);

[N-09] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs.

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;  

algorithms\Algorithm.sol
    1: pragma solidity ^0.8.4;  

ethregistrar\ETHRegistrarController.sol
    1: pragma solidity >=0.8.4;  

ethregistrar\IETHRegistrarController.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\IReverseRegistrar.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\IMetadataService.sol
    1: pragma solidity >=0.8.4;  

wrapper\INameWrapper.sol
    2: pragma solidity ^0.8.4;  

wrapper\NameWrapper.sol
    2: pragma solidity ^0.8.4;

[N-10] INCOMPLETE NATSPEC COMMENTS

NatSpec provides rich documentation for code. @param and/or @return are missing for the following NatSpec functions.

dnssec-oracle\DNSSECImpl.sol
    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) {  

wrapper\ERC1155Fuse.sol
    33: function supportsInterface(bytes4 interfaceId)  
    53: function balanceOf(address account, uint256 id)  
    78: function balanceOfBatch(address[] memory accounts, uint256[] memory ids)  
    102: function setApprovalForAll(address operator, bool approved)  
    119: function isApprovedForAll(address account, address operator)  
    132: function getData(uint256 tokenId)  
    154: function _setData(  
    169: function safeTransferFrom(  
    188: function safeBatchTransferFrom(  

wrapper\NameWrapper.sol
    105: function setMetadataService(IMetadataService _newMetadataService)

[N-11] MISSING NATSPEC COMMENTS

NatSpec provides rich documentation for code. NatSpec comments are missing for the following contracts and functions.

dnssec-oracle\DNSSEC.sol
    5: abstract contract DNSSEC {   

dnssec-oracle\Owned.sol
    6: contract Owned {   

ethregistrar\ETHRegistrarController.sol
    17: contract ETHRegistrarController is Ownable, IETHRegistrarController {   

registry\ReverseRegistrar.sol
    51: function setDefaultResolver(address resolver) public override onlyOwner {
    181: function ownsContract(address addr) internal view returns (bool) {   

wrapper\Controllable.sol
    6: contract Controllable is Ownable {   

wrapper\ERC1155Fuse.sol
    25: function ownerOf(uint256 id) public view virtual returns (address) {   
    238: function _canTransfer(uint32 fuses) internal virtual returns (bool);   
    240: function _mint(   
    267: function _burn(uint256 tokenId) internal virtual {   
    274: function _transfer(   
    301: function _doSafeTransferAcceptanceCheck(   
    332: function _doSafeBatchTransferAcceptanceCheck(   

wrapper\NameWrapper.sol
    76: function supportsInterface(bytes4 interfaceId)   
    90: function ownerOf(uint256 id)   
    693: function onERC721Received(   
    729: function _canTransfer(uint32 fuses) internal pure override returns (bool) {   
    733: function _makeNode(bytes32 node, bytes32 labelhash)   
    741: function _addLabel(string memory label, bytes memory name)   
    755: function _mint(   
    771: function _wrap(   
    783: function _addLabelAndWrap(   
    795: function _prepareUpgrade(bytes32 node)   
    813: function _transferAndBurnFuses(   
    825: function _getDataAndNormaliseExpiry(   
    846: function _getETH2LDDataAndNormaliseExpiry(   
    867: function _normaliseExpiry(   
    885: function _wrapETH2LD(   
    918: function _unwrap(bytes32 node, address newOwner) private {   
    934: function _setFuses(   
    944: function _setData(   
    954: function _canFusesBeBurned(bytes32 node, uint32 fuses) internal pure {

[N-12] uint256 CAN BE USED INSTEAD OF uint

For explicitness and consistency, uint256 can be used instead uint.

dnssec-oracle\BytesUtils.sol
    11: function keccak(bytes memory self, uint offset, uint len) internal pure returns (bytes32 ret) {     
    44: function compare(bytes memory self, uint offset, uint len, bytes memory other, uint otheroffset, uint otherlen) internal pure returns (int) {     
    45: uint shortest = len;     
    49: uint selfptr;     
    50: uint otherptr;     
    56: for (uint idx = 0; idx < shortest; idx += 32) {
    57: uint a;     
    58: uint b;     
    65: uint mask;     
    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) {     
    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) {     
    207: function memcpy(uint dest, uint src, uint len) private pure {     
    219: uint mask = (256 ** (32 - len)) - 1;     
    234: function substring(bytes memory self, uint offset, uint len) internal pure returns(bytes memory) {     
    238: uint dest;     
    239: uint src;     
    261: function base32HexDecodeWord(bytes memory self, uint off, uint len) internal pure returns(bytes32) {     
    264: uint ret = 0;     
    266: for(uint i = 0; i < len; i++) {     
    277: uint bitlen = len * 5;     

dnssec-oracle\DNSSECImpl.sol
    26: uint constant DNSKEY_FLAG_ZONEKEY = 0x100;     
    28: error InvalidLabelCount(bytes name, uint labelsExpected);     
    93: for(uint i = 0; i < input.length; i++) {     

dnssec-oracle\RRUtils.sol
    19: function nameLength(bytes memory self, uint offset) internal pure returns(uint) {     
    20: uint idx = offset;     
    23: uint labelLen = self.readUint8(idx);     
    38: function readName(bytes memory self, uint offset) internal pure returns(bytes memory ret) {     
    39: uint len = nameLength(self, offset);     
    49: function labelCount(bytes memory self, uint offset) internal pure returns(uint) {     
    50: uint count = 0;     
    53: uint labelLen = self.readUint8(offset);     
    63: uint constant RRSIG_TYPE = 0;    
    64: uint constant RRSIG_ALGORITHM = 2;    
    65: uint constant RRSIG_LABELS = 3;    
    66: uint constant RRSIG_TTL = 4;    
    67: uint constant RRSIG_EXPIRATION = 8;    
    68: uint constant RRSIG_INCEPTION = 12;    
    69: uint constant RRSIG_KEY_TAG = 16;    
    70: uint constant RRSIG_SIGNER_NAME = 18;    
    106: uint offset;     
    110: uint rdataOffset;     
    111: uint nextOffset;     
    120: function iterateRRs(bytes memory self, uint offset) internal pure returns (RRIterator memory ret) {     
    146: uint off = iter.offset + nameLength(iter.data, iter.offset);     
    157: uint rdataLength = iter.data.readUint16(off);     
    181: uint constant DNSKEY_FLAGS = 0;    
    182: uint constant DNSKEY_PROTOCOL = 2;    
    183: uint constant DNSKEY_ALGORITHM = 3;    
    184: uint constant DNSKEY_PUBKEY = 4;    
    193: function readDNSKEY(bytes memory data, uint offset, uint length) internal pure returns(DNSKEY memory self) {     
    200: uint constant DS_KEY_TAG = 0;    
    201: uint constant DS_ALGORITHM = 2;    
    202: uint constant DS_DIGEST_TYPE = 3;    
    203: uint constant DS_DIGEST = 4;    
    212: function readDS(bytes memory data, uint offset, uint length) internal pure returns(DS memory self) {     
    224: uint off;     
    225: uint otheroff;     
    226: uint prevoff;     
    227: uint otherprevoff;     
    228: uint counts = labelCount(self, 0);     
    229: uint othercounts = labelCount(other, 0);     
    270: function progress(bytes memory body, uint off) internal pure returns(uint) {     
    308: uint ac1;     
    309: uint ac2;     
    310: for(uint i = 0; i < data.length + 31; i += 32) {
    311: uint word;     
    316: uint unused = 256 - (data.length - i) * 8;     

ethregistrar\StringUtils.sol
    11: uint len;     
    12: uint i = 0;     
    13: uint bytelength = bytes(s).length;     

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) {     
    26: (bytes32 labelhash, uint newOffset) = readLabel(self, offset);     
    41: function readLabel(bytes memory self, uint256 idx) internal pure returns (bytes32 labelhash, uint newIdx) {     
    43: uint len = uint(uint8(self[idx]));     

[G-01] REQUIRE STATEMENT CAN BE PLACED ABOVE OTHER MORE EXPENSIVE OPERATIONS IF POSSIBLE

When a require statement is placed above other operations that cost more gas, these operations are prevented from running if the require statement reverts.

For the following function, IPriceOracle.Price memory price = rentPrice(name, duration); and require(msg.value >= price.base, "ETHController: Not enough Ether provided for renewal"); can be placed above bytes32 label = keccak256(bytes(name)); because they do not use label.

ethregistrar\ETHRegistrarController.sol
    189-208:
        function renew(string calldata name, uint256 duration)
            external
            payable
            override
        {
            bytes32 label = keccak256(bytes(name));
            IPriceOracle.Price memory price = rentPrice(name, duration);
            require(
                msg.value >= price.base,
                "ETHController: Not enough Ether provided for renewal"
            );
            ...
        }

For the following function, if (parentNode == ETH_NODE) { revert IncompatibleParent(); } can be placed above bytes32 node = _makeNode(parentNode, labelhash); because it does not depend on node.

wrapper\NameWrapper.sol
    295-325:
        function wrap(
            bytes calldata name,
            address wrappedOwner,
            address resolver
        ) public override {
            (bytes32 labelhash, uint256 offset) = name.readLabel(0);
            bytes32 parentNode = name.namehash(offset);
            bytes32 node = _makeNode(parentNode, labelhash);

            if (parentNode == ETH_NODE) {
                revert IncompatibleParent();
            }
            ...
        }

For the following function, require(newOwner != address(0), "ERC1155: mint to the zero address"); and require(newOwner != address(this), "ERC1155: newOwner cannot be the NameWrapper contract"); can be placed above uint256 tokenId = uint256(node); because they only rely on newOwner to do input validation.

wrapper\ERC1155Fuse.sol
    240-265:
        function _mint(   
            bytes32 node,
            address newOwner,
            uint32 fuses,
            uint64 expiry
        ) internal virtual {
            uint256 tokenId = uint256(node);
            address owner = ownerOf(tokenId);
            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"
            );
            ...
        }

[G-02] canCallSetSubnodeOwner MODIFIER CAN BE REFACTORED INTO A FUNCTION FOR NOT REPEATING OPERATIONS

The canCallSetSubnodeOwner modifier can be refactored into a function that receives the parent node and the subnode and does not call the _makeNode function anymore. Afterwards, in the setSubnodeOwner and setSubnodeRecord functions, which no longer use the canCallSetSubnodeOwner modifier, the label hash is created and the _makeNode function is called to produce the subnode only once, and then the canCallSetSubnodeOwner function is called. This saves gas by not repeating the operations for creating the label hash and calling _makeNode.

The original code are shown below for reference.

wrapper\NameWrapper.sol
    504-526:
        function setSubnodeOwner(
            bytes32 parentNode,
            string calldata label,
            address newOwner,
            uint32 fuses,
            uint64 expiry
        )
            public
            onlyTokenOwner(parentNode)
            canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))
            returns (bytes32 node)
        {
            bytes32 labelhash = keccak256(bytes(label));
            node = _makeNode(parentNode, labelhash);
            ...
        }
    539-574:
        function setSubnodeRecord(
            bytes32 parentNode,
            string memory label,
            address newOwner,
            address resolver,
            uint64 ttl,
            uint32 fuses,
            uint64 expiry
        )
            public
            onlyTokenOwner(parentNode)
            canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))
        {
            bytes32 labelhash = keccak256(bytes(label));
            bytes32 node = _makeNode(parentNode, labelhash);
            ...
        }
    657-674:
        modifier canCallSetSubnodeOwner(bytes32 node, bytes32 labelhash) {
            bytes32 subnode = _makeNode(node, labelhash);
            address owner = ens.owner(subnode);
            ...
            _;
        }

[G-03] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

dnssec-oracle\BytesUtils.sol
    56: for (uint idx = 0; idx < shortest; idx += 32) {

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

ethregistrar\StringUtils.sol
    14: for(len = 0; i < bytelength; len++) {

[G-04] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, accounts.length in the following code can be cached outside of the loop like uint256 accountLen = accounts.length, and i < accountLen can be used for each iteration.

ethregistrar\ETHRegistrarController.sol
    256: for (uint256 i = 0; i < data.length; i++) {
	
wrapper\ERC1155Fuse.sol
    92: for (uint256 i = 0; i < accounts.length; ++i) {
    205: for (uint256 i = 0; i < ids.length; ++i) {

[G-05] ++VARIABLE AND --VARIABLE CAN BE USED INSTEAD OF VARIABLE++ AND VARIABLE--

++variable and --variable costs less gas than variable++ and variable--. For example, i++ can be changed to ++i in the following code.

dnssec-oracle\BytesUtils.sol
    266: for(uint i = 0; i < len; i++) {
    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
    235: counts--;
    241: othercounts--;

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

ethregistrar\StringUtils.sol
    14: for(len = 0; i < bytelength; len++) {

[G-06] X = X + Y AND X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y and x = x - y costs less gas than x += y and x -= y. For example, count += 1 can be changed to count = count + 1 in the following code.

dnssec-oracle\BytesUtils.sol
    56: for (uint idx = 0; idx < shortest; idx += 32) {
    75: selfptr += 32;
    76: otherptr += 32;
    209: for (; len >= 32; len -= 32) {
    213: dest += 32;
    214: src += 32;
    284: bitlen -= 2;
    288: bitlen -= 4;
    292: bitlen -= 1;
    296: bitlen -= 3;

dnssec-oracle\RRUtils.sol
    24: idx += labelLen + 1;
    54: offset += labelLen + 1;
    58: count += 1;
    150: off += 2;
    152: off += 2;
    154: off += 4;
    158: off += 2;
    250: counts -= 1;
    310: for(uint i = 0; i < data.length + 31; i += 32) {
    319: ac1 += (word & 0xFF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00) >> 8;
    320: ac2 += (word & 0x00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF00FF);
    333: ac1 += (ac1 >> 16) & 0xFFFF;
	
ethregistrar\StringUtils.sol
    17: i += 1;
    19: i += 2;
    21: i += 3;
    23: i += 4;
    25: i += 5;
    27: i += 6;

[G-07] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, if increasing the counter variable is very unlikely to overflow, then unchecked {++i} at the end of the loop block can be used, where i is the counter variable.

dnssec-oracle\BytesUtils.sol
    266: for(uint i = 0; i < len; i++) {
    313: for(uint256 idx = off; idx < off + len; idx++) {

dnssec-oracle\DNSSECImpl.sol
    93: for(uint i = 0; i < input.length; i++) {

ethregistrar\StringUtils.sol
    14: for(len = 0; i < bytelength; len++) { 

wrapper\ERC1155Fuse.sol
    92: for (uint256 i = 0; i < accounts.length; ++i) {  
    205: for (uint256 i = 0; i < ids.length; ++i) {

[G-08] REDUNDANT TYPE CONVERSION CAN BE AVOIDED

Converting a variable type when not needed costs more unnecessary gas. Converting resolver to an address requires extra operation and more gas for the following code.

registry\ReverseRegistrar.sol
    51-57:
        function setDefaultResolver(address resolver) public override onlyOwner {
            require(
                address(resolver) != address(0),
                "ReverseRegistrar: Resolver address must not be 0"
            );
            defaultResolver = NameResolver(resolver);
        }

[G-09] STATE VARIABLE WHOSE VALUE IS ONLY ASSIGNED IN CONSTRUCTOR CAN BE IMMUTABLE

Providing an immutable keyword to the state variable whose value is only assigned in the constructor can help save gas. anchors can be declared as an immutable in the following code.

dnssec-oracle\DNSSEC.sol
    5-7:
        abstract contract DNSSEC {
		
          bytes public anchors;

[G-10] CONSTANTS CAN BE PRIVATE IF POSSIBLE

For constants that are used only inside the contract, they can be private instead of public to cost less gas. MIN_REGISTRATION_DURATION in the following code can be private since it is only used in the same contract.

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

[G-11] VISIBILITIES CAN BE SET TO EXTERNAL FOR FUNCTIONS THAT ARE NOT CALLED BY OWN CONTRACTS

Calling an external function is cheaper than that for a public function. For the following functions, their visibilities can be set to external because their contracts do not call them.

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

ethregistrar\ETHRegistrarController.sol
    120: function commit(bytes32 commitment) public override { 
    210: function withdraw() public { 

registry\ReverseRegistrar.sol
    51: function setDefaultResolver(address resolver) public override onlyOwner { 
    65: function claim(address owner) public override returns (bytes32) { 
    148: function node(address addr) public pure override returns (bytes32) { 

wrapper\Controllable.sol
    11: function setController(address controller, bool active) onlyOwner() public { 

wrapper\NameWrapper.sol
    117: function uri(uint256 tokenId) public view override returns (string memory) { 

[G-12] UNUSED NAMED RETURNS COSTS UNNECESSARY DEPLOYMENT GAS

When a function has unused named returns, unnecessary deployment gas is burned. Please consider removing the following named return variables.

dnssec-oracle\BytesUtils.sol
    135: function readUint8(bytes memory self, uint idx) internal pure returns (uint8 ret) {

wrapper\NameWrapper.sol
    94: returns (address owner)
    744: returns (bytes memory ret)

[G-13] REVERT REASON STRINGS CAN BE SHORTENED TO FIT IN 32 BYTES

Revert reason strings that are longer than 32 bytes need more memory space and more mstore operation(s) than these that are shorter than 32 bytes when reverting. Please consider shortening the following revert reason strings.

ethregistrar\ETHRegistrarController.sol
    101: "ETHRegistrarController: resolver is required when data is supplied"
    139: "ETHRegistrarController: Not enough ether provided"
    242: require(available(name), "ETHRegistrarController: Name is unavailable"); 
    261: "ETHRegistrarController: Namehash on record do not match the name being registered"

registry\ReverseRegistrar.sol
    46: "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"
    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"     
    87: "ERC1155: accounts and ids length mismatch"     
    109: "ERC1155: setting approval status for self"     
    179: "ERC1155: caller is not owner nor approved"     
    199: require(to != address(0), "ERC1155: transfer to the zero address");
    202: "ERC1155: transfer caller is not owner nor approved"     
    217: "ERC1155: insufficient balance for transfer"     
    249: require(newOwner != address(0), "ERC1155: mint to the zero address");
    252: "ERC1155: newOwner cannot be the NameWrapper contract"     
    292: "ERC1155: insufficient balance for transfer"     
    322: revert("ERC1155: ERC1155Receiver rejected tokens");     
    327: revert("ERC1155: transfer to non ERC1155Receiver implementer");     
    354: revert("ERC1155: ERC1155Receiver rejected tokens");     
    359: revert("ERC1155: transfer to non ERC1155Receiver implementer");

[G-14] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE WITH REVERT REASON STRING

revert with custom error can cost less gas than require with revert reason string. Please consider using revert with custom error to replace the following require.

dnssec-oracle\BytesUtils.sol
    12: require(offset + len <= self.length);    
    146: require(idx + 2 <= self.length);    
    159: require(idx + 4 <= self.length);    
    172: require(idx + 32 <= self.length);    
    185: require(idx + 20 <= self.length);    
    199: require(len <= 32);    
    200: require(idx + len <= self.length);    
    235: require(offset + len <= self.length);    
    262: require(len <= 52);    
    268: require(char >= 0x30 && char <= 0x7A);    
    270: require(decoded <= 0x20);    

dnssec-oracle\Owned.sol
    10: require(msg.sender == owner);    

dnssec-oracle\RRUtils.sol
    307: require(data.length <= 8192, "Long keys not permitted");     

ethregistrar\ETHRegistrarController.sol
    57: require(_maxCommitmentAge > _minCommitmentAge);    
    99: require(     
    121: require(commitments[commitment] + maxCommitmentAge < block.timestamp);    
    137: require(     
    196: require(     
    232: require(     
    238: require(     
    242: require(available(name), "ETHRegistrarController: Name is unavailable"); 
    246: require(duration >= MIN_REGISTRATION_DURATION);    
    259: require(     

registry\ReverseRegistrar.sol
    41: require(     
    52: require(     

wrapper\BytesUtil.sol
    13: require(offset + len <= self.length);    
    28: require(offset == self.length - 1, "namehash: Junk at end of name");     
    42: require(idx < self.length, "readLabel: Index out of bounds");     

wrapper\Controllable.sol
    17: require(controllers[msg.sender], "Controllable: Caller is not a controller");

wrapper\ERC1155Fuse.sol
    60: require(     
    85: require(     
    107: require(     
    176: require(to != address(0), "ERC1155: transfer to the zero address");     
    177: require(     
    195: require(     
    199: require(to != address(0), "ERC1155: transfer to the zero address"); 
    200: require(     
    215: require(     
    248: require(owner == address(0), "ERC1155: mint of existing token");     
    249: require(newOwner != address(0), "ERC1155: mint to the zero address");
    250: require(     
    290: require(

[G-15] NEWER VERSION OF SOLIDITY CAN BE USED

The protocol can benefit from more gas-efficient features and fixes by using a newer version of Solidity. Changes for newer Solidity versions can be viewed here.

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;  

algorithms\Algorithm.sol
    1: pragma solidity ^0.8.4;  

ethregistrar\ETHRegistrarController.sol
    1: pragma solidity >=0.8.4;  

ethregistrar\IETHRegistrarController.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\IReverseRegistrar.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\IMetadataService.sol
    1: pragma solidity >=0.8.4;  

wrapper\INameWrapper.sol
    2: pragma solidity ^0.8.4;  

wrapper\NameWrapper.sol
    2: pragma solidity ^0.8.4;