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

0 stars 0 forks source link

Offchain resolver can return stale values #255

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-ens/blob/45ea10bacb2a398e14d711fe28d1738271cd7640/contracts/dnsregistrar/OffchainDNSResolver.sol#L68

Vulnerability details

Description

Offchain resolver makes sure the offchain-received RRsets are valid:

DNSSEC.RRSetWithSignature[] memory rrsets = abi.decode(
    response,
    (DNSSEC.RRSetWithSignature[])
);
(bytes memory data, ) = oracle.verifyRRSet(rrsets);

However, differently from the on-chain checker, the client could receive a validly signed but old RRset. There is no check that the received set is the newest, which introduces a significant assumption on off-chain data.

EIP 3668 states:

This mechanism allows for offchain lookups of data in a way that is transparent to clients, and allows contract authors to implement whatever validation is necessary; in many cases this can be provided without any additional trust assumptions over and above those required if data is stored onchain.

The submission details an unmentioned trust assumption which can be abused by several offchain means. Due to the proven reduction to offchain trust, which is a lower degree of trust than on a classical smart contract privileged owner, I would argue for a different treatment.

Impact

Trusted use of overriden RRsets can lead to loss of funds, as ENS resolver address can change due to a variety of reasons and wallet addresses are subject to change.

Tools Used

Manual audit

Recommended Mitigation Steps

It is advisable to come up with a cryptographic committment scheme so that offchain checks could guarantee there is not a more up-to-date response than the one returned.

c4-pre-sort commented 1 year ago

thereksfour marked the issue as primary issue

Arachnid commented 1 year ago

This is an unavoidable consequence of validating records submitted from offchain. There's no commitment scheme that can prove a negative, and this issue exists equally for all DNSSEC resolvers, unless they already have knowledge of a newer record.

c4-sponsor commented 1 year ago

Arachnid marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)

dmvt commented 1 year ago

On reconsideration, this is valid even if it's shared by all DNSSEC resolvers. Leaving it in place as QA.

c4-judge commented 1 year ago

dmvt marked the issue as grade-b