QA Report

[code423n4]

Overview Risk Rating	Number of issues
Low Risk	7
Non-Critical Risk	9

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;

[IllIllI000]

_mint() is the safe version - there is no other variant for ERC1155