Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rude-swans-walk.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`ERC165Checker`: Condition the `0xffffff` check in `supportsERC165` to return false if the underlying call reverts.
12 changes: 0 additions & 12 deletions contracts/mocks/ERC165/ERC165MaliciousData.sol

This file was deleted.

7 changes: 0 additions & 7 deletions contracts/mocks/ERC165/ERC165MissingData.sol

This file was deleted.

5 changes: 0 additions & 5 deletions contracts/mocks/ERC165/ERC165NotSupported.sol

This file was deleted.

18 changes: 0 additions & 18 deletions contracts/mocks/ERC165/ERC165ReturnBomb.sol

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

pragma solidity ^0.8.20;

import {IERC165} from "../../utils/introspection/IERC165.sol";
import {IERC165} from "../utils/introspection/IERC165.sol";

/**
* https://eips.ethereum.org/EIPS/eip-214#specification
Expand Down Expand Up @@ -36,7 +36,7 @@ contract SupportsInterfaceWithLookupMock is IERC165 {
/**
* @dev Implement supportsInterface(bytes4) using a lookup table.
*/
function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
return _supportedInterfaces[interfaceId];
}

Expand All @@ -56,3 +56,45 @@ contract ERC165InterfacesSupported is SupportsInterfaceWithLookupMock {
}
}
}

// Similar to ERC165InterfacesSupported, but revert (without reason) when an interface is not supported
contract ERC165RevertInvalid is SupportsInterfaceWithLookupMock {
constructor(bytes4[] memory interfaceIds) {
for (uint256 i = 0; i < interfaceIds.length; i++) {
_registerInterface(interfaceIds[i]);
}
}

function supportsInterface(bytes4 interfaceId) public view override returns (bool) {
require(super.supportsInterface(interfaceId));
return true;
}
}

contract ERC165MaliciousData {
function supportsInterface(bytes4) public pure returns (bool) {
assembly {
mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
return(0, 32)
}
}
}

contract ERC165MissingData {
function supportsInterface(bytes4 interfaceId) public view {} // missing return
}

contract ERC165NotSupported {}

contract ERC165ReturnBombMock is IERC165 {
function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
if (interfaceId == type(IERC165).interfaceId) {
assembly {
mstore(0, 1)
}
}
assembly {
return(0, 101500)
}
}
}
46 changes: 32 additions & 14 deletions contracts/utils/introspection/ERC165Checker.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,12 @@ library ERC165Checker {
function supportsERC165(address account) internal view returns (bool) {
// Any contract that implements ERC-165 must explicitly indicate support of
// InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid
return
supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId) &&
!supportsERC165InterfaceUnchecked(account, INTERFACE_ID_INVALID);
if (supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId)) {
(bool success, bool supported) = _trySupportsInterface(account, INTERFACE_ID_INVALID);
return success && !supported;
} else {
return false;
}
}

/**
Expand Down Expand Up @@ -106,19 +109,34 @@ library ERC165Checker {
* Interface identification is specified in ERC-165.
*/
function supportsERC165InterfaceUnchecked(address account, bytes4 interfaceId) internal view returns (bool) {
// prepare call
bytes memory encodedParams = abi.encodeCall(IERC165.supportsInterface, (interfaceId));
(bool success, bool supported) = _trySupportsInterface(account, interfaceId);
return success && supported;
}

/**
* @dev Attempts to call `supportsInterface` on a contract and returns both the call
* success status and the interface support result.
*
* This function performs a low-level static call to the contract's `supportsInterface`
* function. It returns:
*
* * `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
) private view returns (bool success, bool supported) {
bytes4 selector = IERC165.supportsInterface.selector;

// perform static call
bool success;
uint256 returnSize;
uint256 returnValue;
assembly ("memory-safe") {
success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
returnSize := returndatasize()
returnValue := mload(0x00)
mstore(0x00, selector)
mstore(0x04, interfaceId)
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we checking x!=0 as opposed to x==1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a contract returns 2, is that considered true according to the ERC? I'd argue no.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think its a big deal but anything other than 1 is definitely not true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll break one of our tests, though

#5880 (comment)

I think we should weigh @Amxx opinion here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sourcery beat me here. @Amxx seems to want to keep it as is.

Copy link
Collaborator

@Amxx Amxx Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also agree it is not a big deal, though should we take the opportunity to update it in 6.0?

)
}

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