From 628a6e2866620fe638fc3dd77419b5572d3b87d7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 18 Jul 2022 23:01:20 +0200 Subject: [PATCH] Fix issues caused by abi.decode reverting (#3552) Co-authored-by: Francisco Giordano --- CHANGELOG.md | 5 +++ contracts/mocks/ERC1271WalletMock.sol | 9 +++++ .../mocks/ERC165/ERC165MaliciousData.sol | 12 +++++++ .../utils/cryptography/SignatureChecker.sol | 4 ++- .../utils/introspection/ERC165Checker.sol | 2 +- .../cryptography/SignatureChecker.test.js | 10 ++++++ .../utils/introspection/ERC165Checker.test.js | 33 +++++++++++++++++++ 7 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 contracts/mocks/ERC165/ERC165MaliciousData.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index e1d0da2e011..9a5df7da5c8 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/contracts/mocks/ERC1271WalletMock.sol b/contracts/mocks/ERC1271WalletMock.sol index c92acdba63e..015288998df 100644 --- a/contracts/mocks/ERC1271WalletMock.sol +++ b/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) + } + } +} diff --git a/contracts/mocks/ERC165/ERC165MaliciousData.sol b/contracts/mocks/ERC165/ERC165MaliciousData.sol new file mode 100644 index 00000000000..40b6f9813c5 --- /dev/null +++ b/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) + } + } +} diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 3ed6e719a0b..42b222a0f3b 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/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)); } } diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index c936d3f31ae..4cdf342059d 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/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; } } diff --git a/test/utils/cryptography/SignatureChecker.test.js b/test/utils/cryptography/SignatureChecker.test.js index 6c99e3cf42e..801377a95e7 100644 --- a/test/utils/cryptography/SignatureChecker.test.js +++ b/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); + }); }); }); diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js index c325adb7e0f..5c65bfde2e2 100644 --- a/test/utils/introspection/ERC165Checker.test.js +++ b/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();