Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document Edge Case for supportsERC165InterfaceUnchecked(..) #4011

Closed
YamenMerhi opened this issue Jan 28, 2023 · 2 comments · Fixed by #4017
Closed

Document Edge Case for supportsERC165InterfaceUnchecked(..) #4011

YamenMerhi opened this issue Jan 28, 2023 · 2 comments · Fixed by #4017
Labels
documentation Inline comments, guides, and examples. good first issue Low hanging fruit for new contributors to get involved!

Comments

@YamenMerhi
Copy link
Contributor

What this issue is about ?

The supportsERC165(..) function when calling to check if an address supports a specific interfaceId, according to the ERC165 specs, it will check whether the address queried is supporting the ERC165 interfaceId, the queried interfaceId, also will check that the address queried is not supporting the 0xffffffff interfaceId.

The supportsERC165InterfaceUnchecked(..) function will check whether a specific address supports a single interfaceId, through ERC165. However, some addresses will return always true regardless if they support the interfaceId or not.

These are the precompile addresses 0x000..0000002, 0x000..0000003, and 0x000..0000004. It's unclear to me why this behavior is happening. This issue was pointed out by Runtime verification auditors while doing some fuzzing tests.

📝 Details

This happens only to supportsERC165InterfaceUnchecked(..), because since the precompile addresses 0x000..0000002, 0x000..0000003, and 0x000..0000004 will return always true for all interfaceIds, it means that these addresses are also supporting the 0xffffffff interfaceId.

Since supportsERC165(..) checks that the address does not support 0xffffffff interfaceId, then supportsERC165(..) is excluded from this behavior.

This issue is opened to add documentation to supportsERC165InterfaceUnchecked(..) function using natspec, to acknowledge this edge case, and also to make it easy for developers doing fuzzing tests using foundry, to identify why the tests are failing when lacking context that these addresses will always return true.

🔢 Code to reproduce bug

You can call supportsERC165InterfaceUnchecked(..) on 0x000..0000002 , 0x000..0000003 and 0x000..0000004 addresses with any interfaceId to reproduce the case.

const precompiledAddress = [
      "0x0000000000000000000000000000000000000002",
      "0x0000000000000000000000000000000000000003",
      "0x0000000000000000000000000000000000000004",
    ];

    for (let i = 0; i < precompiledAddress.length; i++) {
      for (let ii = 0; ii < 100; ii++) {
        const result = await contract.supportsERC165Interface(
          precompiledAddress[i],
          ethers.utils.hexlify(ethers.utils.randomBytes(4))
        );
        expect(result).to.be.true;
      }
    }

Please let me know if it makes sense to add this to the function natspec.

@Amxx
Copy link
Collaborator

Amxx commented Jan 30, 2023

Hello @YamenMerhi, and thank for the great example.

This function was introduced in #3339, after a lot of discussion concerning the fact that it should indeed not be used alone. I don't think we thought about the precompiles, which is a great example of why this function should not be used without also checking 0x01ffc9a7 and 0xffffffff.

I definitely agree that this could be documented better. Fell free to open a PR for that.

@Amxx Amxx added good first issue Low hanging fruit for new contributors to get involved! documentation Inline comments, guides, and examples. labels Jan 30, 2023
@YamenMerhi
Copy link
Contributor Author

@Amxx Sure, will do 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Inline comments, guides, and examples. good first issue Low hanging fruit for new contributors to get involved!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants