ENS contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/dnssec-oracle/DNSSECImpl.sol#L126 https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/dnssec-oracle/RRUtils.sol#L267

Vulnerability details

Impact

RRUtils.serialNumberGte casts the passed uint32 to an int32 before doing the calculation. This cast can overflow, resulting in negative values, which invalidates the calculation and results in RRSet that are valid but cannot be validated.

Proof Of Concept

An RRSet has an expiration timestamp of 2147483648 (Tue Jan 19 2038 03:14:08 GMT+0000). On Jan 18 2038 (timestamp 2147382000), the RRSet is validated. Inside serialNumberGte, we have:

int32(2147483648) - int32(2147382000) = -2147483648 - 2147382000

This causes a reversion.

Recommended Mitigation Steps

Although it is stated, the implementation does not actually implement RFC1982, where comparison is defined as:

s1 is said to be less than s2 if, and only if, s1 is not equal to s2,
   and

        (i1 < i2 and i2 - i1 < 2^(SERIAL_BITS - 1)) or
        (i1 > i2 and i1 - i2 > 2^(SERIAL_BITS - 1))

Furthermore, it is stated that "Arithmetic and comparisons applied to i1 and i2 use ordinary unbounded integer arithmetic." which of course is not the case for Soildity.

So you could either actually implement RFC1982 (incorporating bounded arithmetic in Solidity) or use a normal comparison.

• According to the registrar, 2 and 1 letter domains are not valid, but there is a price for them in the StablePriceOracle (https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/ethregistrar/StablePriceOracle.sol#L54), which seems unnecessary.
• In the specification for setNameForAddr (https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/registry/ReverseRegistrar.sol#L124), it is mentioned that "First updates the resolver to the default reverse resolver if necessary.". However, this is not done, the user provided resolver address is used.
• In safeBatchTransferFrom (https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/wrapper/ERC1155Fuse.sol#L209), if oldOwner == to, the iteration could be skipped (like it is done for safeTransferFrom).
• The comment for canCallSetSubnodeOwner (https://github.com/code-423n4/2022-07-ens/blob/1772ac3d16c8e84bb9c1e02a0c4e321cd6f5652b/contracts/wrapper/NameWrapper.sol#L649) seems to be out of date, canReplaceSubdomain does not exist / is not checked.

• The iterations in the following loops can be marked unchecked because no overflow is possible:
  • https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/dnssec-oracle/DNSSECImpl.sol#L93
  • https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/ethregistrar/ETHRegistrarController.sol#L256
  • https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/wrapper/ERC1155Fuse.sol#L92
  • https://github.com/code-423n4/2022-07-ens/blob/04539b27307abd65068b96dd20fc686b451bf9fc/contracts/wrapper/ERC1155Fuse.sol#L205
• In setUpgradeContract, the variable _upgradeAddress could be used for the three calls on line 139 - 141 (https://github.com/code-423n4/2022-07-ens/blob/1772ac3d16c8e84bb9c1e02a0c4e321cd6f5652b/contracts/wrapper/NameWrapper.sol#L139) instead of always reading upgradeContract from memory.