ENS contest - sashik_eth's results

L01 - Lack of event emitting after sensitive action

Contracts do not emit relevant event after setting sensitive variable:

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

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L51-L57

N01 - Missing SPDX identifier

contracts/dnssec-oracle/BytesUtils.sol:1    pragma solidity ^0.8.4; 
contracts/dnssec-oracle/Owned.sol:1 pragma solidity ^0.8.4; 
contracts/dnssec-oracle/RRUtils.sol:1   pragma solidity ^0.8.4; 
contracts/dnssec-oracle/SHA1.sol:1  pragma solidity >=0.8.4; 
contracts/dnssec-oracle/algorithms/Algorithm.sol:1  pragma solidity ^0.8.4;
contracts/ethregistrar/ETHRegistrarController.sol:1 pragma solidity >=0.8.4; 
contracts/ethregistrar/IETHRegistrarController.sol:1    pragma solidity >=0.8.4; 
contracts/registry/ENS.sol:1    pragma solidity >=0.8.4; 
contracts/registry/IReverseRegistrar.sol:1  pragma solidity >=0.8.4; 
contracts/wrapper/IMetadataService.sol:1    pragma solidity >=0.8.4; 

N02 - Floating pragma is used in all contracts

The contracts should be deployed with the same solidity version with which they have been tested. Locking the pragma ensures that the contracts don't get deployed with an older version which might introduce bugs.

N03 - Deprecated code

Due to solidity doc: the pragma pragma experimental ABIEncoderV2; is still valid, but it is deprecated and has no effect. https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html#silent-changes-of-the-semantics

contracts/dnssec-oracle/DNSSEC.sol:3    pragma experimental ABIEncoderV2; 
contracts/dnssec-oracle/DNSSECImpl.sol:3    pragma experimental ABIEncoderV2;

N04 - No need to specify constructor visibility

