fix(multisig): reject non-adjacent duplicate signers in EcdsaSignerManager by Yonkoo11 · Pull Request #676 · OpenZeppelin/compact-contracts

Yonkoo11 · 2026-07-14T07:06:20Z

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Fixes #629

Summary

EcdsaSignerManager.verify only rejected adjacent duplicate commitments:

assert(commitment != state.prevCommitment, "EcdsaSignerManager: duplicate signer");

For n >= 3, a presentation like [A, B, A] passed every check and counted the same signer twice toward the threshold (forged quorum). The module already documented that the adjacent check was only sufficient for at most 2 signers; this PR implements the sorted / strictly-increasing fix noted in #629 and in that notice.

Approach

Require presented commitments to be strictly increasing under Compact's Bytes→integer embedding (first byte is LSB — the same order as Bytes as Field):

Zero sentinel as prevCommitment for the first iteration.
Each next commitment must be strictly greater than the previous.
Equals (adjacent or not) fail with EcdsaSignerManager: duplicate or unsorted signer.

Why not as Field as Uint<248>? Full 32-byte hash commitments often exceed Compact's max Uint (2^248 - 1). That cast compiles but panics at runtime. Comparison uses two 16-byte little-endian limbs as Uint<128> (always in range):

(aHigh > bHigh) || ((aHigh == bHigh) && (aLow > bLow))

Call-site impact (why "breaking" is checked)

Semantics for distinct unsorted pairs change: for n >= 2, callers must present pubkeys/signatures sorted by ascending commitment (first-byte-LSB order). Previously any order of two distinct keys worked.
Current shipped call sites on this branch use verify<2> only; they remain safe if they sort (or already present in ascending order).
Latent bug for any verify with n >= 3 is fixed.

Tests

Mock exposes verify3 for a 3-of-N presentation path.
New cases: three distinct sorted signers pass; [A, B, A] rejects; adjacent duplicate in a triple rejects.
Existing adjacent-duplicate / membership tests updated to sort when presenting distinct keys.
Verified locally: SKIP_ZK=true yarn vitest run src/multisig/test/EcdsaSignerManager.test.ts → 11/11 passed.

PR Checklist

I have read the Contributing Guide
I have added tests that prove my fix is effective or that my feature works.
I have added documentation for new methods or changes to existing method behavior.
CI Workflows Are Passing

Further comments

