QA Report

[code423n4]

[L-01] require() should be used instead of assert():-

  1. File: 2022-07-ens/contracts/dnssec-oracle/RRUtils.sol (line 22):

      `assert(idx < self.length);`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/RRUtils.sol (line 52):

      `assert(offset < self.length);`

[L-02] SPDX license identifiers missing:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol

            

  2. File: 2022-07-ens/contracts/dnssec-oracle/RRUtils.sol

      

  3. File: 2022-07-ens/contracts/dnssec-oracle/SHA1.sol



  4. File: 2022-07-ens/contracts/dnssec-oracle/Owned.sol

            

  5. File: 2022-07-ens/contracts/dnssec-oracle/algorithms/Algorithm.sol

      

  6. File: 2022-07-ens/contracts/dnssec-oracle/digests/Digest.sol



  7. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol

            

  8. File: 2022-07-ens/contracts/ethregistrar/IETHRegistrarController.sol

      

  9. File: 2022-07-ens/contracts/registry/ReverseRegistrar.sol



  10. File: 2022-07-ens/contracts/registry/IReverseRegistrar.sol

            

  11. File: 2022-07-ens/contracts/wrapper/IMetadataService.sol

      

  12. File: 2022-07-ens/contracts/registry/ENS.sol      

[L-03] Missing checks for address(0x0) when assigning values to address state variables:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/Owned.sol (line 19):

      `owner = newOwner;`          

  2. File: 2022-07-ens/contracts/wrapper/Controllable.sol (line 12):

      `controllers[controller] = active;`      

[L-04] MOpen TODOS (Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment):-

  1. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 238):

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

  2. File: 2022-07-ens/contracts/wrapper/Controllable.sol (line 12):

      `controllers[controller] = active;`      

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256():-

  1. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 255):

      `bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label));`           

[L-06] address.call{value:x}() should be used instead of payable.transfer():-

  1. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 183-184):

      `payable(msg.sender).transfer(
            msg.value - (price.base + price.premium)`          

  2. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 204):

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

[L-07] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions:-

  1. File: 22022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 17):

      `contract ETHRegistrarController is Ownable, IETHRegistrarController {`          

  2. File: 2022-07-ens/contracts/registry/ReverseRegistrar.sol (line 18):

      `contract ReverseRegistrar is Ownable, Controllable, IReverseRegistrar {`            

  3. File: 2022-07-ens/contracts/wrapper/ERC1155Fuse.sol (line 14):

      `abstract contract ERC1155Fuse is ERC165, IERC1155, IERC1155MetadataURI {`          

  4. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 27-33):

      `contract NameWrapper is
  Ownable,
  ERC1155Fuse,
  INameWrapper,
  Controllable,
  IERC721Receiver
    {`  

  5. File: 2022-07-ens/contracts/wrapper/Controllable.sol (line 6):

      `contract Controllable is Ownable {`          

[N-01] Adding a return statement when the function defines a named return variable, is redundant:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 247):

      `return ret;`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 97):

      `return proof;`            

[N-02] require()/revert() statements should have descriptive reason strings:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 12):

      `require(offset + len <= self.length);`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 146):

      `require(idx + 2 <= self.length);`            

  3. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 159):

      `require(idx + 4 <= self.length);`          

  4. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 172):

      `require(idx + 32 <= self.length);`  

  5. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 185):

      `require(idx + 20 <= self.length);`

  6. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 199):

      `require(len <= 32);`          

  7. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 200):

      `require(idx + len <= self.length);`            

  8. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 235):

      `require(offset + len <= self.length);`          

  9. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 262):

      `require(len <= 52);`  

  10. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 268):

      `require(char >= 0x30 && char <= 0x7A);`          

  11. File: 2022-07-ens/contracts/dnssec-oracle/BytesUtils.sol (line 270):

      ` require(decoded <= 0x20)`          

  12. File: 2022-07-ens/contracts/dnssec-oracle/Owned.sol (line 10):

      `require(msg.sender == owner);`            

  13. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 57):

      `require(_maxCommitmentAge > _minCommitmentAge);`          

  14. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 121):

      `require(commitments[commitment] + maxCommitmentAge < block.timestamp);`  

  15. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 246):

      `require(duration >= MIN_REGISTRATION_DURATION);`

  16. File: 2022-07-ens/contracts/wrapper/BytesUtil.sol (line 13):

      `require(offset + len <= self.length);`          