contracts/dnssec-oracle/Owned.sol:14    constructor() public {

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L14

N05 - No need to specify interface function visibility

contracts/dnssec-oracle/digests/Digest.sol:13   function verify(bytes calldata data, bytes calldata hash) external virtual pure returns (bool); 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/digests/Digest.sol#L13

N06 - Typo

contracts/wrapper/NameWrapper.sol:534  * @param ttl ttl in the regsitry 

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

N07 - Code Style

Modifier name should be in mixedCase: https://docs.soliditylang.org/en/latest/style-guide.html#modifier-names

contracts/dnssec-oracle/Owned.sol:9 modifier owner_only() { 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/Owned.sol#L9

G01 - unchecked block can be used for gas efficiency of the expression that can't overflow/underflow

Could be unchecked since len > 0, otherwise loop would not start:

contracts/dnssec-oracle/BytesUtils.sol:271  if(i == len - 1) { 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L271

Could be unchecked because due to function logic always bitlen < 256 :

contracts/dnssec-oracle/BytesUtils.sol:301  return bytes32(ret << (256 - bitlen)); 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L301

Could be unchecked due to statement logic, if rrset.signerName.length > name.length - second equation would not be executed:

contracts/dnssec-oracle/DNSSECImpl.sol:186        if(rrset.signerName.length > name.length
contracts/dnssec-oracle/DNSSECImpl.sol:187            || !rrset.signerName.equals(0, name, name.length - rrset.signerName.length)) 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L187

Could be unchecked since always offset < idx due to function logic:

contracts/dnssec-oracle/RRUtils.sol:29  return idx - offset;

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L29

Could be unchecked since count would not overflow due to loop logic:

contracts/dnssec-oracle/RRUtils.sol:58  count += 1;

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L58

Could be unchecked since counts would not overflow due to loop logic:

contracts/dnssec-oracle/RRUtils.sol:235 counts--; 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L235

Could be unchecked since othercounts would not overflow due to loop logic:

contracts/dnssec-oracle/RRUtils.sol:241 othercounts--; 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L241

Could be unchecked since counts would not overflow due to loop logic:

contracts/dnssec-oracle/RRUtils.sol:250 counts -= 1; 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L250

Could be unchecked due to check on L182:

contracts/ethregistrar/ETHRegistrarController.sol:184   msg.value - (price.base + price.premium) 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L184

Could be unchecked due to check on L203:

contracts/ethregistrar/ETHRegistrarController.sol:204   payable(msg.sender).transfer(msg.value - price.base); 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L204

Counter could be unchecked since it can't overflow:

contracts/dnssec-oracle/BytesUtils.sol:266  for(uint i = 0; i < len; i++) { 
contracts/dnssec-oracle/BytesUtils.sol:313  for(uint256 idx = off; idx < off + len; idx++) { 
contracts/dnssec-oracle/DNSSECImpl.sol:93   for(uint i = 0; i < input.length; i++) { 
contracts/dnssec-oracle/RRUtils.sol:235 counts--;
contracts/dnssec-oracle/RRUtils.sol:241 othercounts--; 
contracts/ethregistrar/ETHRegistrarController.sol:256   for (uint256 i = 0; i < data.length; i++) { 
contracts/ethregistrar/StringUtils.sol:14   for(len = 0; i < bytelength; len++) {  
contracts/wrapper/ERC1155Fuse.sol:92    for (uint256 i = 0; i < accounts.length; ++i) { 
contracts/wrapper/ERC1155Fuse.sol:205   for (uint256 i = 0; i < ids.length; ++i) { 

G02 - Using prefix increment

Prefix increment/decrement is cheaper than postfix increment/decrement. Consider using ++i/--i in next lines:

contracts/dnssec-oracle/BytesUtils.sol:265  for(uint i = 0; i < len; i++) { 
contracts/dnssec-oracle/BytesUtils.sol:312  for(uint256 idx = off; idx < off + len; idx++) { 
contracts/dnssec-oracle/DNSSECImpl.sol:93   for(uint i = 0; i < input.length; i++) { 
contracts/dnssec-oracle/RRUtils.sol:235 counts--;
contracts/dnssec-oracle/RRUtils.sol:241 othercounts--; 
contracts/ethregistrar/ETHRegistrarController.sol:256   for (uint256 i = 0; i < data.length; i++) { 
contracts/ethregistrar/StringUtils.sol:14   for(len = 0; i < bytelength; len++) {  

G03 - Avoid long revert strings

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Next revert strings are longer than 32 bytes:

contracts/ethregistrar/ETHRegistrarController.sol:101   "ETHRegistrarController: resolver is required when data is supplied" 
contracts/ethregistrar/ETHRegistrarController.sol:139   "ETHRegistrarController: Not enough ether provided" 
contracts/ethregistrar/ETHRegistrarController.sol:198   "ETHController: Not enough Ether provided for renewal" 
contracts/ethregistrar/ETHRegistrarController.sol:234   "ETHRegistrarController: Commitment is not valid"
contracts/ethregistrar/ETHRegistrarController.sol:240   "ETHRegistrarController: Commitment has expired" 
contracts/ethregistrar/ETHRegistrarController.sol:242   require(available(name), "ETHRegistrarController: Name is unavailable"); 
contracts/ethregistrar/ETHRegistrarController.sol:261   "ETHRegistrarController: Namehash on record do not match the name being registered" /

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L101

contracts/registry/ReverseRegistrar.sol:46  "ReverseRegistrar: Caller is not a controller or authorised by address or the address itself"  
contracts/registry/ReverseRegistrar.sol:54  "ReverseRegistrar: Resolver address must not be 0" 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/registry/ReverseRegistrar.sol#L46

contracts/wrapper/ERC1155Fuse.sol:62    "ERC1155: balance query for the zero address" 
contracts/wrapper/ERC1155Fuse.sol:87    "ERC1155: accounts and ids length mismatch" 
contracts/wrapper/ERC1155Fuse.sol:109   "ERC1155: setting approval status for self" 

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/ERC1155Fuse.sol#L62