Erc165 checker : misleading check

Support Contracts
1

Hi Team.

I noticed: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5e28952cbdc0eb7d19ee62580ab31b30c2376e48/contracts/utils/introspection/ERC165Checker.sol#L120

Whats the reason of having the following:

return success && returnSize >= 0x20 && returnValue > 0;

Why not having:

return success && returnSize == 0x20 && returnValue == 1;

You're generally intereasted that the call is successful or not and whether it returns 32 bytes data. Why do you care checking if it's more than 0x20 ? if it is more, it's 100% false as ERC165 clearly states it returns only true|false.

1 Like
2

Hey @novaknole,

This can be exploited using a returnbomb. This means that a malicious contract can implement a supportsInterface(bytes4 interfaceId) method that "bombs" the return data, sending back a huge amount in the response. This is a way of attacking the cost of memory expansion that the contract calling the supportsInterface is actually paying.

Suppose the following example:

contract Malicious {
    function supporstInterface(bytes4 interfaceId) external pure returns (bytes memory) {
        assembly{
            revert(0, 1_000_000)
        }
    }
}

This is specially dangerous in a library like OpenZeppelin since there are protocols and projects relying on third-party contracts. Take for example a protocol using NFTs for any reason, this will require the implementers to use supportsInterface() for tokens added by the users and there's no way of trusting their return values.

See this example.

Hope this helps.

3

Hi @ernestognw

Thanks for the detailed answer, but still got some questions. Hope that won't be a problem.

Question 1: I modified the example to make it compile. Take a look here: https://gist.github.com/novaknole/5f761849cba2f076d8b89bc78a137af8 (you can directly copy/paste this code in remix and it will compile). Try to compile. When you run it, it actually succeeds. Even if you set gas limit to 80000, it will still succeed. The way I understand it, at the time of a = 10, it should revert, but it actually succeeds(check a value after the call - it will be 10).

Question 2: At the time this code executes(https://github.com/OpenZeppelin/openzeppelin-contracts/blob/5e28952cbdc0eb7d19ee62580ab31b30c2376e48/contracts/utils/introspection/ERC165Checker.sol#L117) , I believe the return bomb data is already copied in the local memory, and making expansion cost. If thats true, when the code gets to return success && returnSize >= 0x20 && returnValue > 0;, it's already late. no ?

I might be misunderstanding some detail, would appreciate it. Thank you.

1 Like
4

I modified the example to make it compile

Yes, it compiles and works as expected, but the security issue is not actually there but in a dependant contract that wraps the call in a try/catch block. That'd might skip state cleanups.

Consider the following example:

contract RelyingInMark {
    Mark mark;

    constructor() {
        mark = new Mark();
    }

    function avoidCleanup(address badGuy) public {
        try mark.oops(badGuy) {} catch {
            // THIS may avoid state cleanups
        }
    }

    function a() public view returns (uint256) {
        return mark.a();
    }
}

It requires a little bit of testing to get it done, but is actually possible to call avoidCleanup while setting storage values before the supporstInterface call and not setting a = 10.

This also answers #2, the function should work even when returnSize and returnValue are larger.

Hope it's clear