[N-03] Use a more recent version of solidity (Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)):-

  1. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 1):

      `pragma solidity >=0.8.4;`          

  2. File: 2022-07-ens/contracts/registry/ReverseRegistrar.sol (line 1):

      `pragma solidity >=0.8.4;`            

  3. File: 2022-07-ens/contracts/wrapper/BytesUtil.sol (line 2):

      `pragma solidity >=0.8.4;`          

  4. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol(line 2):

      `pragma solidity >=0.8.4;`  

[N-04] Event is missing indexed fields (Each event should use three indexed fields if there are three or more fields):-

  1. File: 2022-07-ens/contracts/dnssec-oracle/DNSSEC.sol (line 14-15):

      ` event AlgorithmUpdated(uint8 id, address addr);
event DigestUpdated(uint8 id, address addr);`          

  2. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 34-47):

      `event NameRegistered(
    string name,
    bytes32 indexed label,
    address indexed owner,
    uint256 baseCost,
    uint256 premium,
    uint256 expires
);
event NameRenewed(
    string name,
    bytes32 indexed label,
    uint256 cost,
    uint256 expires
);`            

  3. File: 2022-07-ens/contracts/ethregistrar/IBaseRegistrar.sol (line 9-19):

      `event NameMigrated(
    uint256 indexed id,
    address indexed owner,
    uint256 expires
);
event NameRegistered(
    uint256 indexed id,
    address indexed owner,
    uint256 expires
);
event NameRenewed(uint256 indexed id, uint256 expires);`          

  4. File: 2022-07-ens/contracts/wrapper/INameWrapper.sol(line 19-29):

      `event NameWrapped(
    bytes32 indexed node,
    bytes name,
    address owner,
    uint32 fuses,
    uint64 expiry
);

event NameUnwrapped(bytes32 indexed node, address owner);

event FusesSet(bytes32 indexed node, uint32 fuses, uint64 expiry);`            

  3. File: 2022-07-ens/contracts/registry/ENS.sol (line 5):

      `event NewOwner(bytes32 indexed node, bytes32 indexed label, address owner);`          

  4. File: 2022-07-ens/contracts/registry/ENS.sol (line 17-21):

      `event ApprovalForAll(
    address indexed owner,
    address indexed operator,
    bool approved
);`

[N-05] Use a more recent version of solidity (Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions):-

  1. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 2):

      `pragma solidity >=0.8.4;`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/RRUtils.sol (line 1):

      `pragma solidity >=0.8.4;`            

  3. File: 2022-07-ens/contracts/ethregistrar/ETHRegistrarController.sol (line 1):

      `pragma solidity >=0.8.4;`          

  4. File: 2022-07-ens/contracts/wrapper/ERC1155Fuse.sol (line 2):

      `pragma solidity >=0.8.4;`     

  5. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 2):

      `pragma solidity >=0.8.4;`                     

