diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index c3d045f0ee8..8412c990f38 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,25 @@ 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 + + // If one of the signatures is invalid, reject the batch + if (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false; + bytes32 id = keccak256(signer); - if (previousId >= id || !isValidERC7913SignatureNow(signer, hash, signatures[i])) { - return false; + // If the current signer ID is greater than all previous IDs, then 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; + } } - - previousId = id; } 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(