Avoid returnbomb in ERC165Checker (#3587)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
OpenZeppelin · Jul 27, 2022 · dc4869e · dc4869e
1 parent 8ea1fc8
commit dc4869e
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 3 deletions.
diff --git a/contracts/mocks/ERC165/ERC165ReturnBomb.sol b/contracts/mocks/ERC165/ERC165ReturnBomb.sol
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+import "../../utils/introspection/IERC165.sol";
+
+contract ERC165ReturnBombMock is IERC165 {
+    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
+        if (interfaceId == type(IERC165).interfaceId) {
+            assembly {
+                mstore(0, 1)
+            }
+        }
+        assembly {
+            return(0, 101500)
+        }
+    }
+}
diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol
@@ -105,9 +105,19 @@ 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.encodeWithSelector(IERC165.supportsInterface.selector, interfaceId);
-        (bool success, bytes memory result) = account.staticcall{gas: 30000}(encodedParams);
-        if (result.length < 32) return false;
-        return success && abi.decode(result, (uint256)) > 0;
+
+        // perform static call
+        bool success;
+        uint256 returnSize;
+        uint256 returnValue;
+        assembly {
+            success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
+            returnSize := returndatasize()
+            returnValue := mload(0x00)
+        }
+
+        return success && returnSize >= 0x20 && returnValue > 0;
     }
 }
diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js
@@ -7,6 +7,7 @@ const ERC165MissingData = artifacts.require('ERC165MissingData');
 const ERC165MaliciousData = artifacts.require('ERC165MaliciousData');
 const ERC165NotSupported = artifacts.require('ERC165NotSupported');
 const ERC165InterfacesSupported = artifacts.require('ERC165InterfacesSupported');
+const ERC165ReturnBombMock = artifacts.require('ERC165ReturnBombMock');
 
 const DUMMY_ID = '0xdeadbeef';
 const DUMMY_ID_2 = '0xcafebabe';
@@ -280,4 +281,23 @@ contract('ERC165Checker', function (accounts) {
       expect(supported).to.equal(false);
     });
   });
+
+  it('Return bomb resistance', async function () {
+    this.target = await ERC165ReturnBombMock.new();
+
+    const tx1 = await this.mock.supportsInterface.sendTransaction(this.target.address, DUMMY_ID);
+    expect(tx1.receipt.gasUsed).to.be.lessThan(120000); // 3*30k + 21k + some margin
+
+    const tx2 = await this.mock.getSupportedInterfaces.sendTransaction(
+      this.target.address,
+      [
+        DUMMY_ID,
+        DUMMY_ID_2,
+        DUMMY_ID_3,
+        DUMMY_UNSUPPORTED_ID,
+        DUMMY_UNSUPPORTED_ID_2,
+      ],
+    );
+    expect(tx2.receipt.gasUsed).to.be.lessThan(250000); // (2+5)*30k + 21k + some margin
+  });
 });