ENS contest - dxdv's results

Findings for ENS Contest

Commit: N/A Type of Audit: Security Review Type of Project: DeFi Language: Solidity Methods: Manual review, automated test suite

---

1. Contract Owned.sol

[LOW] Missing validation logic to check zero address in setOwner function

File: L18 Description: No validation logic exists to check that the newOwner argument passed into setOwner function is not a zero address`

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

Recommendation: Consider adding validation logic to ensure that the passed in newOwner argument is not a zero address.

 

[LOW] Missing emitted event in state-changing setOwer function

File: L18

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

Description: State-changing setOwner function does not emit any event.

Recommendation: Consider creating and emitting appropriate event for setOwner function

 

[LOW] Missing NatSpec annotations in setOwner function

File: L18

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

Description: setOwner function is not commented in line with NatSpec standard. Recommendation: Consider commenting setOwner function to provide clear anotations of what the function is designed to do

[LOW] Missing require statement's error message in owner_only modifier

File: L10

modifier owner_only() {
    require(msg.sender == owner);
    _;
}

Description: require statement in owner_only modifier lacks a descriptive error message Recommendation: Consider adding short descriptive error message in owner_only require statement

 

2. Contract ETHRegistrarController.sol

[LOW] Use of transfer function call for Ether transfer

File: L204

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");

    uint256 expires = base.renew(uint256(label), duration);

    if (msg.value > price.base) {
        payable(msg.sender).transfer(msg.value - price.base);
    }

    emit NameRenewed(name, label, msg.value, expires);
}

Description: transfer function is no longer recommended for sending Ether

Recommendation: Use call instead of tranfer

 if (msg.value > price.base) {
    (bool sent,) = payable(msg.sender).call{value: msg.value - price.base}("");
    require(sent, "Failed to send Ether");
}

[LOW] Missing emitted event emission for state-changing withdraw function

File: L210

function withdraw() public {
    payable(owner()).transfer(address(this).balance);
}

Description: State-changing withdraw function does not emit any event.

Recommendation: Consider creating and emitting appropriate event for withdraw function

 

[LOW] No event emitted in state-changing commit function

File: L120

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

Description: State-changing commit function does not emit any event.

Recommendation: Consider creating and emitting appropriate event for commit function

 

[LOW] Missing NatSpec annotations in ETHRegistrarController.sol contract

File: L210

Description: No NatSpec format comment missing in constructor, rentPrice, valid, available, makeCommitment commit, register, renew, withdraw, _consumeCommitment, _setRecords and_setReverseRecord functions Recommendation: Consider adding appropriate NatSpec comments to constructor, rentPrice, valid, available, makeCommitment commit, register, renew, withdraw, _consumeCommitment, _setRecords and _setReverseRecord functions in order to provide clear anotations of what the function is designed to do

 

[LOW] Use of IPriceOracle without direct import of this interface

File: L26

IPriceOracle public immutable prices;

Description: Whereas IPriceOracle's immutable reference in ETHRegistrarController.sol contract is prices which is used in constructor and rentPrice function, IPriceOracle is never directly imported in this contract.

Recommendation: Consider importing IPriceOracle interface in ETHRegistrarController.sol contract

 

3. Contract DummyOracle.sol

[LOW] No emitted event in set function

File: L14

function set(int _value) public {
    value = _value;
}

Description: No event is emitted for state-changing set function. This could result in invalid return adverse effect on clients listening for event emissions on

Recommendation: Consider creating and emitting appropriate event for set function

 

4. Contract NameWrapper.sol

[LOW] Missing validation logic to ensure that the passed in _newMetadataService argument accurately matches the targeted StaticMetadataService contract address

File: L14

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

Description: Given that no validation logic exists to check that the passed in _newMetadataService argument accurately matches the targeted address of StaticMetadataService contract, an unintended address could be passed in thus resulting in flawed reference to the targeted StaticMetadataService contract

Recommendation: Consider adding validation logic to ensure that the passed in _newMetadataService argument accurately matches the targeted address of StaticMetadataService contract

function setMetadataService(IMetadataService _newMetadataService) public onlyOwner 
{
    require(address(MetadataService) == _newMetadataService, "MetadaService mismatch");
}

 

5. Contract BaseRegistrarImplementation.sol

[LOW] Missing error message for require statement checks in live and onlyController modifiers as well as in ownerOf, renew and reclaim functions

Files: L52 L57 L68 L140 L141 L152

modifier live {
    require(ens.owner(baseNode) == address(this));
    _;
}

modifier onlyController {
    require(controllers[msg.sender]);
    _;
}

function ownerOf(uint256 tokenId) public view override(IERC721, ERC721) returns (address) {
    require(expiries[tokenId] > block.timestamp);
    return super.ownerOf(tokenId);
}

function renew(uint256 id, uint duration) external override live onlyController returns(uint) {
    require(expiries[id] + GRACE_PERIOD >= block.timestamp); // Name must be registered here or in grace period 
    require(expiries[id] + duration + GRACE_PERIOD > duration + GRACE_PERIOD); // Prevent future overflow
    expiries[id] += duration;
    emit NameRenewed(id, expiries[id]);
    return expiries[id];
}

function reclaim(uint256 id, address owner) external override live {
    require(_isApprovedOrOwner(msg.sender, id));
    ens.setSubnodeOwner(baseNode, bytes32(id), owner);
}

Description: require statement in owner_only modifier lacks a descriptive error message which would makes transaction revert in an unclear manner

Recommendation: Consider adding short descriptive appropriate error message in owner_only

function setMetadataService(IMetadataService _newMetadataService) public onlyOwner  {
    require(address(MetadataService) == _newMetadataService, "MetadaService mismatch");
}

 

[LOW] Inconsistent use of uint as uint256

Files: L10 L90 L105 L115 L119 L138

Description: Although uint can be used to express uint256, it is used inconsistently in expiries mapping, nameExpires, register, registerOnly, _register and renew functions. Recommendation: Consider using uint256 to ensure that this datatype is used consistently throughout the entire code base.

 

6. Contract ENS.sol

[LOW] Unconventional naming of ENS as Interface

File: L3

interface ENS {...}

Description: While ENS is an interface, it is named like a contract. This could be misleading. Recommendation: Consider modifying ENS interface name to IENS so that it is consistent with the interface naming convention used in the codebase and complies with industry standards

 

7. Contract BytesUtils.sol

[LOW] Uninitialized local variable decoded in base32HexDecodeWord function

File: L265

Description: decoded local variable is never initialized in base32HexDecodeWord function Recommendation: Add appropriate value to initialize decoded as a local variable in base32HexDecodeWord function

 

8. Contract Algorithm.sol

[LOW] verify function marked as virtual

File: L14

function verify(bytes calldata key, bytes calldata data, bytes calldata signature) external virtual view returns (bool);

Description: Given that interface functions are implicitly virtual, there is no need to mark verify function as virtual Recommendation: Remove virtual keyword in verify function

 

[LOW] Typo representation of if as iff

File: L12

* @return True iff the signature is valid.

Description: typo iff detected in @return Natspec doxygen comment tag for verify function Recommendation: Correctly use if to replace iff

 

9. Contract Resolver.sol

[LOW] Unconventional naming of Resolver as Interface

File: L20

interface  {...}

Description: While Resolver is an interface, it is named like a contract. This could be misleading. Recommendation: Consider modifying Resolver interface name to IResolver so that it is consistent with the interface naming convention used in the codebase and complies with industry standards