From c2ecf9124c057d1891890dd74e9634266c1e8a00 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Jun 2025 14:59:53 +0200 Subject: [PATCH 1/5] Add fallback to check uniqueness if signers are not ordered --- .../utils/cryptography/SignatureChecker.sol | 21 ++-- .../cryptography/SignatureChecker.test.js | 119 +++++++++++------- 2 files changed, 85 insertions(+), 55 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index c3d045f0ee8..0c14673e234 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -95,8 +95,8 @@ library SignatureChecker { /** * @dev Verifies multiple ERC-7913 `signatures` for a given `hash` using a set of `signers`. * - * The signers must be ordered by their `keccak256` hash to ensure no duplicates and to optimize - * the verification process. The function will return `false` if the signers are not properly ordered. + * The signers should be ordered by their `keccak256` hash to ensure efficient duplication check. Unordered + * signers are supported, but the uniqueness check will be more expensive. * * Requirements: * @@ -110,17 +110,20 @@ library SignatureChecker { bytes[] memory signers, bytes[] memory signatures ) internal view returns (bool) { - bytes32 previousId = bytes32(0); + bytes32 lastId = bytes32(0); for (uint256 i = 0; i < signers.length; ++i) { bytes memory signer = signers[i]; - // Signers must ordered by id to ensure no duplicates - bytes32 id = keccak256(signer); - if (previousId >= id || !isValidERC7913SignatureNow(signer, hash, signatures[i])) { - return false; - } - previousId = id; + if (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false; + + bytes32 id = keccak256(signer); + if (lastId < id) { + lastId = id; + } else + for (uint256 j = 0; j < i; ++j) { + if (id == keccak256(signers[j])) return false; + } } return true; diff --git a/test/utils/cryptography/SignatureChecker.test.js b/test/utils/cryptography/SignatureChecker.test.js index 51d23539546..b141ab9bf5e 100644 --- a/test/utils/cryptography/SignatureChecker.test.js +++ b/test/utils/cryptography/SignatureChecker.test.js @@ -191,47 +191,90 @@ describe('SignatureChecker (ERC1271)', function () { it('should validate multiple signatures with different signer types', async function () { const signature = await this.signer.signMessage(TEST_MESSAGE); const pairs = [ - [ethers.zeroPadValue(this.signer.address, 20), signature], - [ethers.zeroPadValue(this.wallet.target, 20), signature], - [ethers.concat([this.verifier.target, VALID_SW_KEY_1]), VALID_SW_SIGNATURE_1], - ].sort(([a], [b]) => ethers.keccak256(a) - ethers.keccak256(b)); - const signers = pairs.map(([signer]) => signer); - const signatures = pairs.map(([, signature]) => signature); - await expect(this.mock.$areValidERC7913SignaturesNow(TEST_MESSAGE_HASH, signers, signatures)).to.eventually.be - .true; + { signer: ethers.zeroPadValue(this.signer.address, 20), signature }, + { signer: ethers.zeroPadValue(this.wallet.target, 20), signature }, + { signer: ethers.concat([this.verifier.target, VALID_SW_KEY_1]), signature: VALID_SW_SIGNATURE_1 }, + ].sort(({ signer: a }, { signer: b }) => ethers.keccak256(a) - ethers.keccak256(b)); + + await expect( + this.mock.$areValidERC7913SignaturesNow( + TEST_MESSAGE_HASH, + pairs.map(({ signer }) => signer), + pairs.map(({ signature }) => signature), + ), + ).to.eventually.be.true; }); it('should validate multiple EOA signatures', async function () { const pairs = [ - [ethers.zeroPadValue(this.signer.address, 20), await this.signer.signMessage(TEST_MESSAGE)], - [ethers.zeroPadValue(this.extraSigner.address, 20), await this.extraSigner.signMessage(TEST_MESSAGE)], - ].sort(([a], [b]) => ethers.keccak256(a) - ethers.keccak256(b)); - const signers = pairs.map(([signer]) => signer); - const signatures = pairs.map(([, signature]) => signature); - await expect(this.mock.$areValidERC7913SignaturesNow(TEST_MESSAGE_HASH, signers, signatures)).to.eventually.be - .true; + { + signer: ethers.zeroPadValue(this.signer.address, 20), + signature: await this.signer.signMessage(TEST_MESSAGE), + }, + { + signer: ethers.zeroPadValue(this.extraSigner.address, 20), + signature: await this.extraSigner.signMessage(TEST_MESSAGE), + }, + ].sort(({ signer: a }, { signer: b }) => ethers.keccak256(a) - ethers.keccak256(b)); + + await expect( + this.mock.$areValidERC7913SignaturesNow( + TEST_MESSAGE_HASH, + pairs.map(({ signer }) => signer), + pairs.map(({ signature }) => signature), + ), + ).to.eventually.be.true; }); it('should validate multiple ERC-1271 wallet signatures', async function () { const pairs = [ - [ethers.zeroPadValue(this.wallet.target, 20), await this.signer.signMessage(TEST_MESSAGE)], - [ethers.zeroPadValue(this.wallet2.target, 20), await this.extraSigner.signMessage(TEST_MESSAGE)], - ].sort(([a], [b]) => ethers.keccak256(a) - ethers.keccak256(b)); - const signers = pairs.map(([signer]) => signer); - const signatures = pairs.map(([, signature]) => signature); - await expect(this.mock.$areValidERC7913SignaturesNow(TEST_MESSAGE_HASH, signers, signatures)).to.eventually.be - .true; + { + signer: ethers.zeroPadValue(this.wallet.target, 20), + signature: await this.signer.signMessage(TEST_MESSAGE), + }, + { + signer: ethers.zeroPadValue(this.wallet2.target, 20), + signature: await this.extraSigner.signMessage(TEST_MESSAGE), + }, + ].sort(({ signer: a }, { signer: b }) => ethers.keccak256(a) - ethers.keccak256(b)); + + await expect( + this.mock.$areValidERC7913SignaturesNow( + TEST_MESSAGE_HASH, + pairs.map(({ signer }) => signer), + pairs.map(({ signature }) => signature), + ), + ).to.eventually.be.true; + }); + + it('should validate multiple ERC-7913 signatures (ordered by ID)', async function () { + const pairs = [ + { signer: ethers.concat([this.verifier.target, VALID_SW_KEY_1]), signature: VALID_SW_SIGNATURE_1 }, + { signer: ethers.concat([this.verifier.target, VALID_SW_KEY_2]), signature: VALID_SW_SIGNATURE_2 }, + ].sort(({ signer: a }, { signer: b }) => ethers.keccak256(a) - ethers.keccak256(b)); + + await expect( + this.mock.$areValidERC7913SignaturesNow( + TEST_MESSAGE_HASH, + pairs.map(({ signer }) => signer), + pairs.map(({ signature }) => signature), + ), + ).to.eventually.be.true; }); - it('should validate multiple ERC-7913 signatures', async function () { + it('should validate multiple ERC-7913 signatures (unordered)', async function () { const pairs = [ - [ethers.concat([this.verifier.target, VALID_SW_KEY_1]), VALID_SW_SIGNATURE_1], - [ethers.concat([this.verifier.target, VALID_SW_KEY_2]), VALID_SW_SIGNATURE_2], - ].sort(([a], [b]) => ethers.keccak256(a) - ethers.keccak256(b)); - const signers = pairs.map(([signer]) => signer); - const signatures = pairs.map(([, signature]) => signature); - await expect(this.mock.$areValidERC7913SignaturesNow(TEST_MESSAGE_HASH, signers, signatures)).to.eventually.be - .true; + { signer: ethers.concat([this.verifier.target, VALID_SW_KEY_1]), signature: VALID_SW_SIGNATURE_1 }, + { signer: ethers.concat([this.verifier.target, VALID_SW_KEY_2]), signature: VALID_SW_SIGNATURE_2 }, + ].sort(({ signer: a }, { signer: b }) => ethers.keccak256(b) - ethers.keccak256(a)); // reverse + + await expect( + this.mock.$areValidERC7913SignaturesNow( + TEST_MESSAGE_HASH, + pairs.map(({ signer }) => signer), + pairs.map(({ signature }) => signature), + ), + ).to.eventually.be.true; }); it('should return false if any signature is invalid', async function () { @@ -244,22 +287,6 @@ describe('SignatureChecker (ERC1271)', function () { ).to.eventually.be.false; }); - it('should return false if signers are not ordered by ID', async function () { - const pairs = [ - [ethers.zeroPadValue(this.signer.address, 20), await this.signer.signMessage(TEST_MESSAGE)], - [ethers.zeroPadValue(this.extraSigner.address, 20), await this.extraSigner.signMessage(TEST_MESSAGE)], - ]; - - if (ethers.keccak256(pairs[0][0]) < ethers.keccak256(pairs[1][0])) { - pairs.reverse(); - } - - const signers = pairs.map(([signer]) => signer); - const signatures = pairs.map(([, signature]) => signature); - await expect(this.mock.$areValidERC7913SignaturesNow(TEST_MESSAGE_HASH, signers, signatures)).to.eventually.be - .false; - }); - it('should return false if there are duplicate signers', async function () { await expect( this.mock.$areValidERC7913SignaturesNow( From d3de5ca31f1489e346e39cad9800e5e6175b0846 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Jun 2025 15:43:00 +0200 Subject: [PATCH 2/5] lint --- contracts/utils/cryptography/SignatureChecker.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 0c14673e234..bb5641dcff3 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -120,10 +120,11 @@ library SignatureChecker { bytes32 id = keccak256(signer); if (lastId < id) { lastId = id; - } else + } else { for (uint256 j = 0; j < i; ++j) { if (id == keccak256(signers[j])) return false; } + } } return true; From fd2c32335400c9ced1e0b3d23c84355b94eec7aa Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Jun 2025 18:51:02 +0200 Subject: [PATCH 3/5] Apply suggestions from code review --- contracts/utils/cryptography/SignatureChecker.sol | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index bb5641dcff3..760c501d3d5 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -118,9 +118,12 @@ library SignatureChecker { if (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false; bytes32 id = keccak256(signer); + // if the current signer id is greater than all previous signer ids, the this is a new signer. if (lastId < id) { lastId = id; } else { + // If this signer id is not greater than all the previous ones, verify that it is not a duplicate of a previous one + // This loop is never executed if the signers are ordered by id. for (uint256 j = 0; j < i; ++j) { if (id == keccak256(signers[j])) return false; } From 26f0da5999d61591c9fa66e2af7c5c3d5fe3c096 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Jun 2025 18:51:43 +0200 Subject: [PATCH 4/5] Update contracts/utils/cryptography/SignatureChecker.sol --- 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 760c501d3d5..8e3805284ba 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -118,7 +118,7 @@ library SignatureChecker { if (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false; bytes32 id = keccak256(signer); - // if the current signer id is greater than all previous signer ids, the this is a new signer. + // If the current signer ID is greater than all previous IDs, then this is a new signer. if (lastId < id) { lastId = id; } else { From a7029b02ae2061aaa690ac10ed184fa5f2c530d0 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 4 Jun 2025 18:52:07 +0200 Subject: [PATCH 5/5] Update contracts/utils/cryptography/SignatureChecker.sol --- contracts/utils/cryptography/SignatureChecker.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index 8e3805284ba..8412c990f38 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -115,6 +115,7 @@ library SignatureChecker { for (uint256 i = 0; i < signers.length; ++i) { bytes memory signer = signers[i]; + // If one of the signatures is invalid, reject the batch if (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false; bytes32 id = keccak256(signer);