I was just checking the newish OffchainDNSResolver contract, and there seems to be an unhandled failure in the OffchainDNSResolver when a user includes an invalid resolver address as part of their DNS record.
As far as I understand, the root problem is in the way OffchainDNSResolver::parseAndResolve attempts to parse the user-provided value to an address.
Whatever value is passed in the nameOrAddress parameter, as long as it starts with 0x, it will be passed down to the HexUtils::hexToAddress function. Where it will attempt to cast it to an Ethereum address. Internally, this function never checks whether the length of the supplied data is longer than an address. It just casts it to a bytes32 value, and then to address.
As an example, the string 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00 would be considered to be the address 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00. This means that OffchainDNSResolver::parseRR would return a non-zero address (which is interpreted as a signal for a valid record found) and the name would try to be resolved calling the 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00 account.
If the called account happens to be empty (most likely the case if the record was just set incorrectly due to a user's oversight), then execution is reverted with error "function returned an unexpected amount of data" instead of the more gracious CouldNotResolve(bytes) error. This is because the call to IERC165(dnsresolver).supportsInterface(...) is using the explicit cast to the IERC165 interface, which is always expecting a boolean in return - reverting otherwise.
Perhaps the code could be changed to fail earlier. Non-addresses shouldn't be silently parsed as addresses.
Once that's fixed, we'd be left with the scenario where the user sets a valid address in the record, but that address corresponds to a custom resolver that does not follow IERC165. If you want to support that case (which appears to be so, given this fallback-like staticcall), then instead of explicitly casting to IERC165 you might want to consider using the ERC165Checker lib or anything similar that does not revert, but instead allows you to explicitly handle errors when querying unsupported interfaces.
I was just checking the newish
OffchainDNSResolvercontract, and there seems to be an unhandled failure in theOffchainDNSResolverwhen a user includes an invalid resolver address as part of their DNS record.As far as I understand, the root problem is in the way
OffchainDNSResolver::parseAndResolveattempts to parse the user-provided value to an address. Whatever value is passed in thenameOrAddressparameter, as long as it starts with 0x, it will be passed down to theHexUtils::hexToAddressfunction. Where it will attempt to cast it to an Ethereum address. Internally, this function never checks whether the length of the supplied data is longer than an address. It just casts it to abytes32value, and then toaddress.As an example, the string
0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00would be considered to be the address0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00. This means thatOffchainDNSResolver::parseRRwould return a non-zero address (which is interpreted as a signal for a valid record found) and the name would try to be resolved calling the0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00account.If the called account happens to be empty (most likely the case if the record was just set incorrectly due to a user's oversight), then execution is reverted with error "function returned an unexpected amount of data" instead of the more gracious
CouldNotResolve(bytes)error. This is because the call toIERC165(dnsresolver).supportsInterface(...)is using the explicit cast to theIERC165interface, which is always expecting a boolean in return - reverting otherwise.Perhaps the code could be changed to fail earlier. Non-addresses shouldn't be silently parsed as addresses.
Once that's fixed, we'd be left with the scenario where the user sets a valid address in the record, but that address corresponds to a custom resolver that does not follow
IERC165. If you want to support that case (which appears to be so, given this fallback-likestaticcall), then instead of explicitly casting toIERC165you might want to consider using the ERC165Checker lib or anything similar that does not revert, but instead allows you to explicitly handle errors when querying unsupported interfaces.