Skip to content

Add fallback to check uniqueness if signers are not ordered #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ernestognw merged 5 commits into from
Jun 4, 2025
Merged

Add fallback to check uniqueness if signers are not ordered #56

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2ecf91
Add fallback to check uniqueness if signers are not ordered
Amxx Jun 4, 2025
d3de5ca
lint
Amxx Jun 4, 2025
fd2c323
Apply suggestions from code review
Amxx Jun 4, 2025
26f0da5
Update contracts/utils/cryptography/SignatureChecker.sol
Amxx Jun 4, 2025
a7029b0
Update contracts/utils/cryptography/SignatureChecker.sol
Amxx Jun 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions contracts/utils/cryptography/SignatureChecker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
*
Expand All @@ -110,17 +110,21 @@ 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 (!isValidERC7913SignatureNow(signer, hash, signatures[i])) return false;

bytes32 id = keccak256(signer);
if (previousId >= id || !isValidERC7913SignatureNow(signer, hash, signatures[i])) {
return false;
if (lastId < id) {
lastId = id;
} else {
for (uint256 j = 0; j < i; ++j) {
if (id == keccak256(signers[j])) return false;
}
}

previousId = id;
}

return true;
Expand Down
119 changes: 73 additions & 46 deletions test/utils/cryptography/SignatureChecker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -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(
Expand Down
Loading