refactor(multisig): regroup modules, split signer manager by scheme, finalize tests#628
Open
0xisk wants to merge 13 commits into
Open
refactor(multisig): regroup modules, split signer manager by scheme, finalize tests#6280xisk wants to merge 13 commits into
0xisk wants to merge 13 commits into
Conversation
Move the flat multisig modules into concern-based subdirectories so the package reads as composable building blocks rather than a flat pile: * signer/ — Signer, SignerManager * proposal/ — ProposalManager * treasury/ — ShieldedTreasury, ShieldedTreasuryStateless, UnshieldedTreasury * forwarder/ — ForwarderPrivate, ForwarderShielded, ForwarderUnshielded Presets and mocks keep their locations; only their import paths change. Mocks stay flat, so compiled artifact names (flat by basename) and every TS test import are untouched. The five moved modules that reach into utils/ gain an extra ../ in their import path. Also mark V3's inlined mint/burn with a TODO pointing at the future ShieldedToken module. Pure move plus import-path change, no logic change. compact:multisig compiles 24/24; the multisig suite passes 273/273. Refs: #619
SignerManager<T> is an older generation of the same module as Signer<T>: identical signer-set/threshold state, but without the init-safety guards (assertInitialized / re-init protection) and the custom-setup _setThreshold path that Signer<T> adds. Remove the legacy module together with its mock, simulator, witnesses, and test. The next commit renames Signer<T> into its place; splitting the removal out first lets git record that as a rename (preserving Signer's history) rather than a rewrite of this file. Refs: #619
Rename the hardened Signer<T> into the SignerManager name freed by the previous commit, keeping the more descriptive name on the init-safe implementation. Git records this as a rename, so the module's history, blame, and log --follow carry over. * signer/Signer.compact -> signer/SignerManager.compact (module decl + assert prefix Signer: -> SignerManager:) * mock, simulator, witnesses, and test renamed to match * repoint the ShieldedMultiSigV3 import and migrate the preset tests to the new module name and messages (the init-safe module reports "threshold must not be zero" rather than the legacy "threshold must be > 0") compact:multisig compiles 22/22; multisig suite passes 249/249. Refs: #619
Reshape the multisig package per the team layout review: reusable primitives sit at the package root, the composable behaviour modules become example contracts, and presets are named for what they do instead of a version number. * Root primitives: move SignerManager to the package root and add the extracted SignatureVerifier (commitment signer registry + threshold ECDSA-commitment verification), the single owner of the registry the signature presets share. * examples/: house the behaviour modules (SignatureTreasury, SignatureMintBurn, ProposalTreasury) and the three forwarder example contracts. * Presets named for behaviour: ShieldedMultiSigV2 becomes NativeShieldedStatelessTreasury, ShieldedMultiSigV3 becomes NativeShieldedMintBurn, ShieldedMultiSig becomes NativeShieldedProposal; add NativeShieldedTokenVault (mint/burn plus treasury under one signer set, the no-C2C composition). * Rename treasury and forwarder modules to NativeShielded / Private naming and repoint every import path, including the test mocks. Compiles green under SKIP_ZK (28/28 multisig contracts). The .ts test stacks (specs, simulators, witnesses) are renamed on disk but their rewiring to the new names is deferred to a follow-up.
Make signature verification a swappable per-scheme module, since Compact cannot make `verify` generic over a signature scheme. * SignerManager<T> stays the general signer registry (signer set, threshold, membership, add/remove), generic over the identity type: <Bytes<32>> commitments for the signature path, <Either<...>> for caller-authorized governance. * EcdsaSignerManager (formerly SignatureVerifier) wraps SignerManager<Bytes<32>> and adds threshold ECDSA-commitment verification (instance salt, commitment derivation, verify). The cryptographic check is stubbed until the Compact ECDSA primitive lands. It is the single entrance the signature examples import; a future SchnorrSignerManager is a sibling of the same shape. * SignatureTreasury, SignatureMintBurn, and the NativeShieldedTokenVault preset import EcdsaSignerManager. ProposalTreasury (caller-auth V1) keeps using SignerManager<Either> directly. The signer count stays a single source of truth: one SignerManager registry under EcdsaSignerManager, shared across the Vault's mint/burn/execute via the same import path. Compiles green under SKIP_ZK (28/28 multisig contracts). The .ts test stacks remain renamed-on-disk and deferred to a follow-up.
The commitment domain separator padded the full descriptive string "PrivateNativeShieldedForwarder:commitment" (41 bytes) to 32, which exceeds the pad width and fails to compile. Use the module name alone (30 bytes) as the unique domain tag. This unblocks MockForwarderPrivate and the private-forwarder suite, which the compiler wrapper had been masking as a false success.
The deployable forwarder example contracts (examples/*Forwarder) are thin top-level wrappers in the same category as the presets, and their basenames collide with the forwarder modules in the shared flat artifact namespace. Remove them (and their now-stale preset tests and simulators) from this branch; they return with the presets in a follow-up branch. The forwarder modules and their Mock-based tests stay and remain the coverage for forwarder behavior.
…esses Every multisig contract has empty private state and declares no witnesses, so each per-contract *Witnesses.ts was byte-identical to the shared EmptyWitnesses.ts (the forwarder simulators already used it). Delete them all and repoint the remaining simulators (SignerManager, ProposalManager, ShieldedTreasury) at EmptyWitnesses, matching where main is heading and shrinking the rebase surface.
…nerManager The example modules (SignatureTreasury, SignatureMintBurn, ProposalTreasury) are composable behaviors with no constructor, so each gets a Mock top-level wrapper plus a simulator and spec, matching how every other module in the package is tested. The specs carry over from the old ShieldedMultiSigV2/V3/(V1) suites (rename-preserved) and target the modules directly via EmptyWitnesses. Also rename the signature primitive's test stack to match the renamed module: MockSignatureVerifier -> MockEcdsaSignerManager, with a new EcdsaSignerManager spec/simulator and the corrected assert prefix. The example modules' duplicate-signer assertion now reads "EcdsaSignerManager: duplicate signer".
The forwarder modules were renamed (ForwarderShielded -> NativeShieldedForwarder, etc.) and their assert messages updated, but the kept module tests still expected the old prefixes. Update the toThrow expectations to the current "Native*Forwarder:" / "PrivateNativeShieldedForwarder:" messages.
The deployable preset contracts (NativeShieldedMintBurn, NativeShieldedProposal, NativeShieldedStatelessTreasury, NativeShieldedTokenVault) need rework and move to a separate branch. Remove them here and drop the now-dangling `presets/...` references from the SignatureTreasury and SignatureMintBurn module docs, describing the thin-wrapper composition pattern in prose instead.
Wire the previously-orphan MockShieldedTreasuryStateless to a simulator and spec covering deposit, full/partial send (with change accounting), and the over-send rejection. Uses the shared EmptyWitnesses.
This was referenced Jun 25, 2026
Move the multisig examples (SignatureMintBurn, SignatureTreasury, ProposalTreasury) out of src/multisig/examples/ into test/integration/_mocks/, converting each from a module (+ test mock) into a self-contained top-level contract prefixed with Multisig (MultisigSignatureMintBurn, MultisigSignatureTreasury, MultisigProposalTreasury). They stay as usage examples and now sit where they can double as integration-test fixtures. Remove the now-redundant unit tests, simulators, and mocks for the three. Integration tests are not added here; they are tracked in #630. Refs: #630
Contributor
WalkthroughAdds an ECDSA signer manager, tightens signer-manager initialization and threshold handling, renames forwarder and treasury modules, and rewires multisig integration mocks, simulators, and tests to the new module layout. ChangesMultisig signer and contract migration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EcdsaSignerManager
participant SignerManager
participant stubVerifySignature
Caller->>EcdsaSignerManager: initialize(salt, signers, thresh)
EcdsaSignerManager->>SignerManager: initialize(signers, thresh)
Caller->>EcdsaSignerManager: verify(msgHash, pubkeys, signatures)
loop each pubkey/signature pair
EcdsaSignerManager->>EcdsaSignerManager: verifySignature(state, pk, signature)
EcdsaSignerManager->>SignerManager: isSigner(commitment)
EcdsaSignerManager->>stubVerifySignature: validate signature
end
EcdsaSignerManager->>SignerManager: assertThresholdMet(validCount)
sequenceDiagram
participant Caller
participant MultisigSignatureTreasury
participant EcdsaSignerManager
participant NativeShieldedTreasury
Caller->>MultisigSignatureTreasury: execute(to, amount, coin, pubkeys, signatures)
MultisigSignatureTreasury->>MultisigSignatureTreasury: snapshot and increment _nonce
MultisigSignatureTreasury->>EcdsaSignerManager: verify(messageHash, pubkeys, signatures)
MultisigSignatureTreasury->>NativeShieldedTreasury: _send(recipient, amount)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/multisig/treasury/NativeShieldedTreasuryStateless.compact (1)
6-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the module docs to describe the stateless behavior.
This header still says the module stores per-color UTXOs and tracks received/sent totals, but this implementation has no treasury ledger state and only proxies deposit/send with caller-provided coins. That mismatch will mislead consumers reading the generated docs.
Suggested doc update
/** * `@module` NativeShieldedTreasuryStateless - * `@description` Manages shielded (private) token deposits, accounting, - * and transfers for multisig governance contracts. - * - * Coins are stored on the contract ledger in a map keyed by token color, - * with one UTXO per color. Deposits are merged with existing coins of - * the same color via `mergeCoinImmediate`. This simplifies coin selection - * at spend time — the executor doesn't need to choose between multiple - * UTXOs of the same color. - * - * Cumulative received and sent totals are tracked per color for audit - * purposes. The canonical balance query is `getTokenBalance`, which - * reads the actual coin value from the UTXO map. + * `@description` Stateless shielded deposit/send helpers for multisig + * governance contracts. + * + * Deposits are received directly at the protocol layer. Sends consume a + * caller-supplied coin and return any change back to this contract. + * No per-color treasury ledger state or accounting totals are tracked. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/multisig/treasury/NativeShieldedTreasuryStateless.compact` around lines 6 - 20, Update the module header docs in NativeShieldedTreasuryStateless to match the actual stateless design: remove claims about storing coins in a contract ledger, per-color UTXO maps, mergeCoinImmediate behavior, and cumulative received/sent totals. Describe that this module only proxies deposit/send operations using caller-provided coins and does not maintain treasury ledger state; keep the docs aligned with the module-level purpose and any exported entrypoints in NativeShieldedTreasuryStateless.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/src/multisig/EcdsaSignerManager.compact`:
- Around line 199-218: The `verify()` flow in `EcdsaSignerManager` is currently
unsafe because `stubVerifySignature()` always returns true, so any arbitrary
signature passes. Make this production circuit unusable until real ECDSA
verification exists by removing or gating the `stubVerifySignature`, and ensure
`verify()`/`assert(...)` cannot succeed in non-test builds unless a real
`ecdsaVerify` implementation is wired in. Keep the current stub only behind an
explicit test-only path or fail closed with a clear error in
`EcdsaSignerManager.verify` and `stubVerifySignature`.
- Around line 113-126: The generic verify<`#n`> entrypoint in
EcdsaSignerManager.compact still allows non-adjacent duplicate approvals to be
counted multiple times, so tighten uniqueness handling before threshold checks.
Either constrain verify<`#n`> to only support up to two presented signers, or
update verifySignature/fold-based processing to enforce full uniqueness across
the entire pubkeys/signatures set instead of only comparing against
state.prevCommitment, and apply the same fix to the other verify entrypoint
mentioned in the diff.
In `@contracts/src/multisig/forwarder/PrivateNativeShieldedForwarder.compact`:
- Around line 60-62: The documented parentCommitment formula is inconsistent
with the circuit’s actual hash domain tag, so make the domain tag match
everywhere. Update the docstring for parentCommitment and the
_calculateParentCommitment logic to use the same pad(32,
"PrivateNativeShieldedForwarder...") value as the _drain parent-check path, and
ensure the commitment computed off-chain matches what the circuit verifies. Keep
the symbol names PrivateNativeShieldedForwarder, _calculateParentCommitment, and
_drain aligned so deployments initialized from the docs can pass the parent
commitment check.
In `@contracts/src/multisig/SignerManager.compact`:
- Around line 257-309: The custom setup path in SignerManager is incomplete
because `_setThreshold()` can mutate threshold state while `_isInitialized`
remains false, leaving `assertSigner`, `assertThresholdMet`, `getSignerCount`,
and `getThreshold` unusable behind `assertInitialized()`. Update the setup flow
around `_setThreshold`, `assertInitialized`, and `assertNotInitialized` to
include an explicit finalization step that marks the manager initialized after
custom configuration, or else remove the custom-setup path from the
API/docs/tests so only fully operational states are exposed.
In `@contracts/test/integration/_mocks/MultisigSignatureTreasury.compact`:
- Around line 69-101: The signed hash in execute should bind the full execution
domain, not just nonce, to.address, coin.color, and amount. Update the msgHash
construction in MultisigSignatureTreasury.execute to also include to.kind and a
deployment-specific domain separator/value so signatures cannot be replayed
across recipient variants or deployments. Keep the change localized around
persistentHash, Signer_verify<2>, and Treasury__send so the verified message
exactly matches the intended execution context.
- Around line 39-52: The constructor-configured threshold in
MultisigSignatureTreasury does not match the approval count enforced by execute,
which is still fixed to two signatures. Update the execute flow (including the
Signer_verify call and the presented approvals Vector shape) to honor the stored
thresh value from Signer_initialize, or otherwise validate and enforce that the
constructor can only accept a threshold compatible with execute. Keep the change
aligned with the existing execute and constructor symbols so the contract cannot
be deployed in a state where execution is impossible or under-enforced.
---
Outside diff comments:
In `@contracts/src/multisig/treasury/NativeShieldedTreasuryStateless.compact`:
- Around line 6-20: Update the module header docs in
NativeShieldedTreasuryStateless to match the actual stateless design: remove
claims about storing coins in a contract ledger, per-color UTXO maps,
mergeCoinImmediate behavior, and cumulative received/sent totals. Describe that
this module only proxies deposit/send operations using caller-provided coins and
does not maintain treasury ledger state; keep the docs aligned with the
module-level purpose and any exported entrypoints in
NativeShieldedTreasuryStateless.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ec4febf-8dca-47bf-9fd4-e7751e3715ef
📒 Files selected for processing (60)
contracts/src/multisig/EcdsaSignerManager.compactcontracts/src/multisig/Signer.compactcontracts/src/multisig/SignerManager.compactcontracts/src/multisig/forwarder/NativeShieldedForwarder.compactcontracts/src/multisig/forwarder/NativeUnshieldedForwarder.compactcontracts/src/multisig/forwarder/PrivateNativeShieldedForwarder.compactcontracts/src/multisig/presets/ShieldedMultiSigV2.compactcontracts/src/multisig/presets/ShieldedMultiSigV3.compactcontracts/src/multisig/presets/forwarder/ForwarderPrivate.compactcontracts/src/multisig/presets/forwarder/ForwarderShielded.compactcontracts/src/multisig/presets/forwarder/ForwarderUnshielded.compactcontracts/src/multisig/proposal/ProposalManager.compactcontracts/src/multisig/test/EcdsaSignerManager.test.tscontracts/src/multisig/test/Forwarder.test.tscontracts/src/multisig/test/ForwarderPrivate.test.tscontracts/src/multisig/test/NativeShieldedTreasuryStateless.test.tscontracts/src/multisig/test/ShieldedMultiSig.test.tscontracts/src/multisig/test/ShieldedMultiSigV2.test.tscontracts/src/multisig/test/ShieldedMultiSigV3.test.tscontracts/src/multisig/test/Signer.test.tscontracts/src/multisig/test/SignerManager.test.tscontracts/src/multisig/test/mocks/MockEcdsaSignerManager.compactcontracts/src/multisig/test/mocks/MockForwarderPrivate.compactcontracts/src/multisig/test/mocks/MockForwarderShielded.compactcontracts/src/multisig/test/mocks/MockForwarderUnshielded.compactcontracts/src/multisig/test/mocks/MockProposalManager.compactcontracts/src/multisig/test/mocks/MockShieldedTreasury.compactcontracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compactcontracts/src/multisig/test/mocks/MockSigner.compactcontracts/src/multisig/test/mocks/MockSignerManager.compactcontracts/src/multisig/test/mocks/MockUnshieldedTreasury.compactcontracts/src/multisig/test/presets/ForwarderPrivate.test.tscontracts/src/multisig/test/presets/ForwarderShielded.test.tscontracts/src/multisig/test/presets/ForwarderUnshielded.test.tscontracts/src/multisig/test/simulators/EcdsaSignerManagerSimulator.tscontracts/src/multisig/test/simulators/NativeShieldedTreasuryStatelessSimulator.tscontracts/src/multisig/test/simulators/ProposalManagerSimulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigSimulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigV2Simulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.tscontracts/src/multisig/test/simulators/ShieldedTreasurySimulator.tscontracts/src/multisig/test/simulators/SignerManagerSimulator.tscontracts/src/multisig/test/simulators/SignerSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderUnshieldedSimulator.tscontracts/src/multisig/test/witnesses/ProposalManagerWitnesses.tscontracts/src/multisig/test/witnesses/ShieldedMultiSigV2Witnesses.tscontracts/src/multisig/test/witnesses/ShieldedMultiSigV3Witnesses.tscontracts/src/multisig/test/witnesses/ShieldedMultiSigWitnesses.tscontracts/src/multisig/test/witnesses/ShieldedTreasuryWitnesses.tscontracts/src/multisig/test/witnesses/SignerManagerWitnesses.tscontracts/src/multisig/test/witnesses/SignerWitnesses.tscontracts/src/multisig/test/witnesses/UnshieldedTreasuryWitnesses.tscontracts/src/multisig/treasury/NativeShieldedTreasury.compactcontracts/src/multisig/treasury/NativeShieldedTreasuryStateless.compactcontracts/src/multisig/treasury/NativeUnshieldedTreasury.compactcontracts/test/integration/_mocks/MultisigProposalTreasury.compactcontracts/test/integration/_mocks/MultisigSignatureMintBurn.compactcontracts/test/integration/_mocks/MultisigSignatureTreasury.compact
💤 Files with no reviewable changes (29)
- contracts/src/multisig/test/simulators/SignerSimulator.ts
- contracts/src/multisig/test/witnesses/ShieldedTreasuryWitnesses.ts
- contracts/src/multisig/test/witnesses/SignerWitnesses.ts
- contracts/src/multisig/presets/ShieldedMultiSigV2.compact
- contracts/src/multisig/test/simulators/presets/ForwarderUnshieldedSimulator.ts
- contracts/src/multisig/presets/ShieldedMultiSigV3.compact
- contracts/src/multisig/test/ShieldedMultiSigV2.test.ts
- contracts/src/multisig/test/simulators/ShieldedMultiSigV2Simulator.ts
- contracts/src/multisig/test/Signer.test.ts
- contracts/src/multisig/test/witnesses/UnshieldedTreasuryWitnesses.ts
- contracts/src/multisig/test/witnesses/ShieldedMultiSigV3Witnesses.ts
- contracts/src/multisig/presets/forwarder/ForwarderUnshielded.compact
- contracts/src/multisig/test/witnesses/ProposalManagerWitnesses.ts
- contracts/src/multisig/test/presets/ForwarderShielded.test.ts
- contracts/src/multisig/test/simulators/presets/ForwarderShieldedSimulator.ts
- contracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.ts
- contracts/src/multisig/presets/forwarder/ForwarderShielded.compact
- contracts/src/multisig/presets/forwarder/ForwarderPrivate.compact
- contracts/src/multisig/test/presets/ForwarderPrivate.test.ts
- contracts/src/multisig/test/simulators/presets/ForwarderPrivateSimulator.ts
- contracts/src/multisig/test/ShieldedMultiSig.test.ts
- contracts/src/multisig/test/witnesses/SignerManagerWitnesses.ts
- contracts/src/multisig/test/presets/ForwarderUnshielded.test.ts
- contracts/src/multisig/test/ShieldedMultiSigV3.test.ts
- contracts/src/multisig/test/simulators/ShieldedMultiSigSimulator.ts
- contracts/src/multisig/test/witnesses/ShieldedMultiSigWitnesses.ts
- contracts/src/multisig/test/witnesses/ShieldedMultiSigV2Witnesses.ts
- contracts/src/multisig/test/mocks/MockSigner.compact
- contracts/src/multisig/Signer.compact
Comment on lines
+113
to
+126
| export circuit verify<#n>( | ||
| msgHash: Bytes<32>, | ||
| pubkeys: Vector<n, Bytes<64>>, | ||
| signatures: Vector<n, Bytes<64>> | ||
| ): [] { | ||
| const initialState = VerificationState { | ||
| validCount: 0 as Uint<8>, | ||
| prevCommitment: pad(32, ""), | ||
| msgHash: msgHash | ||
| }; | ||
|
|
||
| const finalState = fold(verifySignature, initialState, pubkeys, signatures); | ||
| Signer_assertThresholdMet(finalState.validCount); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Generic verify<#n> still double-counts non-adjacent duplicates.
The fold only rejects commitment == state.prevCommitment. For n >= 3, an approval list like [A, B, A] increments validCount three times and can satisfy a 3-of-3 threshold with only two distinct signers. Either hard-cap this API to two presented signers for now, or add full uniqueness enforcement before exposing the generic entrypoint.
Also applies to: 187-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/multisig/EcdsaSignerManager.compact` around lines 113 - 126,
The generic verify<`#n`> entrypoint in EcdsaSignerManager.compact still allows
non-adjacent duplicate approvals to be counted multiple times, so tighten
uniqueness handling before threshold checks. Either constrain verify<`#n`> to only
support up to two presented signers, or update verifySignature/fold-based
processing to enforce full uniqueness across the entire pubkeys/signatures set
instead of only comparing against state.prevCommitment, and apply the same fix
to the other verify entrypoint mentioned in the diff.
Comment on lines
+199
to
+218
| // TODO: Replace with ecdsaVerify when the Compact ECDSA primitive is available | ||
| assert(stubVerifySignature(pubkey, state.msgHash, signature), "EcdsaSignerManager: invalid signature"); | ||
|
|
||
| return VerificationState { | ||
| validCount: state.validCount + 1 as Uint<8>, | ||
| prevCommitment: commitment, | ||
| msgHash: state.msgHash | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * @description Stub for ECDSA signature verification. Always returns true. | ||
| * MUST be replaced before any non-test deployment. | ||
| */ | ||
| circuit stubVerifySignature( | ||
| pubkey: Bytes<64>, | ||
| msgHash: Bytes<32>, | ||
| signature: Bytes<64> | ||
| ): Boolean { | ||
| return true; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
verify() currently accepts forged approvals.
stubVerifySignature() always returns true, so any caller can satisfy the threshold with registered public keys and arbitrary 64-byte blobs as “signatures”. Any contract that relies on EcdsaSignerManager.verify() for authorization is therefore bypassable. Please keep this behavior in a test-only mock or make the production circuit unusable until real signature verification exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/multisig/EcdsaSignerManager.compact` around lines 199 - 218,
The `verify()` flow in `EcdsaSignerManager` is currently unsafe because
`stubVerifySignature()` always returns true, so any arbitrary signature passes.
Make this production circuit unusable until real ECDSA verification exists by
removing or gating the `stubVerifySignature`, and ensure
`verify()`/`assert(...)` cannot succeed in non-test builds unless a real
`ecdsaVerify` implementation is wired in. Keep the current stub only behind an
explicit test-only path or fail closed with a clear error in
`EcdsaSignerManager.verify` and `stubVerifySignature`.
Comment on lines
60
to
62
| * @param {Bytes<32>} parentCommitment - Domain-tagged | ||
| * `persistentHash([pad(32, "ForwarderPrivate:commitment"), parentAddr, opSecret])` | ||
| * `persistentHash([pad(32, "PrivateNativeShieldedForwarder:commitment"), parentAddr, opSecret])` | ||
| * computed off-chain by the deployer (see `_calculateParentCommitment`). |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the documented commitment formula with the circuit.
Line 61 tells deployers to initialize with pad(32, "PrivateNativeShieldedForwarder:commitment"), but Lines 199-200 actually hash pad(32, "PrivateNativeShieldedForwarder"). A contract initialized from the documented formula can never satisfy _drain’s parent check, so deposits become unrecoverable. Pick one domain tag and use it consistently in both the docs and _calculateParentCommitment.
Also applies to: 191-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/multisig/forwarder/PrivateNativeShieldedForwarder.compact`
around lines 60 - 62, The documented parentCommitment formula is inconsistent
with the circuit’s actual hash domain tag, so make the domain tag match
everywhere. Update the docstring for parentCommitment and the
_calculateParentCommitment logic to use the same pad(32,
"PrivateNativeShieldedForwarder...") value as the _drain parent-check path, and
ensure the commitment computed off-chain matches what the circuit verifies. Keep
the symbol names PrivateNativeShieldedForwarder, _calculateParentCommitment, and
_drain aligned so deployments initialized from the docs can pass the parent
commitment check.
Comment on lines
+257
to
+309
| /** | ||
| * @description Sets the approval threshold without checking | ||
| * against the current signer count. | ||
| * | ||
| * @warning This is intended for use during contract construction | ||
| * or custom setup flows where signers may not yet be registered. | ||
| * | ||
| * @notice Access control is NOT enforced here. | ||
| * The consuming contract must gate this behind its own | ||
| * authorization policy. Use `_changeThreshold` for | ||
| * operational threshold changes with signer count validation. | ||
| * | ||
| * @circuitInfo k=6, rows=40 | ||
| * | ||
| * Requirements: | ||
| * | ||
| * - `newThreshold` must not be zero. | ||
| * | ||
| * @param {Uint<8>} newThreshold - The minimum number of approvals required. | ||
| * @returns {[]} Empty tuple. | ||
| */ | ||
| export circuit _setThreshold(newThreshold: Uint<8>): [] { | ||
| assert(newThreshold != 0, "SignerManager: threshold must not be zero"); | ||
| _threshold = disclose(newThreshold); | ||
| } | ||
|
|
||
| // ─── Init guards ───────────────────────────────────────────────────────── | ||
|
|
||
| /** | ||
| * @description Asserts that the contract has been initialized, throwing an error if not. | ||
| * | ||
| * Requirements: | ||
| * | ||
| * - Contract must be initialized. | ||
| * | ||
| * @return {[]} - Empty tuple. | ||
| */ | ||
| circuit assertInitialized(): [] { | ||
| assert(_isInitialized, "SignerManager: contract not initialized"); | ||
| } | ||
|
|
||
| /** | ||
| * @description Asserts that the contract has not been initialized, throwing an error if it has. | ||
| * | ||
| * Requirements: | ||
| * | ||
| * - Contract must not be initialized. | ||
| * | ||
| * @return {[]} - Empty tuple. | ||
| */ | ||
| circuit assertNotInitialized(): [] { | ||
| assert(!_isInitialized, "SignerManager: contract already initialized"); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Custom setup never becomes operational.
_setThreshold() is documented as a custom-setup tool, but skipping initialize() leaves _isInitialized false forever. After that, assertSigner, assertThresholdMet, getSignerCount, and getThreshold still hard-fail behind assertInitialized(), so the not-initialized flow only mutates raw state and never yields a usable manager. Please add an explicit finalization step for custom setup, or remove this path from the public API/docs/tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/src/multisig/SignerManager.compact` around lines 257 - 309, The
custom setup path in SignerManager is incomplete because `_setThreshold()` can
mutate threshold state while `_isInitialized` remains false, leaving
`assertSigner`, `assertThresholdMet`, `getSignerCount`, and `getThreshold`
unusable behind `assertInitialized()`. Update the setup flow around
`_setThreshold`, `assertInitialized`, and `assertNotInitialized` to include an
explicit finalization step that marks the manager initialized after custom
configuration, or else remove the custom-setup path from the API/docs/tests so
only fully operational states are exposed.
Comment on lines
+39
to
+52
| /** | ||
| * @description Initializes the shared signer registry and instance salt. | ||
| * | ||
| * @param {Bytes<32>} instanceSalt - Random salt for commitment derivation. | ||
| * @param {Vector<3, Bytes<32>>} signerCommitments - Signer commitments. | ||
| * @param {Uint<8>} thresh - Minimum approvals required. | ||
| */ | ||
| constructor( | ||
| instanceSalt: Bytes<32>, | ||
| signerCommitments: Vector<3, Bytes<32>>, | ||
| thresh: Uint<8> | ||
| ) { | ||
| Signer_initialize<3>(instanceSalt, signerCommitments, thresh); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the configured threshold match the execute API.
thresh is constructor-configurable, but execute is hard-wired to exactly two presented approvals via Vector<2, ...> and Signer_verify<2>. With thresh = 3, this contract can never execute; with thresh = 1, callers still must supply two signatures.
Suggested guard
constructor(
instanceSalt: Bytes<32>,
signerCommitments: Vector<3, Bytes<32>>,
thresh: Uint<8>
) {
+ assert(thresh == 2, "MultisigSignatureTreasury: threshold must be 2");
Signer_initialize<3>(instanceSalt, signerCommitments, thresh);
}Also applies to: 82-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/test/integration/_mocks/MultisigSignatureTreasury.compact` around
lines 39 - 52, The constructor-configured threshold in MultisigSignatureTreasury
does not match the approval count enforced by execute, which is still fixed to
two signatures. Update the execute flow (including the Signer_verify call and
the presented approvals Vector shape) to honor the stored thresh value from
Signer_initialize, or otherwise validate and enforce that the constructor can
only accept a threshold compatible with execute. Keep the change aligned with
the existing execute and constructor symbols so the contract cannot be deployed
in a state where execution is impossible or under-enforced.
Comment on lines
+69
to
+101
| /** | ||
| * @description Executes a shielded send authorized by threshold signatures. | ||
| * Reads and increments the nonce, reconstructs the off-chain message hash | ||
| * `persistentHash(nonce, recipient address, coin color, amount)`, verifies the | ||
| * signatures against the shared registry, then sends from the treasury. | ||
| * | ||
| * @param {Proposal_Recipient} to - The recipient. | ||
| * @param {Uint<128>} amount - The amount to send. | ||
| * @param {QualifiedShieldedCoinInfo} coin - The coin to spend (operator pool). | ||
| * @param {Vector<2, Bytes<64>>} pubkeys - ECDSA public keys of approving signers. | ||
| * @param {Vector<2, Bytes<64>>} signatures - Signatures over the operation. | ||
| * @returns {ShieldedSendResult} The send result including any change. | ||
| */ | ||
| export circuit execute( | ||
| to: Proposal_Recipient, | ||
| amount: Uint<128>, | ||
| coin: QualifiedShieldedCoinInfo, | ||
| pubkeys: Vector<2, Bytes<64>>, | ||
| signatures: Vector<2, Bytes<64>> | ||
| ): ShieldedSendResult { | ||
| const currentNonce = _nonce; | ||
| _nonce.increment(1); | ||
|
|
||
| const msgHash = persistentHash<Vector<4, Bytes<32>>>([ | ||
| currentNonce as Bytes<32>, | ||
| to.address, | ||
| coin.color, | ||
| amount as Bytes<32> | ||
| ]); | ||
|
|
||
| Signer_verify<2>(msgHash, pubkeys, signatures); | ||
|
|
||
| return Treasury__send(coin, Proposal_toShieldedRecipient(to), amount); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Bind the full execution domain into the signed hash.
The hash only covers [nonce, to.address, coin.color, amount]. It does not bind to.kind, and it does not include any deployment-specific domain value. That means the same signatures can be replayed for a different recipient variant that shares the same address bytes, and across another deployment that reuses the same signer pubkeys and nonce progression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/test/integration/_mocks/MultisigSignatureTreasury.compact` around
lines 69 - 101, The signed hash in execute should bind the full execution
domain, not just nonce, to.address, coin.color, and amount. Update the msgHash
construction in MultisigSignatureTreasury.execute to also include to.kind and a
deployment-specific domain separator/value so signatures cannot be replayed
across recipient variants or deployments. Keep the change localized around
persistentHash, Signer_verify<2>, and Treasury__send so the verified message
exactly matches the intended execution context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
Restructures the
multisigpackage: groups modules intoproposal/,treasury/,forwarder/subdirs and a root primitive layer; renamesSigner → SignerManager; splits signature verification into per-schememanagers (
EcdsaSignerManager, withSignerManager<T>as the sharedregistry); moves the composable behaviors into
examples/and adds theirtest coverage.
Tests: every example module (
SignatureTreasury,SignatureMintBurn,ProposalTreasury) and the stateless treasury now have Mock-based suites;per-contract witnesses collapsed into the shared
EmptyWitnesses. 252 testspass and all 24 multisig contracts compile under
SKIP_ZK.Deferred to a follow-up branch: the deployable presets and the forwarder
example wrappers.
Known limitation:
NativeUnshieldedTreasuryhas no suite — the simulatorcannot drive unshielded token operations (
receiveUnshieldedfails to decodeu64); the module is unused on this branch.PR Checklist
Further comments
This PR is the multisig restructure discussed previously; it is large because
it spans the full regroup, the
Signer → SignerManagerrename, and theper-scheme signer-manager split, plus the example test coverage. Presets and
forwarder example wrappers are intentionally split into a follow-up branch to
keep their rework separate.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor