Fuzz co-admin multisig verification and nonce-replay logic - #1149
Merged
joelpeace48-cell merged 2 commits intoAug 28, 2026
Merged
Conversation
…nesseStudioLab#851) Widened two private module items in contracts/campaign/src/lib.rs to pub: multisig_message() (the free function building the exact byte payload a co-admin signature is verified against) and OP_SET_MERKLE_ROOT (the op discriminant it's the only current caller of). Neither is a #[contractimpl] entrypoint, so this adds no new on-chain callable surface or behavior change - it only lets an external crate (the new fuzz harness) reconstruct a genuinely valid signed payload instead of only ever exercising rejection paths.
…inesseStudioLab#851) Adds contracts/campaign/fuzz/fuzz_targets/fuzz_multisig.rs, a cargo-fuzz target driving verify_multisig through its only real entry point, set_merkle_root (verify_multisig itself is a private helper). Covers: unknown signer -> typed error not panic; nonce replay -> typed error; threshold=0 bypasses multisig entirely; threshold > co_admins.len() rejected at set_multisig_threshold time, and the same condition reached transiently by removing a co-admin after configuring a valid threshold -> typed error, not panic; a genuine valid quorum (real ed25519 signatures over the real signed payload) succeeds. Also documents, and deliberately exercises under catch_unwind rather than hides, a real finding: Env::crypto().ed25519_verify is a Soroban host function with verify-or-trap semantics (no Result/bool-returning variant exists in the SDK). A registered co-admin paired with a signature that doesn't verify reaches that call and traps; with panic = "abort" in this contract's release profile, that trap can't be caught by the contract's own code even in principle. A real fix would mean replacing the host call with a guest-side, Result-returning verifier (e.g. ed25519-dalek compiled into the contract) - a materially larger, separate change to the contract's core trust model, not made in this PR. The harness asserts this is the *only* thing that can panic (a known co-admin, wrong signature bytes), not a broader "no panics under any input" claim that the current implementation genuinely can't make. Verified with `cargo check --bin fuzz_multisig` against the real contract types (compiled cleanly after fixing 3 real type-signature mistakes it caught - the try_* Result shape is Err(Ok(Error)) not Ok(Err(Error)), and non-try client methods unwrap/panic internally rather than returning Result). cargo-fuzz itself isn't installed in this environment, so the actual `cargo fuzz run` execution wasn't performed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| client.initialize(&admin); | ||
|
|
||
| let mut co_admins: std::vec::Vec<CoAdmin> = std::vec::Vec::new(); | ||
| let mut nonce: u64 = 0; |
| client.initialize(&admin); | ||
|
|
||
| let over_threshold = (NUM_COADMINS as u32) + 1 + (data[0] as u32 % 3); | ||
| let result = client.try_set_multisig_threshold(&admin, &0u64, &over_threshold); |
| garbage_sigs.push_back((garbage_signer, garbage_sig)); | ||
| let root2 = BytesN::from_array(&env2, &[data.get(10).copied().unwrap_or(0); 32]); | ||
|
|
||
| let result = client2.try_set_merkle_root(&admin2, &0u64, &root2, &garbage_sigs); |
joelpeace48-cell
merged commit Aug 28, 2026
2493c75
into
FinesseStudioLab:main
9 of 24 checks passed
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
3 participants
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.
Summary
Partial progress on #851 only. #778, #779, and #775 are not addressed in this PR — see below.
#851 — Fuzz the multisig verification and nonce-replay logic. Added
contracts/campaign/fuzz/fuzz_targets/fuzz_multisig.rs, a cargo-fuzz target drivingverify_multisigthrough its only real entry point,set_merkle_root(verify_multisigitself is a private helper). Covers: unknown signer → typed error, not panic; nonce replay → typed error;threshold=0bypasses multisig entirely;threshold > co_admins.len()rejected atset_multisig_thresholdtime, and the same condition reached transiently by removing a co-admin after configuring a valid threshold → typed error, not panic; a genuine valid quorum (real ed25519 signatures over the real signed payload, not just rejection-path fuzzing) succeeds.To make the "genuine valid quorum" case possible,
multisig_messageandOP_SET_MERKLE_ROOTwere widened from private topubincontracts/campaign/src/lib.rs— neither is a#[contractimpl]entry point, so this adds no new on-chain callable surface; it only lets the fuzz harness reconstruct the exact byte-for-byte payload a signature is verified against instead of only ever exercising rejection paths.A real finding, documented rather than hidden:
Env::crypto().ed25519_verifyis a Soroban host function with verify-or-trap semantics — noResult/bool-returning variant exists in the SDK. A registered co-admin paired with a signature that doesn't verify reaches that call and traps; withpanic = "abort"in this contract's release profile, that trap can't be caught by the contract's own code even in principle. A real fix would mean replacing the host call with a guest-side,Result-returning verifier (e.g.ed25519-dalekcompiled into the contract) — a materially larger, separate change to the contract's core trust model, not made in this PR. The harness asserts this is the only thing that can panic (a known co-admin, wrong signature bytes) — not a broader "no panics under any input" claim the current implementation genuinely can't make yet.#778, #779, #775 not started. Distributed tracing across HTTP→job→RPC (#778), SLO dashboards + burn-rate alerting wired to on-call (#779), and reproducible/signed WASM builds with provenance attestation (#775) are all substantial, infra-heavy projects (OTel span propagation across async boundaries with a real trace-assertion test, a live Grafana + alerting pipeline, and toolchain-pinned deterministic builds + SLSA/in-toto tooling respectively) that don't fit safely into this round. Flagging as open rather than rushing any of them.
Test plan
cd contracts/campaign/fuzz && cargo check --bin fuzz_multisig— compiles cleanly against the real contract types (caught and fixed 3 real type-signature mistakes along the way: thetry_*client Result shape isErr(Ok(Error))notOk(Err(Error)), and non-try_client methods unwrap/panic internally rather than returningResult)cargo-fuzzitself isn't installed in this environment, so the actualcargo fuzz run fuzz_multisigexecution wasn't performed — only the type-checked build