Multisig weight benchmarks

[illuzen]

Summary

Address V12 audit findings for the multisig pallet related to weight undercharging and DoS vectors.

Changes

1. propose weight reservation for inner call decode cost

Issue: The propose function decodes arbitrary RuntimeCall bytes and calls get_dispatch_info(), which is O(inner_call_count) for nested calls like Utility::batch. The benchmark only measured flat remark calls, so the per-byte weight (333 ps) only reflects memcpy, not recursive enum decoding.

Fix:

• Add MaxInnerCallWeight to upfront weight reservation (like execute already does)
• Burn full reserved weight on post-decode rejections (InvalidCall, CallWeightExceedsLimit, CallNotAllowedForHighSecurityMultisig)
• Refund based on actual inner call weight from get_dispatch_info() on success

2. create_multisig unbounded signer vector DoS

Issue: create_multisig accepts an unbounded Vec<AccountId>, performs expensive normalization (clone, sort O(n log n), dedup) before enforcing MaxSigners. An attacker could submit huge duplicate-heavy vectors that cost more CPU than the benchmarked weight covers.

Fix: Check signers.len() <= MaxSigners immediately after basic validation, before calling normalize_signers().

3. Failed path weight undercharging across multiple dispatchables

Issue: After loading variable-size proposal data from storage, error paths like ProposalExpired, AlreadyApproved, NotProposer, etc. used err_with_weight() which only charged fixed DB read counts, not the actual size-dependent storage read cost.

Fix: Calculate size-aware actual_weight immediately after loading proposal data and use it for all subsequent error paths:

• approve: ProposalExpired, AlreadyApproved
• cancel: NotProposer, ProposalNotActive
• remove_expired: ProposalNotActive, ProposalNotExpired
• execute: ProposalNotApproved, ProposalExpired (pre-decode); burn full weight for post-decode errors

4. create_multisig missing Wormhole storage weight

Issue: The benchmark only measured Multisig::Multisigs storage, but production calls T::ProofRecorder::reveal_address() which reads the account balance and, if nonzero, mutates Wormhole::PotentialWormholeBalance. An attacker could pre-fund derived multisig addresses to trigger this unweighted path.

Fix: Add Wormhole storage operations to create_multisig weight:

• +2 reads: System::Account, Wormhole::PotentialWormholeBalance
• +1 write: Wormhole::PotentialWormholeBalance

Testing

All 54 existing multisig pallet tests pass.

Security Impact

These fixes ensure block weight accounting accurately reflects actual CPU and storage costs, preventing availability DoS through underpriced extrinsics. No funds-at-risk issues.

---

Note

Medium Risk
Changes consensus-critical fee/weight math on multisig dispatchables; behavior shifts (higher propose cost, stricter failure charging) but targets DoS undercharging rather than fund logic.

Overview
Tightens block weight and CPU accounting on multisig extrinsics so underpriced work cannot be used for availability griefing (V12 audit follow-up).

create_multisig rejects signers.len() > MaxSigners before normalize_signers (clone/sort/dedup), so huge duplicate-heavy vectors cannot force expensive normalization on undercharged weight.

propose now reserves bookkeeping + MaxInnerCallWeight upfront (decode + get_dispatch_info() can scale with nested calls, not just byte length). After decode, failures (InvalidCall, CallWeightExceedsLimit, high-security whitelist) return actual_weight: None so the full reservation is burned; success charges bookkeeping plus the measured inner call weight.

approve, cancel, remove_expired, and execute compute call-size-aware weights as soon as the proposal is loaded and use them on common error paths instead of fixed read-only err_with_weight. execute likewise burns the full reservation on post-decode validation failures.

^{Reviewed by Cursor Bugbot for commit 288b134. Configure here.}

[n13]

Review: Multisig weight benchmarks (V12 audit follow-up)

Verdict: Request changes — the weight-accounting work for #1–#3 is sound and well reasoned, but claim #4 is described yet not implemented, and the refactor leaves dead code that breaks the post-merge clippy gate. Both are quick fixes.

Reviewed in an isolated worktree against origin/main; ran the pallet suite locally — 54/54 tests pass.

What's correct and well done

• #1 propose reservation — annotation now reserves propose_high_security(len) + MaxInnerCallWeight (lib.rs:530), returns actual_weight: None (burns the full reservation) on every post-decode rejection (InvalidCall, CallWeightExceedsLimit, HS whitelist), and refunds to bookkeeping + actual call_weight on success (lib.rs:722). Reservation ≥ actual on all paths, so refunds are always valid and conservative.
• #2 create_multisig DoS bound — signers.len() <= MaxSigners moved ahead of normalize_signers() (lib.rs:445), so the O(n log n) sort/dedup can't run on unbounded input. Correct.
• #3 failure-path weights — approve/cancel/remove_expired/execute compute a size-aware actual_weight right after loading the proposal and use it on error paths instead of fixed reads(2). Upfront reservation is …(MaxCallSize), returned value ≤ that → valid refund. execute success returns bookkeeping + actual inner weight ≤ reserved execute(MaxCallSize) + MaxInnerCallWeight. Correct.

Issues to address

