Fix ERC165Checker detection (#5880)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog
 
+### Bug fixes
+
+- `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
 

contracts/mocks/ERC165/ERC165MaliciousData.sol

@@ -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)
-        }
-    }
-}

contracts/mocks/ERC165/ERC165MissingData.sol

@@ -1,7 +0,0 @@
-// SPDX-License-Identifier: MIT
-
-pragma solidity ^0.8.20;
-
-contract ERC165MissingData {
-    function supportsInterface(bytes4 interfaceId) public view {} // missing return
-}

contracts/mocks/ERC165/ERC165NotSupported.sol

@@ -1,5 +0,0 @@
-// SPDX-License-Identifier: MIT
-
-pragma solidity ^0.8.20;
-
-contract ERC165NotSupported {}

contracts/mocks/ERC165/ERC165ReturnBomb.sol

@@ -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)
-        }
-    }
-}

contracts/mocks/ERC165/ERC165InterfacesSupported.sol → 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
@@ -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];
     }
 
@@ -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)
+        }
+    }
+}

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
-        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;
+        }
     }
 
     /**
@@ -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 32 bytes of returndata
+                iszero(iszero(mload(0x00))) // the first 32 bytes of returndata are non-zero
+            )
         }
-
-        return success && returnSize >= 0x20 && returnValue > 0;
     }
 }

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,50 @@ 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', [[DUMMY_ID]]);
+    });
+
+    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('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;
     });
   });
 
@@ -76,23 +103,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 +129,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 +155,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 +182,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 +215,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 +223,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;
     });
   });