From e814665e7a926a8a8df9beaa2c381570b3ae91f6 Mon Sep 17 00:00:00 2001 From: YamenMerhi Date: Thu, 5 Jan 2023 19:10:52 +0200 Subject: [PATCH 01/11] Add `isValidERC1271SignatureNow` function in SignatureChecker --- .../utils/cryptography/SignatureChecker.sol | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index e06778debd7..5d99fa3616b 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -27,9 +27,26 @@ library SignatureChecker { return true; } + return isValidERC1271SignatureNow(signer,hash,signature); + } + + + /** + * @dev Checks if a signature is valid for a given signer and data hash. The signature is validated + * against the signer smart contract using ERC1271. + * + * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus + * change through time. It could return true at block N and false at block N+1 (or the opposite). + */ + function isValidERC1271SignatureNow( + address signer, + bytes32 hash, + bytes memory signature + ) internal view returns (bool) { (bool success, bytes memory result) = signer.staticcall( abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature) ); + return (success && result.length == 32 && abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector)); From d8139cd9457afafd7baaf1bbc18f8a6d90124848 Mon Sep 17 00:00:00 2001 From: YamenMerhi Date: Thu, 5 Jan 2023 19:11:03 +0200 Subject: [PATCH 02/11] Add tests for the added function --- .../cryptography/SignatureChecker.test.js | 112 ++++++++++++------ 1 file changed, 78 insertions(+), 34 deletions(-) diff --git a/test/utils/cryptography/SignatureChecker.test.js b/test/utils/cryptography/SignatureChecker.test.js index 11054c3e1b4..1dc3f0c1dcb 100644 --- a/test/utils/cryptography/SignatureChecker.test.js +++ b/test/utils/cryptography/SignatureChecker.test.js @@ -40,44 +40,88 @@ contract('SignatureChecker (ERC1271)', function (accounts) { }); context('ERC1271 wallet', function () { - it('with matching signer and signature', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.wallet.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(true); - }); + context('when testing isValidERC1271SignatureNow', function () { + it('with matching signer and signature', async function () { + expect( + await this.signaturechecker.$isValidERC1271SignatureNow( + this.wallet.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(true); + }); - it('with invalid signer', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.signaturechecker.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(false); - }); + it('with invalid signer', async function () { + expect( + await this.signaturechecker.$isValidERC1271SignatureNow( + this.signaturechecker.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); - it('with invalid signature', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.wallet.address, - toEthSignedMessageHash(WRONG_MESSAGE), - this.signature, - ), - ).to.equal(false); + it('with invalid signature', async function () { + expect( + await this.signaturechecker.$isValidERC1271SignatureNow( + this.wallet.address, + toEthSignedMessageHash(WRONG_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); + + it('with malicious wallet', async function () { + expect( + await this.signaturechecker.$isValidERC1271SignatureNow( + this.malicious.address, + toEthSignedMessageHash(TEST_MESSAGE), + 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); + context('when testing isValidSignatureNow', function () { + it('with matching signer and signature', async function () { + expect( + await this.signaturechecker.$isValidSignatureNow( + this.wallet.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(true); + }); + + it('with invalid signer', async function () { + expect( + await this.signaturechecker.$isValidSignatureNow( + this.signaturechecker.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); + + it('with invalid signature', async function () { + expect( + await this.signaturechecker.$isValidSignatureNow( + this.wallet.address, + toEthSignedMessageHash(WRONG_MESSAGE), + 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 c11f0a317b609a02f0d24ef394b3455e0edee3dd Mon Sep 17 00:00:00 2001 From: YamenMerhi Date: Thu, 5 Jan 2023 19:11:12 +0200 Subject: [PATCH 03/11] Add the changes to the changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f6a9be4002..c090a617ff7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * `Strings`: add `toString` method for signed integers. ([#3773](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3773)) * `MerkleProof`: optimize by using unchecked arithmetic. ([#3745](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3745)) * `EnumerableMap`: add a `keys()` function that returns an array containing all the keys. ([#3920](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3920)) + * `SignatureChecker`: add `isValidERC1271SignatureNow` for checking the hash and THE signature against a smart contract using ERC1271. ([#3931](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3931)) ### Deprecations From 76388986e0aa796e966b389f1c02a96f5e827c02 Mon Sep 17 00:00:00 2001 From: Yamen Merhi Date: Thu, 5 Jan 2023 19:13:18 +0200 Subject: [PATCH 04/11] modify the PR link in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c090a617ff7..bc9fdad7ab7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ * `Strings`: add `toString` method for signed integers. ([#3773](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3773)) * `MerkleProof`: optimize by using unchecked arithmetic. ([#3745](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3745)) * `EnumerableMap`: add a `keys()` function that returns an array containing all the keys. ([#3920](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3920)) - * `SignatureChecker`: add `isValidERC1271SignatureNow` for checking the hash and THE signature against a smart contract using ERC1271. ([#3931](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3931)) + * `SignatureChecker`: add `isValidERC1271SignatureNow` for checking the hash and the signature against a smart contract using ERC1271. ([#3932](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3932)) ### Deprecations From 8a784530405ef41389c44cbeaf6e2b099eede444 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 6 Jan 2023 14:06:07 +0100 Subject: [PATCH 05/11] Update SignatureChecker.sol --- contracts/utils/cryptography/SignatureChecker.sol | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 5d99fa3616b..24feed18c14 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -23,11 +23,9 @@ library SignatureChecker { */ function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); - if (error == ECDSA.RecoverError.NoError && recovered == signer) { - return true; - } - - return isValidERC1271SignatureNow(signer,hash,signature); + + return (error == ECDSA.RecoverError.NoError && recovered == signer) + || isValidERC1271SignatureNow(signer,hash,signature); } From a0bda75ef0a4103f1e0cb743603c2c5036ff8a37 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 6 Jan 2023 14:07:01 +0100 Subject: [PATCH 06/11] Update SignatureChecker.sol --- contracts/utils/cryptography/SignatureChecker.sol | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 24feed18c14..3bc94a1a205 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -23,9 +23,8 @@ library SignatureChecker { */ function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); - - return (error == ECDSA.RecoverError.NoError && recovered == signer) - || isValidERC1271SignatureNow(signer,hash,signature); + return (error == ECDSA.RecoverError.NoError && recovered == signer) || + isValidERC1271SignatureNow(signer,hash,signature); } @@ -44,7 +43,6 @@ library SignatureChecker { (bool success, bytes memory result) = signer.staticcall( abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature) ); - return (success && result.length == 32 && abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector)); From 4ccf3d9b8cebb88faab592e7589ae3e2085cc070 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 12 Jan 2023 16:27:33 +0100 Subject: [PATCH 07/11] test factorisation --- .../cryptography/SignatureChecker.test.js | 124 ++++++------------ 1 file changed, 42 insertions(+), 82 deletions(-) diff --git a/test/utils/cryptography/SignatureChecker.test.js b/test/utils/cryptography/SignatureChecker.test.js index 1dc3f0c1dcb..ba8b100d1a2 100644 --- a/test/utils/cryptography/SignatureChecker.test.js +++ b/test/utils/cryptography/SignatureChecker.test.js @@ -40,88 +40,48 @@ contract('SignatureChecker (ERC1271)', function (accounts) { }); context('ERC1271 wallet', function () { - context('when testing isValidERC1271SignatureNow', function () { - it('with matching signer and signature', async function () { - expect( - await this.signaturechecker.$isValidERC1271SignatureNow( - this.wallet.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(true); + for (const signature of ['isValidERC1271SignatureNow', 'isValidSignatureNow']) { + context(signature, function () { + it('with matching signer and signature', async function () { + expect( + await this.signaturechecker[`$${signature}`]( + this.wallet.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(true); + }); + + it('with invalid signer', async function () { + expect( + await this.signaturechecker[`$${signature}`]( + this.signaturechecker.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); + + it('with invalid signature', async function () { + expect( + await this.signaturechecker[`$${signature}`]( + this.wallet.address, + toEthSignedMessageHash(WRONG_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); + + it('with malicious wallet', async function () { + expect( + await this.signaturechecker[`$${signature}`]( + this.malicious.address, + toEthSignedMessageHash(TEST_MESSAGE), + this.signature, + ), + ).to.equal(false); + }); }); - - it('with invalid signer', async function () { - expect( - await this.signaturechecker.$isValidERC1271SignatureNow( - this.signaturechecker.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(false); - }); - - it('with invalid signature', async function () { - expect( - await this.signaturechecker.$isValidERC1271SignatureNow( - this.wallet.address, - toEthSignedMessageHash(WRONG_MESSAGE), - this.signature, - ), - ).to.equal(false); - }); - - it('with malicious wallet', async function () { - expect( - await this.signaturechecker.$isValidERC1271SignatureNow( - this.malicious.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(false); - }); - }); - - context('when testing isValidSignatureNow', function () { - it('with matching signer and signature', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.wallet.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(true); - }); - - it('with invalid signer', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.signaturechecker.address, - toEthSignedMessageHash(TEST_MESSAGE), - this.signature, - ), - ).to.equal(false); - }); - - it('with invalid signature', async function () { - expect( - await this.signaturechecker.$isValidSignatureNow( - this.wallet.address, - toEthSignedMessageHash(WRONG_MESSAGE), - 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 0e86f7810ce7f7124717275a8c86b98c6816fabb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 23 Jan 2023 10:54:24 +0100 Subject: [PATCH 08/11] add changeset --- .changeset/slimy-knives-hug.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/slimy-knives-hug.md diff --git a/.changeset/slimy-knives-hug.md b/.changeset/slimy-knives-hug.md new file mode 100644 index 00000000000..970da5c5bf4 --- /dev/null +++ b/.changeset/slimy-knives-hug.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`SignatureChecker`: add `isValidERC1271SignatureNow` for checking the hash and the signature against a smart contract using ERC1271. From 6cbcad01327a8b84888dc7f1fa239eb023a18551 Mon Sep 17 00:00:00 2001 From: Francisco Date: Tue, 21 Feb 2023 11:47:11 -0300 Subject: [PATCH 09/11] whitespace --- contracts/utils/cryptography/SignatureChecker.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 3bc94a1a205..a2334d9bd21 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -24,7 +24,7 @@ library SignatureChecker { function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); return (error == ECDSA.RecoverError.NoError && recovered == signer) || - isValidERC1271SignatureNow(signer,hash,signature); + isValidERC1271SignatureNow(signer, hash, signature); } From a58e95f2e479b019607f36a3dea58322300d4f0f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Feb 2023 11:50:35 -0300 Subject: [PATCH 10/11] lint --- contracts/utils/cryptography/SignatureChecker.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index a2334d9bd21..c3a724dd046 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -23,11 +23,11 @@ library SignatureChecker { */ function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); - return (error == ECDSA.RecoverError.NoError && recovered == signer) || + return + (error == ECDSA.RecoverError.NoError && recovered == signer) || isValidERC1271SignatureNow(signer, hash, signature); } - /** * @dev Checks if a signature is valid for a given signer and data hash. The signature is validated * against the signer smart contract using ERC1271. From ecc103a6b53bf0044c5f013d8d707466c98f3322 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 21 Feb 2023 11:51:49 -0300 Subject: [PATCH 11/11] changeset wording --- .changeset/slimy-knives-hug.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/slimy-knives-hug.md b/.changeset/slimy-knives-hug.md index 970da5c5bf4..94099eea7f6 100644 --- a/.changeset/slimy-knives-hug.md +++ b/.changeset/slimy-knives-hug.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -`SignatureChecker`: add `isValidERC1271SignatureNow` for checking the hash and the signature against a smart contract using ERC1271. +`SignatureChecker`: Add `isValidERC1271SignatureNow` for checking a signature directly against a smart contract using ERC-1271.