1. Claim #4 (Wormhole storage weight for create_multisig) is in the description but not in the code.
The diff only touches pallets/multisig/src/lib.rs. The create_multisig weight annotation (lib.rs:428) and weights.rs::create_multisig (Multisig::Multisigs r:1 w:1 only) are unchanged — there is no +2 reads / +1 write.

It is a genuine undercharge: runtime wires type ProofRecorder = Wormhole (runtime/src/configs/mod.rs:643); create_multisig calls reveal_address → Wormhole::reveal_account, which always reads System::Account (total_balance) and, when the derived address was pre-funded, reads+writes PotentialWormholeBalance (pallets/wormhole/src/lib.rs:686, :717). The benchmark derives an unfunded address (benchmarking.rs:162-188), so the mutate path is never measured.

Severity is low (bounded per call; create_multisig also burns MultisigFee), but for an audit-remediation PR please either implement the weight addition or drop #4 from the description — as written it marks a V12 finding as fixed when it isn't.

2. Dead code: err_with_weight_raw is now unused.
The execute decode path that used it was inlined, leaving the helper (lib.rs:1233) with zero callers. Confirmed locally:

warning: associated function `err_with_weight_raw` is never used
  --> pallets/multisig/src/lib.rs:1233
  = note: `#[warn(dead_code)]`

ci.yml clippy is --workspace without -D warnings, so PR CI stays green — but pm-quality-check.yml runs cargo clippy --package pallet-multisig … -- -D warnings and pallets/multisig is in its matrix, so this becomes a hard error post-merge. Please remove the helper.

Nits

• DRY: the inline Err(DispatchErrorWithPostInfo { post_info: PostDispatchInfo { actual_weight: …, pays_fee: Pays::Yes }, error: … }) block now appears ~10×. Since err_with_weight_raw is being deleted anyway, consider two small helpers instead — err_with_actual_weight(error, Weight) and err_burn_full(error) (the actual_weight: None case) — mirroring the still-used err_with_weight.
• execute computes bookkeeping_weight twice with the identical value (lib.rs:1123 via WeightInfo::execute(call_size) and lib.rs:1177 via Self::bookkeeping_weight(...)). Collapse to one.
• Behavior change from #2: raw inputs with len > MaxSigners are now rejected even if they'd dedup to ≤ MaxSigners (previously accepted). Right tradeoff, but user-visible — add a dedicated test and a changelog note.
• MaxInnerCallWeight is used as the proxy for decode + get_dispatch_info cost. Reasonable conservative over-reservation, but decode cost ≠ dispatch weight — worth a one-line comment noting it's intentional.

Tests

54/54 pallet tests pass. No new test covers the new weight/DoS behavior; at minimum add one for the create_multisig oversized-raw-vector rejection.

---

Net: good fixes for #1–#3. Resolve the missing #4 (implement or de-scope) and the dead-code warning, and this is ready.

[n13]

Re-review: Multisig weight benchmarks — findings resolved

Verdict: Approve.

Re-reviewed at c54c005 (my first pass was against 288b134) in the same isolated worktree. Pallet suite now 55/55 pass (incl. the new regression test) and the refactor compiles warning-free.

Resolutions

• #4 Wormhole reveal storage (4486ca12) — create_multisig weight now adds the reveal-path storage: +1 read System::Account, +1 read / +1 write PotentialWormholeBalance (→ reads(3)/writes(2)), in both SubstrateWeight<T> and (), with docs. Resolves the ref-time undercharge.
• Dead code (85fcb1e) — err_with_weight_raw removed and replaced by err_with_actual_weight + err_burn_full/err_burn_full_raw, which also collapse the ~10 repeated inline DispatchErrorWithPostInfo blocks (my DRY nit). Compiles clean.
• Double bookkeeping_weight in execute — collapsed to a single computation.
• #2 behavior change (237549f) — documented as a BREAKING CHANGE and covered by create_multisig_rejects_oversized_raw_input_even_if_would_dedup_to_valid.
• MaxInnerCallWeight decode proxy (761217a) — intent now documented inline.

Correction to my original review (the false positive)

My secondary claim — that the unused err_with_weight_raw would "become a hard error post-merge" — was wrong. pm-quality-check.yml (the -D warnings clippy job) is workflow_dispatch-only, and ci.yml runs clippy --workspace without -D warnings, so the dead code would not have failed any automated gate. The finding (unused helper) was real and the cleanup is still worthwhile, but the CI consequence I attached to it was a false positive. Apologies for that.

Optional, non-blocking

• The create_multisig weight bumps ref-time (reads(3)/writes(2)) but leaves the proof_size component (from_parts(_, 10357)) unchanged, even though the new docs list System::Account (added: 2603) and PotentialWormholeBalance (added: 511). DbWeight::reads() only contributes ref-time, so the PoV for those two reads isn't counted. Immaterial for this chain (ref-time is the binding DoS constraint and is now covered), but for full 2D precision you could bump the proof-size literal — or regenerate the benchmark with a pre-funded derived address so it captures both dimensions natively rather than hand-editing the generated file.

Weight accounting is now consistent and the DRY refactor reads cleanly. Good to merge once the optional point is considered.