Base branch: refactor/multisig-signature-verifier (#628). EcdsaSignerManager is not on main; targeting main would be incorrect.

Alternatives considered: bitmap of registered-signer indices (also noted in #629) — stronger without a sort requirement, but needs more fold state / registry coupling. Strict order keeps O(1) fold state and matches the module's documented direction.

coderabbitai · 2026-07-14T07:06:28Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e96fc436-55e0-4ca7-8f32-3999ea7b5ff9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

0xisk · 2026-07-14T09:35:37Z

🔵 followup: No CHANGELOG.md entry in the PR. The repo uses Keep-a-Changelog (## Unreleased → ### Fixed); add an entry for the #629 fix, or confirm it is intentionally folded into the #628 line since EcdsaSignerManager is not on main yet.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-07-14T09:36:29Z

+    const aHigh = slice<16>(a, 16) as Uint<128>;
+    const bLow = slice<16>(b, 0) as Uint<128>;
+    const bHigh = slice<16>(b, 16) as Uint<128>;
+    return (aHigh > bHigh) || ((aHigh == bHigh) && (aLow > bLow));


🔵 followup: The low-limb branch (aHigh == bHigh && aLow > bLow) and the little-endian Bytes<16> as Uint<128> assumption it rests on are not exercised — all test commitments are persistentHash outputs whose high limbs differ, so this branch never runs. Distinctness (the #629 fix) holds regardless of endianness, but if the cast were not LSB-first the shipped sortByCommitmentField would mis-order callers and reject honest n>=3 quorums (liveness, not a security break). Add a unit test on crafted 32-byte values: a pair equal in bytes 16–31 differing only in bytes 0–15, and a pair crossing the limb boundary; assert the TS sort order matches on-chain accept/reject.

added by claude (dev3-midnight-basic-review)

0xisk · 2026-07-14T09:36:30Z

    it('should reject an unregistered signer', async () => {
+      const { pubkeys, signatures } = sortedPair(PK_UNKNOWN, PK2);
+      // Order may place unknown first or second; either fails membership or order
+      await expect(verifier.verify(MSG, pubkeys, signatures)).rejects.toThrow();


⚪ nitpick: This lost its specific-message assertion because sort order decides whether the unknown key fails on membership or on ordering. Make it deterministic — present a registered key that sorts before the unknown one — so you can assert 'SignerManager: not a signer' again instead of "any throw".

added by claude (dev3-midnight-basic-review)

0xisk · 2026-07-14T09:36:31Z

+    const bHigh = slice<16>(b, 16) as Uint<128>;
+    return (aHigh > bHigh) || ((aHigh == bHigh) && (aLow > bLow));
+  }
 }


⚪ nitpick (if-minor): Stray trailing blank line after the module closing brace; compact fmt / biome will likely flag it.

added by claude (dev3-midnight-basic-review)

0xisk

Thank you @Yonkoo11 LGTM! Howver, I'm sorry you will need to sign your commit. Also for the followups, nitpicks you can ignore them. We can take care of that.

Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com> Co-authored-by: 0xisk <0xisk@proton.me>

…enZeppelin#616)

…nager verify only compared each commitment to the previous one, so [A,B,A] counted A twice toward the threshold when n >= 3. Require strictly increasing commitments under Compact Bytes→integer order (first byte LSB, same as Bytes as Field). Compare as two Uint<128> limbs so values never overflow the 248-bit Uint cap. Callers must sort pubkeys by ascending commitment. Adds verify3 tests for the OpenZeppelin#629 [A,B,A] case. Fixes OpenZeppelin#629

Yonkoo11 · 2026-07-26T07:41:09Z

Thanks @0xisk — commits are now signed (SSH). Ready for re-review.

Yonkoo11 requested review from a team as code owners July 14, 2026 07:06

0xisk reviewed Jul 14, 2026

View reviewed changes

0xisk self-assigned this Jul 15, 2026

andrew-fleming and others added 5 commits July 26, 2026 08:38

Add elgamal (OpenZeppelin#617)

35b2af3

Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com> Co-authored-by: 0xisk <0xisk@proton.me>

Add allowlist (OpenZeppelin#625)

353cc15

Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com> Co-authored-by: 0xisk <0xisk@proton.me>

Add blocklist (OpenZeppelin#626)

c81dc03

Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com> Co-authored-by: 0xisk <0xisk@proton.me>

fix(token): resolve zero-value revert audit findings (L-01, L-02) (Op…

72132e7

…enZeppelin#616)

Yonkoo11 force-pushed the fix/issue-629-distinct-signers branch from d5fa119 to 6e11b09 Compare July 26, 2026 07:41

0xisk merged commit 801845d into OpenZeppelin:refactor/multisig-signature-verifier Jul 28, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(multisig): reject non-adjacent duplicate signers in EcdsaSignerManager - #676

fix(multisig): reject non-adjacent duplicate signers in EcdsaSignerManager#676
0xisk merged 5 commits into
OpenZeppelin:refactor/multisig-signature-verifierfrom
Yonkoo11:fix/issue-629-distinct-signers

Yonkoo11 commented Jul 14, 2026

Uh oh!

coderabbitai Bot commented Jul 14, 2026 •

edited

Loading

Review skipped

Uh oh!

0xisk commented Jul 14, 2026

Uh oh!

0xisk Jul 14, 2026

Uh oh!

0xisk Jul 14, 2026

Uh oh!

0xisk Jul 14, 2026

Uh oh!

0xisk left a comment

Uh oh!

Yonkoo11 commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Yonkoo11 commented Jul 14, 2026

Types of changes

Summary

Approach

Call-site impact (why "breaking" is checked)

Tests

PR Checklist

Further comments

Uh oh!

coderabbitai Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

0xisk commented Jul 14, 2026

Uh oh!

0xisk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

0xisk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

0xisk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

0xisk left a comment

Choose a reason for hiding this comment

Uh oh!

Yonkoo11 commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 14, 2026 •

edited

Loading