[N-06] public functions not called by the contract should be declared external instead:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 58-61):

      `function setAlgorithm(uint8 id, Algorithm algo) public owner_only {
    algorithms[id] = algo;
    emit AlgorithmUpdated(id, address(algo));
}`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 69-72):

      ` function setDigest(uint8 id, Digest digest) public owner_only {
    digests[id] = digest;
    emit DigestUpdated(id, address(digest));
}`            

  3. File: 2022-07-ens/contracts/dnssec-oracle/Owned.sol (line 18-20):

      `function setOwner(address newOwner) public owner_only {
    owner = newOwner;
}`          

  4. File: 2022-07-ens/contracts/registry/ReverseRegistrar.sol (line 51-57):

      `function setDefaultResolver(address resolver) public override onlyOwner {
    require(
        address(resolver) != address(0),
        "ReverseRegistrar: Resolver address must not be 0"
    );
    defaultResolver = NameResolver(resolver);
}`     

  5. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 105-110):

      `function setMetadataService(IMetadataService _newMetadataService)
    public
    onlyOwner
{
    metadataService = _newMetadataService;
}`                               

  6. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 128-131):

      `function setUpgradeContract(INameWrapperUpgrade _upgradeAddress)
    public
    onlyOwner
{`          

  7. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 335-339):

      `function unwrapETH2LD(
    bytes32 labelhash,
    address newRegistrant,
    address newController
) public override onlyTokenOwner(_makeNode(ETH_NODE, labelhash)) {`            

  8. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 356-360):

      `function unwrap(
    bytes32 parentNode,
    bytes32 labelhash,
    address newController
) public override onlyTokenOwner(_makeNode(parentNode, labelhash)) {`          

  9. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 373-378):

      `function setFuses(bytes32 node, uint32 fuses)
    public
    onlyTokenOwner(node)
    operationAllowed(node, CANNOT_BURN_FUSES)
    returns (uint32)
{`     

  10. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 504-515):

      `function setSubnodeOwner(
    bytes32 parentNode,
    string calldata label,
    address newOwner,
    uint32 fuses,
    uint64 expiry
)
    public
    onlyTokenOwner(parentNode)
    canCallSetSubnodeOwner(parentNode, keccak256(bytes(label)))
    returns (bytes32 node)
{`       

  11. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 584-631):

      `    function setRecord(
    bytes32 node,
    address owner,
    address resolver,
    uint64 ttl
)
    public
    override
    onlyTokenOwner(node)
    operationAllowed(
        node,
        CANNOT_TRANSFER | CANNOT_SET_RESOLVER | CANNOT_SET_TTL
    )
{
    ens.setRecord(node, address(this), resolver, ttl);
    (address oldOwner, , ) = getData(uint256(node));
    _transfer(oldOwner, owner, uint256(node), 1, "");
}

/**
 * @notice Sets resolver contract in the registry
 * @param node namehash of the name
 * @param resolver the resolver contract
 */

function setResolver(bytes32 node, address resolver)
    public
    override
    onlyTokenOwner(node)
    operationAllowed(node, CANNOT_SET_RESOLVER)
{
    ens.setResolver(node, resolver);
}

/**
 * @notice Sets TTL in the registry
 * @param node namehash of the name
 * @param ttl TTL in the registry
 */

function setTTL(bytes32 node, uint64 ttl)
    public
    override
    onlyTokenOwner(node)
    operationAllowed(node, CANNOT_SET_TTL)
{
    ens.setTTL(node, ttl);
}`     

[N-07] Unused file:-

  1. File: 2022-07-ens/contracts/dnssec-oracle/DNSSECImpl.sol (line 1):

      `// SPDX-License-Identifier: MIT`          

  2. File: 2022-07-ens/contracts/dnssec-oracle/DNSSEC.sol (line 1):

      `// SPDX-License-Identifier: MIT`            

  3. File: 2022-07-ens/contracts/wrapper/BytesUtil.sol (line 1):

      `// SPDX-License-Identifier: MIT`          

  4. File: 2022-07-ens/contracts/wrapper/ERC1155Fuse.sol (line 1):

      `// SPDX-License-Identifier: MIT`     

  5. File: 2022-07-ens/contracts/wrapper/NameWrapper.sol (line 1):

      `// SPDX-License-Identifier: MIT`     

  6. File: 2022-07-ens/contracts/wrapper/INameWrapper.sol (line 1):

      `// SPDX-License-Identifier: MIT`          

  7. File: 2022-07-ens/contracts/wrapper/Controllable.sol (line 1):

      `// SPDX-License-Identifier: MIT`            

  8. File: 2022-07-ens/contracts/wrapper/INameWrapperUpgrade.sol (line 1):

      `// SPDX-License-Identifier: MIT`          

  9. File: 2022-07-ens/contracts/resolvers/Resolver.sol (line 1):