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

Co-authored-by: Francisco Giordano <[email protected]>
(cherry picked from commit 628a6e2)

Author: Amxx

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## 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');
 
@@ -46,6 +47,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();