ENS Contest - MalfurionWhitehat's results

Lines of code

https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/OffchainDNSResolver.sol#L109-L113 https://github.com/code-423n4/2023-04-ens/blob/main/contracts/dnsregistrar/OffchainDNSResolver.sol#L119

Vulnerability details

The resolveCallback function from OffchainDNSResolver is used as part of the EIP-3668 standard to properly resolve DNS names using an off-chain gateway and validating RRsets against the DNSSEC oracle.

The issue is that the function lacks proper error handling, specifically, a try/catch block to properly bubble up OffchainLookup error from the dnsresolver extracted from the RRset. As the EIP specifies,

When a CCIP-aware contract wishes to make a call to another contract, and the possibility exists that the callee may implement CCIP read, the calling contract MUST catch all OffchainLookup errors thrown by the callee, and revert with a different error if the sender field of the error does not match the callee address. [...] Where the possibility exists that a callee implements CCIP read, a CCIP-aware contract MUST NOT allow the default solidity behaviour of bubbling up reverts from nested calls.

Impact

As per the EIP, the result would be an OffchainLookup that looks valid to the client, as the sender field matches the address of the contract that was called, but does not execute correctly.

Proof of Concept

1. Client calls OffchainDNSResolver.resolve, which reverts with OffchainLookup, and prompts the client to execute resolveCallback after having fetched the necessary data from the gatewayURL
2. The RRset returned by the gateway contains a dnsresolver that is a CCIP-aware contract, and also supports the IExtendedDNSResolver.resolve.selector interface
3. Calling IExtendedDNSResolver(dnsresolver).resolve(name,query,context); could trigger another OffchainLookup error, but with a sender that does not match the dnsresolver, which would be just returned to the client without any modifications
4. As a result, the sender field would be incorrect as per the EIP

Tools Used

Manual review

Recommended Mitigation Steps

Use the recommended example from the EIP in order to support nested lookups.

QA-01 DNSClaimChecker.parseString reverts with unexpected reason if str contains less than 4 bytes

    function parseString(
        bytes memory str,
        uint256 idx,
        uint256 len
    ) internal pure returns (address, bool) {
        // TODO: More robust parsing that handles whitespace and multiple key/value pairs
        if (str.readUint32(idx) != 0x613d3078) return (address(0x0), false); // 0x613d3078 == 'a=0x'
        return str.hexToAddress(idx + 4, idx + len);
    }

For example, attempting to claim the RRset a= will revert, as it contains only 2 bytes

Remediation: check if len - idx >= 4 before calling str.readUint32(idx)