Fix ERC165Checker detection #5880
Conversation
|
Reviewer's GuideThis PR refines ERC-165 detection by introducing a low-level trySupportsInterface helper in ERC165Checker, refactors existing checks to use it, adds edge-case mock contracts, updates tests to promise-aware assertions with new scenarios, and includes minor import path and documentation fixes. Entity relationship diagram for ERC165Checker and mock contractserDiagram
ERC165Checker ||--o| ERC165MaliciousData : checks
ERC165Checker ||--o| ERC165MissingData : checks
ERC165Checker ||--o| ERC165NotSupported : checks
ERC165Checker ||--o| ERC165ReturnBombMock : checks
ERC165Checker ||--o| ERC165RevertInvalid : checks
ERC165ReturnBombMock ||--|{ IERC165 : implements
ERC165RevertInvalid ||--|{ IERC165 : implements
Class diagram for new and updated ERC165 mock contractsclassDiagram
class ERC165MaliciousData {
+supportsInterface(bytes4): bool
}
class ERC165MissingData {
+supportsInterface(bytes4)
}
class ERC165NotSupported {
}
class ERC165ReturnBombMock {
+supportsInterface(bytes4): bool
}
class ERC165RevertInvalid {
+supportsInterface(bytes4): bool
}
ERC165ReturnBombMock --|> IERC165
ERC165RevertInvalid --|> IERC165
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ernestognw
force-pushed
the
inaccuracies
branch
from
8e509fb to
a827be1
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `contracts/utils/introspection/ERC165Checker.sol:123` </location>
<code_context>
+ * * `success`: true if the call didn't revert, false if it did
+ * * `supported`: true if the call succeeded AND returned data indicating the interface is supported
+ */
+ function trySupportsInterface(
+ address account,
+ bytes4 interfaceId
+ ) internal view returns (bool success, bool supported) {
uint256 returnSize;
uint256 returnValue;
+ bytes memory encodedParams = abi.encodeCall(IERC165.supportsInterface, (interfaceId));
assembly ("memory-safe") {
success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
returnSize := returndatasize()
returnValue := mload(0x00)
}
-
- return success && returnSize >= 0x20 && returnValue > 0;
+ return (success, returnSize >= 0x20 && returnValue > 0);
}
}
</code_context>
<issue_to_address>
The assembly block assumes the return value is at memory position 0, which may not always be safe.
Using mload(0x00) may not correctly capture the return value if the returned data is less than 32 bytes or formatted differently. To ensure reliability, use returndatacopy to copy the returned data before loading it.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
assembly ("memory-safe") {
success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
returnSize := returndatasize()
returnValue := mload(0x00)
}
=======
assembly ("memory-safe") {
success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
returnSize := returndatasize()
// Copy the returned data to memory position 0x00
returndatacopy(0x00, 0x00, returnSize)
// Load the first 32 bytes from memory position 0x00
returnValue := mload(0x00)
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `contracts/utils/introspection/ERC165Checker.sol:135` </location>
<code_context>
}
-
- return success && returnSize >= 0x20 && returnValue > 0;
+ return (success, returnSize >= 0x20 && returnValue > 0);
}
}
</code_context>
<issue_to_address>
The check for returnValue > 0 may not strictly conform to ERC165, which expects a boolean true/false.
Using returnValue == 1 would ensure strict ERC165 compliance and prevent contracts returning other nonzero values from being incorrectly considered as supporting the interface.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return (success, returnSize >= 0x20 && returnValue > 0);
=======
return (success, returnSize >= 0x20 && returnValue == 1);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
IMO we should we add a changelog entry for this fix |
| success := staticcall(30000, account, 0x00, 0x24, 0x00, 0x20) | ||
| supported := and( | ||
| gt(returndatasize(), 0x1F), // we have at least 20 bytes of returndata | ||
| iszero(iszero(mload(0x00))) // the first 20 bytes of returndata are non-zero |
There was a problem hiding this comment.
why are we checking x!=0 as opposed to x==1?
There was a problem hiding this comment.
We want anything that's not 0 to be interpreted as true. I think the rationale was to avoid the abi.decode to revert (see here). With assembly that might not be relevant anymore.
There was a problem hiding this comment.
If a contract returns 2, is that considered true according to the ERC? I'd argue no.
There was a problem hiding this comment.
It's technically correct because the ERC only distinguishes between 0 and 1 (false and true), so it's unclear how to treat the entire 32-byte value returned. I agree it's not ideal.
I would keep it for backwards compatibilty
There was a problem hiding this comment.
i don't think its a big deal but anything other than 1 is definitely not true
There was a problem hiding this comment.
There was a problem hiding this comment.
ah sourcery beat me here. @Amxx seems to want to keep it as is.
There was a problem hiding this comment.
I'm not sure if we should consider all non-zero value as true, or if only 1 should be considered true. I guess both approach make sens.
What I'm sure is that so far (since OZ 2.0.0), the library assumed all non zero mean true. Changing that behavior is a breaking change. I would avoid that unless we have very strong reasons to change the behavior.
There was a problem hiding this comment.
I would also agree it is not a big deal, though should we take the opportunity to update it in 6.0?
Co-authored-by: Hadrien Croubois <[email protected]>
| success := staticcall(30000, account, 0x00, 0x24, 0x00, 0x20) | ||
| supported := and( | ||
| gt(returndatasize(), 0x1F), // we have at least 20 bytes of returndata | ||
| iszero(iszero(mload(0x00))) // the first 20 bytes of returndata are non-zero |
There was a problem hiding this comment.
I would also agree it is not a big deal, though should we take the opportunity to update it in 6.0?
Co-authored-by: James Toussaint <[email protected]>
4 participants
Fixes #5878
PR Checklist
npx changeset add)Summary by Sourcery
Improve ERC165Checker by introducing a low-level call helper to distinguish between call failures and false responses, update interface support logic to handle reverts and malformed returns, refactor related code and mocks, and enhance tests with async-await and edge-case scenarios
New Features:
Bug Fixes:
Enhancements:
Tests: