diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b846bde107..3419465e863 100644 --- a/CHANGELOG.md +++ b/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)) 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 6a240e15531..c1b9f69bbd8 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 c3a6cdc66e4..a0908ecb2d9 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'); @@ -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();