From a827be1b27ad6373258cc4071b13f34afeb2eb0d Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 22 Aug 2025 11:17:36 -1000 Subject: [PATCH 1/9] Fix ERC165 --- .../mocks/ERC165/ERC165MaliciousData.sol | 12 -- contracts/mocks/ERC165/ERC165MissingData.sol | 7 -- contracts/mocks/ERC165/ERC165NotSupported.sol | 5 - contracts/mocks/ERC165/ERC165ReturnBomb.sol | 18 --- ...InterfacesSupported.sol => ERC165Mock.sol} | 39 ++++++- .../utils/introspection/ERC165Checker.sol | 31 ++++-- .../utils/introspection/ERC165Checker.test.js | 104 +++++++++++------- 7 files changed, 125 insertions(+), 91 deletions(-) delete mode 100644 contracts/mocks/ERC165/ERC165MaliciousData.sol delete mode 100644 contracts/mocks/ERC165/ERC165MissingData.sol delete mode 100644 contracts/mocks/ERC165/ERC165NotSupported.sol delete mode 100644 contracts/mocks/ERC165/ERC165ReturnBomb.sol rename contracts/mocks/{ERC165/ERC165InterfacesSupported.sol => ERC165Mock.sol} (63%) diff --git a/contracts/mocks/ERC165/ERC165MaliciousData.sol b/contracts/mocks/ERC165/ERC165MaliciousData.sol deleted file mode 100644 index 35427567d4e..00000000000 --- a/contracts/mocks/ERC165/ERC165MaliciousData.sol +++ /dev/null @@ -1,12 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -contract ERC165MaliciousData { - function supportsInterface(bytes4) public pure returns (bool) { - assembly { - mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) - return(0, 32) - } - } -} diff --git a/contracts/mocks/ERC165/ERC165MissingData.sol b/contracts/mocks/ERC165/ERC165MissingData.sol deleted file mode 100644 index fec43391b97..00000000000 --- a/contracts/mocks/ERC165/ERC165MissingData.sol +++ /dev/null @@ -1,7 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -contract ERC165MissingData { - function supportsInterface(bytes4 interfaceId) public view {} // missing return -} diff --git a/contracts/mocks/ERC165/ERC165NotSupported.sol b/contracts/mocks/ERC165/ERC165NotSupported.sol deleted file mode 100644 index 78ef9c8e01e..00000000000 --- a/contracts/mocks/ERC165/ERC165NotSupported.sol +++ /dev/null @@ -1,5 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -contract ERC165NotSupported {} diff --git a/contracts/mocks/ERC165/ERC165ReturnBomb.sol b/contracts/mocks/ERC165/ERC165ReturnBomb.sol deleted file mode 100644 index 4bfacfd669e..00000000000 --- a/contracts/mocks/ERC165/ERC165ReturnBomb.sol +++ /dev/null @@ -1,18 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.20; - -import {IERC165} from "../../utils/introspection/IERC165.sol"; - -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) - } - } -} diff --git a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol b/contracts/mocks/ERC165Mock.sol similarity index 63% rename from contracts/mocks/ERC165/ERC165InterfacesSupported.sol rename to contracts/mocks/ERC165Mock.sol index dffd6a24ed6..82aa0e2afda 100644 --- a/contracts/mocks/ERC165/ERC165InterfacesSupported.sol +++ b/contracts/mocks/ERC165Mock.sol @@ -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 @@ -56,3 +56,40 @@ contract ERC165InterfacesSupported is SupportsInterfaceWithLookupMock { } } } + +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) + } + } +} + +contract ERC165RevertInvalid is IERC165 { + function supportsInterface(bytes4 interfaceId) public view virtual returns (bool) { + if (interfaceId == 0xffffffff) { + revert(); + } + return interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7 + } +} diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 8650f5503cc..619f3be9db0 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -22,9 +22,9 @@ 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)) return false; + (bool success, bool supported) = trySupportsInterface(account, INTERFACE_ID_INVALID); + return success && !supported; } /** @@ -106,19 +106,32 @@ 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; + } - // perform static call - bool success; + /** + * @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 + ) 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); } } diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js index 1bbe8a57130..7e336057a48 100644 --- a/test/utils/introspection/ERC165Checker.test.js +++ b/test/utils/introspection/ERC165Checker.test.js @@ -24,23 +24,23 @@ describe('ERC165Checker', function () { }); it('does not support ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.false; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.false; }); it('does not support mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.false; }); it('does not support mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.false; }); it('does not support mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.deep.equal([false]); + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); }); it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.false; }); }); @@ -50,23 +50,49 @@ describe('ERC165Checker', function () { }); it('does not support ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.false; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.false; }); it('does not support mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.false; }); it('does not support mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.false; }); it('does not support mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.deep.equal([false]); + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); }); it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.be.true; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.true; + }); + }); + + describe('ERC165 revert on invalid interface', function () { + beforeEach(async function () { + this.target = await ethers.deployContract('ERC165RevertInvalid'); + }); + + it('does not support ERC165', async function () { + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.false; + }); + + it('does not support mock interface via supportsInterface', async function () { + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.false; + }); + + it('does not support mock interface via supportsAllInterfaces', async function () { + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.false; + }); + + it('does not support mock interface via getSupportedInterfaces', async function () { + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); + }); + + it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.false; }); }); @@ -76,23 +102,23 @@ describe('ERC165Checker', function () { }); it('does not support ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.false; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.false; }); it('does not support mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.false; }); it('does not support mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.false; }); it('does not support mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.deep.equal([false]); + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); }); it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.false; }); }); @@ -102,23 +128,23 @@ describe('ERC165Checker', function () { }); it('supports ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.true; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.true; }); it('does not support mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.false; }); it('does not support mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.false; }); it('does not support mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.deep.equal([false]); + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); }); it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.false; }); }); @@ -128,23 +154,23 @@ describe('ERC165Checker', function () { }); it('supports ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.true; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.true; }); it('supports mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(this.target, DUMMY_ID)).to.be.true; + await expect(this.mock.$supportsInterface(this.target, DUMMY_ID)).to.eventually.be.true; }); it('supports mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.be.true; + await expect(this.mock.$supportsAllInterfaces(this.target, [DUMMY_ID])).to.eventually.be.true; }); it('supports mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.deep.equal([true]); + await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([true]); }); it('supports mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.be.true; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.true; }); }); @@ -155,32 +181,32 @@ describe('ERC165Checker', function () { }); it('supports ERC165', async function () { - expect(await this.mock.$supportsERC165(this.target)).to.be.true; + await expect(this.mock.$supportsERC165(this.target)).to.eventually.be.true; }); it('supports each interfaceId via supportsInterface', async function () { for (const interfaceId of supportedInterfaces) { - expect(await this.mock.$supportsInterface(this.target, interfaceId)).to.be.true; + await expect(this.mock.$supportsInterface(this.target, interfaceId)).to.eventually.be.true; } }); it('supports all interfaceIds via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(this.target, supportedInterfaces)).to.be.true; + await expect(this.mock.$supportsAllInterfaces(this.target, supportedInterfaces)).to.eventually.be.true; }); it('supports none of the interfaces queried via supportsAllInterfaces', async function () { const interfaceIdsToTest = [DUMMY_UNSUPPORTED_ID, DUMMY_UNSUPPORTED_ID_2]; - expect(await this.mock.$supportsAllInterfaces(this.target, interfaceIdsToTest)).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, interfaceIdsToTest)).to.eventually.be.false; }); it('supports not all of the interfaces queried via supportsAllInterfaces', async function () { const interfaceIdsToTest = [...supportedInterfaces, DUMMY_UNSUPPORTED_ID]; - expect(await this.mock.$supportsAllInterfaces(this.target, interfaceIdsToTest)).to.be.false; + await expect(this.mock.$supportsAllInterfaces(this.target, interfaceIdsToTest)).to.eventually.be.false; }); it('supports all interfaceIds via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(this.target, supportedInterfaces)).to.deep.equal( + await expect(this.mock.$getSupportedInterfaces(this.target, supportedInterfaces)).to.eventually.deep.equal( supportedInterfaces.map(i => supportedInterfaces.includes(i)), ); }); @@ -188,7 +214,7 @@ describe('ERC165Checker', function () { it('supports none of the interfaces queried via getSupportedInterfaces', async function () { const interfaceIdsToTest = [DUMMY_UNSUPPORTED_ID, DUMMY_UNSUPPORTED_ID_2]; - expect(await this.mock.$getSupportedInterfaces(this.target, interfaceIdsToTest)).to.deep.equal( + await expect(this.mock.$getSupportedInterfaces(this.target, interfaceIdsToTest)).to.eventually.deep.equal( interfaceIdsToTest.map(i => supportedInterfaces.includes(i)), ); }); @@ -196,37 +222,37 @@ describe('ERC165Checker', function () { it('supports not all of the interfaces queried via getSupportedInterfaces', async function () { const interfaceIdsToTest = [...supportedInterfaces, DUMMY_UNSUPPORTED_ID]; - expect(await this.mock.$getSupportedInterfaces(this.target, interfaceIdsToTest)).to.deep.equal( + await expect(this.mock.$getSupportedInterfaces(this.target, interfaceIdsToTest)).to.eventually.deep.equal( interfaceIdsToTest.map(i => supportedInterfaces.includes(i)), ); }); it('supports each interfaceId via supportsERC165InterfaceUnchecked', async function () { for (const interfaceId of supportedInterfaces) { - expect(await this.mock.$supportsERC165InterfaceUnchecked(this.target, interfaceId)).to.be.true; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, interfaceId)).to.eventually.be.true; } }); }); describe('account address does not support ERC165', function () { it('does not support ERC165', async function () { - expect(await this.mock.$supportsERC165(DUMMY_ACCOUNT)).to.be.false; + await expect(this.mock.$supportsERC165(DUMMY_ACCOUNT)).to.eventually.be.false; }); it('does not support mock interface via supportsInterface', async function () { - expect(await this.mock.$supportsInterface(DUMMY_ACCOUNT, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsInterface(DUMMY_ACCOUNT, DUMMY_ID)).to.eventually.be.false; }); it('does not support mock interface via supportsAllInterfaces', async function () { - expect(await this.mock.$supportsAllInterfaces(DUMMY_ACCOUNT, [DUMMY_ID])).to.be.false; + await expect(this.mock.$supportsAllInterfaces(DUMMY_ACCOUNT, [DUMMY_ID])).to.eventually.be.false; }); it('does not support mock interface via getSupportedInterfaces', async function () { - expect(await this.mock.$getSupportedInterfaces(DUMMY_ACCOUNT, [DUMMY_ID])).to.deep.equal([false]); + await expect(this.mock.$getSupportedInterfaces(DUMMY_ACCOUNT, [DUMMY_ID])).to.eventually.deep.equal([false]); }); it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - expect(await this.mock.$supportsERC165InterfaceUnchecked(DUMMY_ACCOUNT, DUMMY_ID)).to.be.false; + await expect(this.mock.$supportsERC165InterfaceUnchecked(DUMMY_ACCOUNT, DUMMY_ID)).to.eventually.be.false; }); }); From ed190367c5c01ec0651d0ad421e7bc2fba338139 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 25 Aug 2025 10:28:17 +0200 Subject: [PATCH 2/9] minor mock/test refactor --- contracts/mocks/ERC165Mock.sol | 25 +++++++++++-------- .../utils/introspection/ERC165Checker.test.js | 7 +++--- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/contracts/mocks/ERC165Mock.sol b/contracts/mocks/ERC165Mock.sol index 82aa0e2afda..53ba24ed344 100644 --- a/contracts/mocks/ERC165Mock.sol +++ b/contracts/mocks/ERC165Mock.sol @@ -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]; } @@ -57,6 +57,20 @@ 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 { @@ -84,12 +98,3 @@ contract ERC165ReturnBombMock is IERC165 { } } } - -contract ERC165RevertInvalid is IERC165 { - function supportsInterface(bytes4 interfaceId) public view virtual returns (bool) { - if (interfaceId == 0xffffffff) { - revert(); - } - return interfaceId == type(IERC165).interfaceId; // 0x01ffc9a7 - } -} diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js index 7e336057a48..7954b3d7621 100644 --- a/test/utils/introspection/ERC165Checker.test.js +++ b/test/utils/introspection/ERC165Checker.test.js @@ -72,7 +72,7 @@ describe('ERC165Checker', function () { describe('ERC165 revert on invalid interface', function () { beforeEach(async function () { - this.target = await ethers.deployContract('ERC165RevertInvalid'); + this.target = await ethers.deployContract('ERC165RevertInvalid', [[DUMMY_ID]]); }); it('does not support ERC165', async function () { @@ -91,8 +91,9 @@ describe('ERC165Checker', function () { await expect(this.mock.$getSupportedInterfaces(this.target, [DUMMY_ID])).to.eventually.deep.equal([false]); }); - it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () { - await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.false; + it('support mock interface via supportsERC165InterfaceUnchecked', async function () { + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, '0xffffffff')).to.eventually.be.false; + await expect(this.mock.$supportsERC165InterfaceUnchecked(this.target, DUMMY_ID)).to.eventually.be.true; }); }); From 49ef98f53c434a220c724252ede3175ab86122a9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 25 Aug 2025 10:43:44 +0200 Subject: [PATCH 3/9] use assembly call --- .../utils/introspection/ERC165Checker.sol | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 619f3be9db0..4449dac2d66 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -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 - if (!supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId)) return false; - (bool success, bool supported) = trySupportsInterface(account, INTERFACE_ID_INVALID); - return success && !supported; + if (supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId)) { + (bool success, bool supported) = trySupportsInterface(account, INTERFACE_ID_INVALID); + return success && !supported; + } else { + return false; + } } /** @@ -124,14 +127,16 @@ library ERC165Checker { address account, bytes4 interfaceId ) internal view returns (bool success, bool supported) { - uint256 returnSize; - uint256 returnValue; - bytes memory encodedParams = abi.encodeCall(IERC165.supportsInterface, (interfaceId)); + bytes4 selector = IERC165.supportsInterface.selector; + 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 + mload(0x00) // the first 20 bytes of returndata are non-zero + ) } - return (success, returnSize >= 0x20 && returnValue > 0); } } From b3a2c82bd43bc83cdb96c7677f8430e8edeedf05 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 25 Aug 2025 11:27:47 +0200 Subject: [PATCH 4/9] cleanup loaded value --- contracts/utils/introspection/ERC165Checker.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 4449dac2d66..23e49cbb643 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -135,7 +135,7 @@ library ERC165Checker { success := staticcall(30000, account, 0x00, 0x24, 0x00, 0x20) supported := and( gt(returndatasize(), 0x1F), // we have at least 20 bytes of returndata - mload(0x00) // the first 20 bytes of returndata are non-zero + iszero(iszero(mload(0x00))) // the first 20 bytes of returndata are non-zero ) } } From e3c2544373e0e90dc1cffda280b276958494bb28 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 25 Aug 2025 05:48:07 -1000 Subject: [PATCH 5/9] up --- contracts/utils/introspection/ERC165Checker.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 23e49cbb643..eb2c6732d3b 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -23,7 +23,7 @@ library ERC165Checker { // Any contract that implements ERC-165 must explicitly indicate support of // InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid if (supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId)) { - (bool success, bool supported) = trySupportsInterface(account, INTERFACE_ID_INVALID); + (bool success, bool supported) = _trySupportsInterface(account, INTERFACE_ID_INVALID); return success && !supported; } else { return false; @@ -109,7 +109,7 @@ library ERC165Checker { * Interface identification is specified in ERC-165. */ function supportsERC165InterfaceUnchecked(address account, bytes4 interfaceId) internal view returns (bool) { - (bool success, bool supported) = trySupportsInterface(account, interfaceId); + (bool success, bool supported) = _trySupportsInterface(account, interfaceId); return success && supported; } @@ -123,10 +123,10 @@ library ERC165Checker { * * `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( + function _trySupportsInterface( address account, bytes4 interfaceId - ) internal view returns (bool success, bool supported) { + ) private view returns (bool success, bool supported) { bytes4 selector = IERC165.supportsInterface.selector; assembly ("memory-safe") { From 75ae31b2b7399d6d6d2f9c5ad7cfdc78d4e34ea0 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 25 Aug 2025 09:30:21 -1000 Subject: [PATCH 6/9] Add changeset --- .changeset/rude-swans-walk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/rude-swans-walk.md diff --git a/.changeset/rude-swans-walk.md b/.changeset/rude-swans-walk.md new file mode 100644 index 00000000000..4f717c36dc0 --- /dev/null +++ b/.changeset/rude-swans-walk.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ERC165Checker`: Condition the `0xffffff` check in `supportsERC165` to return false if the underlying call reverts. From 6093319e04f79f2ee25196b5c370c75701fed7b5 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 26 Aug 2025 08:07:33 -1000 Subject: [PATCH 7/9] Remove changeset and add to changelog --- .changeset/rude-swans-walk.md | 5 ----- CHANGELOG.md | 3 +++ 2 files changed, 3 insertions(+), 5 deletions(-) delete mode 100644 .changeset/rude-swans-walk.md diff --git a/.changeset/rude-swans-walk.md b/.changeset/rude-swans-walk.md deleted file mode 100644 index 4f717c36dc0..00000000000 --- a/.changeset/rude-swans-walk.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'openzeppelin-solidity': minor ---- - -`ERC165Checker`: Condition the `0xffffff` check in `supportsERC165` to return false if the underlying call reverts. diff --git a/CHANGELOG.md b/CHANGELOG.md index 74e0952bedf..8a08556f022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +### Bug fixes + +- `ERC165Checker`: Ensure the `supportsERC165` function returns false if the second underlying call to `supportsInterface` while detecting ERC-165 reverts. ([#5810](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5880)) ### Breaking changes From 19509048e8d54c1a31115b007ee9075fa6a12326 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Tue, 26 Aug 2025 21:35:54 -1000 Subject: [PATCH 8/9] Update CHANGELOG.md Co-authored-by: Hadrien Croubois --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a08556f022..beaeed885ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Bug fixes -- `ERC165Checker`: Ensure the `supportsERC165` function returns false if the second underlying call to `supportsInterface` while detecting ERC-165 reverts. ([#5810](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5880)) +- `ERC165Checker`: Ensure the `supportsERC165` function returns false if the target reverts during the `supportsInterface(0xffffffff)` call. ([#5810](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5880)) ### Breaking changes From 741634784009988bbef4e5442e8e84e0889f2cd3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 27 Aug 2025 14:42:51 +0200 Subject: [PATCH 9/9] Update contracts/utils/introspection/ERC165Checker.sol Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com> --- contracts/utils/introspection/ERC165Checker.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index eb2c6732d3b..60e1b966316 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -134,8 +134,8 @@ library ERC165Checker { 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 + gt(returndatasize(), 0x1F), // we have at least 32 bytes of returndata + iszero(iszero(mload(0x00))) // the first 32 bytes of returndata are non-zero ) } }