From 264db558ef2775c9cffdb77568332cc89a7fd3a9 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 15 Jul 2022 11:47:27 +0200 Subject: [PATCH 1/7] Fix issue causing SignatureChecker to revert on invalid response format --- contracts/mocks/ERC1271WalletMock.sol | 9 +++++++++ contracts/utils/cryptography/SignatureChecker.sol | 4 +++- test/utils/cryptography/SignatureChecker.test.js | 10 ++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) 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/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/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); + }); }); }); From e68e6c262943e7f3b302450b4a4066f0f47eb21c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 15 Jul 2022 18:58:49 +0200 Subject: [PATCH 2/7] Fix issue causing supportsERC165InterfaceUnchecked to revert on invalid response format --- .../mocks/ERC165/ERC165MaliciousData.sol | 12 +++++++ .../utils/introspection/ERC165Checker.sol | 2 +- .../utils/introspection/ERC165Checker.test.js | 33 +++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 contracts/mocks/ERC165/ERC165MaliciousData.sol diff --git a/contracts/mocks/ERC165/ERC165MaliciousData.sol b/contracts/mocks/ERC165/ERC165MaliciousData.sol new file mode 100644 index 00000000000..4c029416e5c --- /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 interfaceId) public view returns (bool) { + assembly { + mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) + return(0, 32) + } + } +} diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index c936d3f31ae..2571e2ab604 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)) == 1; } } diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js index c325adb7e0f..ed2f0d65452 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(false); + }); + }); + context('ERC165 not supported', function () { beforeEach(async function () { this.target = await ERC165NotSupported.new(); From eabf46ce4cad739e04cb20f9a33525a655ad112e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 15 Jul 2022 19:06:23 +0200 Subject: [PATCH 3/7] add changelog entry --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06409f694c0..b801027fa9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) * `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481)) +### Bugfixes + * `SignatureChecker`: fix an issue that cause `isValidSignatureNow` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) + * `ERC165Checker`: fix an issue that cause `supportsERC165InterfaceUnchecked` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) + + ### Compatibility Note 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. From 2e0e4e10ac3408e76ecbb6163d44f57ba9133a51 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 15 Jul 2022 19:10:49 +0200 Subject: [PATCH 4/7] fix lint --- CHANGELOG.md | 1 - contracts/mocks/ERC165/ERC165MaliciousData.sol | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b801027fa9a..65de195f7f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,6 @@ * `SignatureChecker`: fix an issue that cause `isValidSignatureNow` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) * `ERC165Checker`: fix an issue that cause `supportsERC165InterfaceUnchecked` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) - ### Compatibility Note 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. diff --git a/contracts/mocks/ERC165/ERC165MaliciousData.sol b/contracts/mocks/ERC165/ERC165MaliciousData.sol index 4c029416e5c..40b6f9813c5 100644 --- a/contracts/mocks/ERC165/ERC165MaliciousData.sol +++ b/contracts/mocks/ERC165/ERC165MaliciousData.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; contract ERC165MaliciousData { - function supportsInterface(bytes4 interfaceId) public view returns (bool) { + function supportsInterface(bytes4) public view returns (bool) { assembly { mstore(0, 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) return(0, 32) From 4674de2f1bd53840fd517c778c6ac93044377fbf Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 17:17:37 -0300 Subject: [PATCH 5/7] move changelog entries and rephrase --- CHANGELOG.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65de195f7f9..711a04c0363 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,14 +9,15 @@ * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) * `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481)) -### Bugfixes - * `SignatureChecker`: fix an issue that cause `isValidSignatureNow` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) - * `ERC165Checker`: fix an issue that cause `supportsERC165InterfaceUnchecked` to revert when processing invalid return data. ([#3552](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3552)) - ### Compatibility Note 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)) From 0a5f7b25ce1aff269211e08a73a11542a83e5795 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 17:22:20 -0300 Subject: [PATCH 6/7] use all nonzero values as true --- contracts/utils/introspection/ERC165Checker.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 2571e2ab604..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, (uint256)) == 1; + return success && abi.decode(result, (uint256)) > 0; } } From 43698a3d77668affa333eb1870db7db848f92c26 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Mon, 18 Jul 2022 17:49:18 -0300 Subject: [PATCH 7/7] fix test --- test/utils/introspection/ERC165Checker.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/introspection/ERC165Checker.test.js b/test/utils/introspection/ERC165Checker.test.js index ed2f0d65452..5c65bfde2e2 100644 --- a/test/utils/introspection/ERC165Checker.test.js +++ b/test/utils/introspection/ERC165Checker.test.js @@ -80,7 +80,7 @@ contract('ERC165Checker', function (accounts) { 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(false); + expect(supported).to.equal(true); }); });