Fix issues caused by abi.decode reverting (#3552)

Co-authored-by: Francisco Giordano <[email protected]>

Author: Amxx

CHANGELOG.md

@@ -15,6 +15,11 @@
 
 ERC-721 integrators that interpret contract state from events should make sure that they implement the clearing of approval that is implicit in every transfer according to the EIP. Previous versions of OpenZeppellin Contracts emitted an explicit `Approval` event even though it was not required by the specification, and this is no longer the case.
 
+## 4.7.1
+
+ * `SignatureChecker`: Fix an issue that causes `isValidSignatureNow` to revert when the target contract returns ill-encoded data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552))
+ * `ERC165Checker`: Fix an issue that causes `supportsInterface` to revert when the target contract returns ill-encoded data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552))
+
 ## 4.7.0 (2022-06-29)
 
  * `TimelockController`: Migrate `_call` to `_execute` and allow inheritance and overriding similar to `Governor`. ([#3317](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3317))

contracts/mocks/ERC1271WalletMock.sol

@@ -15,3 +15,12 @@ contract ERC1271WalletMock is Ownable, IERC1271 {
         return ECDSA.recover(hash, signature) == owner() ? this.isValidSignature.selector : bytes4(0);
     }
 }
+
+contract ERC1271MaliciousMock is IERC1271 {
+    function isValidSignature(bytes32, bytes memory) public pure override returns (bytes4) {
+        assembly {
+            mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
+            return(0, 32)
+        }
+    }
+}

contracts/mocks/ERC165/ERC165MaliciousData.sol

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+contract ERC165MaliciousData {
+    function supportsInterface(bytes4) public view returns (bool) {
+        assembly {
+            mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
+            return(0, 32)
+        }
+    }
+}

contracts/utils/cryptography/SignatureChecker.sol

@@ -35,6 +35,8 @@ library SignatureChecker {
         (bool success, bytes memory result) = signer.staticcall(
             abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)
         );
-        return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector);
+        return (success &&
+            result.length == 32 &&
+            abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
     }
 }

contracts/utils/introspection/ERC165Checker.sol

@@ -108,6 +108,6 @@ library ERC165Checker {
         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, (bool));
+        return success && abi.decode(result, (uint256)) > 0;
     }
 }

test/utils/cryptography/SignatureChecker.test.js

@@ -4,6 +4,7 @@ const { expect } = require('chai');
 
 const SignatureCheckerMock = artifacts.require('SignatureCheckerMock');
 const ERC1271WalletMock = artifacts.require('ERC1271WalletMock');
+const ERC1271MaliciousMock = artifacts.require('ERC1271MaliciousMock');
 
 const TEST_MESSAGE = web3.utils.sha3('OpenZeppelin');
 const WRONG_MESSAGE = web3.utils.sha3('Nope');
@@ -14,6 +15,7 @@ contract('SignatureChecker (ERC1271)', function (accounts) {
   before('deploying', async function () {
     this.signaturechecker = await SignatureCheckerMock.new();
     this.wallet = await ERC1271WalletMock.new(signer);
+    this.malicious = await ERC1271MaliciousMock.new();
     this.signature = await web3.eth.sign(TEST_MESSAGE, signer);
   });
 
@@ -67,5 +69,13 @@ contract('SignatureChecker (ERC1271)', function (accounts) {
         this.signature,
       )).to.equal(false);
     });
+
+    it('with malicious wallet', async function () {
+      expect(await this.signaturechecker.isValidSignatureNow(
+        this.malicious.address,
+        toEthSignedMessageHash(TEST_MESSAGE),
+        this.signature,
+      )).to.equal(false);
+    });
   });
 });

test/utils/introspection/ERC165Checker.test.js

@@ -4,6 +4,7 @@ const { expect } = require('chai');
 
 const ERC165CheckerMock = artifacts.require('ERC165CheckerMock');
 const ERC165MissingData = artifacts.require('ERC165MissingData');
+const ERC165MaliciousData = artifacts.require('ERC165MaliciousData');
 const ERC165NotSupported = artifacts.require('ERC165NotSupported');
 const ERC165InterfacesSupported = artifacts.require('ERC165InterfacesSupported');
 
@@ -51,6 +52,38 @@ contract('ERC165Checker', function (accounts) {
     });
   });
 
+  context('ERC165 malicious return data', function () {
+    beforeEach(async function () {
+      this.target = await ERC165MaliciousData.new();
+    });
+
+    it('does not support ERC165', async function () {
+      const supported = await this.mock.supportsERC165(this.target.address);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via supportsInterface', async function () {
+      const supported = await this.mock.supportsInterface(this.target.address, DUMMY_ID);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via supportsAllInterfaces', async function () {
+      const supported = await this.mock.supportsAllInterfaces(this.target.address, [DUMMY_ID]);
+      expect(supported).to.equal(false);
+    });
+
+    it('does not support mock interface via getSupportedInterfaces', async function () {
+      const supported = await this.mock.getSupportedInterfaces(this.target.address, [DUMMY_ID]);
+      expect(supported.length).to.equal(1);
+      expect(supported[0]).to.equal(false);
+    });
+
+    it('does not support mock interface via supportsERC165InterfaceUnchecked', async function () {
+      const supported = await this.mock.supportsERC165InterfaceUnchecked(this.target.address, DUMMY_ID);
+      expect(supported).to.equal(true);
+    });
+  });
+
   context('ERC165 not supported', function () {
     beforeEach(async function () {
       this.target = await ERC165NotSupported.new();