Eiger wormhole audit

[illuzen]

addresses the following issues

• EQ-QNT-MINING-R-04
• EQ-QNT-MINING-R-03
• EQ-QNT-MINING-R-02
• EQ-QNT-MINING-R-01
• EQ-QNT-WORMHOLE-F-03
• EQ-QNT-WORMHOLE-O-01
• EQ-QNT-MISC-R-01
• EQ-QNT-MISC-O-01
• EQ-QNT-MISC-R-02
• EQ-QNT-WORMHOLE-F-05
• EQ-QNT-WORMHOLE-F-02

[n13]

Here's my review of PR #471 - Eiger Wormhole Audit.

---

PR #471 Review: Eiger Wormhole Audit

Author: illuzen | +1501 / -531 across 18 files | CI: Format check failing

Overview

This PR addresses 11 findings from the Eiger wormhole audit. The changes span the wormhole pallet, mining-rewards pallet, runtime config, and the WormholeProofRecorderExtension transaction extension. Overall the direction is good, but there is one critical bug and several items to address.

---

Critical: Duplicate Proof Recording from Event Scanning

The refactored WormholeProofRecorderExtension (EQ-QNT-WORMHOLE-F-05) switches from call-matching to event-scanning via read_events_no_consensus(). However, events accumulate across the entire block and are only cleared in on_initialize of the next block. This means:

1. Tx1 executes, emits Transfer(Alice→Bob), post_dispatch scans events → records 1 proof
2. Tx2 executes, emits Transfer(Charlie→Dave), post_dispatch scans all events → sees Alice→Bob again + Charlie→Dave → records 2 proofs
3. Tx3 executes any call → post_dispatch scans all events → re-records proofs for Tx1 and Tx2

Every transfer gets recorded N times where N is the number of subsequent successful transactions in the block. This inflates TransferCount, creates phantom transfer proofs, and could corrupt wormhole ZK state.

Fix: Capture the event count in prepare (changing Pre from () to u32) and only process events with index >= that count in post_dispatch:

type Pre = u32; // event count snapshot

fn prepare(...) -> Result<Self::Pre, ...> {
    Ok(frame_system::Pallet::<Runtime>::event_count())
}

fn post_dispatch(event_count_before: Self::Pre, ...) {
    if result.is_ok() {
        Self::record_proofs_from_events_since(event_count_before);
    }
}

---

High: Weight Underestimation for Event-Based Proof Recording

The weight() method now returns a flat T::DbWeight::get().reads(2) regardless of how many transfers the transaction contains. The old approach charged reads_writes(n, 2*n) per transfer.

Each recorded proof still performs: 1 storage read (TransferCount) + 2 writes (TransferProof + TransferCount) + Poseidon hashing. A batch of 50 transfers pays for 2 reads but actually performs ~50 reads + ~100 writes. This is a weight undercharge that could be exploited to fill blocks cheaply.

Suggestion: Either:

• Keep the call-inspection approach for weight() estimation (counting transfers from the call), or
• Use post_dispatch weight correction via PostDispatchInfo to charge the actual cost after execution

---

Medium: Potential Double-Recording of Mining Reward Proofs

The extension now records proofs for Balances::Minted events. Mining rewards in on_finalize already record proofs via the TransferProofRecorder trait. If any user extrinsic runs after mining rewards mint events become visible (unlikely since on_finalize runs last, but worth considering for edge cases or future changes), the minted tokens could get duplicate proofs.

Additionally, the AssetMintingAccount parameter is added to the runtime but it's unclear if it matches the actual pallet_assets admin/minter account. Worth verifying that the sentinel address makes sense downstream.

---

Low: CI Formatting Failure

Two formatting issues flagged by cargo +nightly fmt:

1. pallets/wormhole/src/tests.rs:437 - line break placement in multiplication
2. runtime/src/configs/mod.rs:564 - line break placement in match arm

These are trivially fixable by running cargo +nightly fmt --all.

---

What Looks Good

1. Config trait cleanup (EQ-QNT-MISC-R-02, EQ-QNT-MISC-O-01) - Moving bounds from where clauses into explicit associated types (NativeBalance, AssetId, AssetBalance) is a clean improvement. Removes complex type alias gymnastics.

2. Proper benchmarking (EQ-QNT-WORMHOLE-F-02) - Real aggregated proof data for benchmarks, split into pre_validate_proof and verify_aggregated_proof. The weight file now has actual measured numbers instead of placeholders.

3. Weight correction on early failure (EQ-QNT-WORMHOLE-O-01) - Changing verify_aggregated_proof to return DispatchResultWithPostInfo and returning pre_validate_proof weight on pre-validation failure is a good optimization for block capacity.

4. Pass-by-reference in mint_reward (EQ-QNT-MINING-R-01) - Option<T::AccountId> → Option<&T::AccountId> reduces unnecessary cloning.

5. Shared test helpers (EQ-QNT-MINING-R-03) - TestMiner, account_id(), and MINTING_ACCOUNT centralized in qp_wormhole primitives eliminates duplicate test utilities across pallet mocks. Good DRY improvement.

6. Edge case tests (EQ-QNT-WORMHOLE-F-03) - Thorough coverage for malformed digests (wrong engine ID, wrong length, oversized, empty), fee-to-treasury fallback, replay protection, and aggregated proof lifecycle.

7. Removed dead code (EQ-QNT-MISC-R-01) - VerifierNotAvailable error variant, unused MaxTokenAmount, DefaultMintAmount, MinimumDeposit, and unused imports cleaned up.

8. frame-system test fix - Weight::from_all(t) → Weight::from_parts(t, 0) correctly sets proof_size to 0 to match pre-dispatch weight.

---

Minor Notes

• The MINTING_ACCOUNT in test mocks uses [3u8; 32] while production uses [1u8; 32] - this is fine for test isolation but worth a note in the constants.
• The zero_reward_does_not_record_proof test doesn't actually test the zero-reward path since it can't easily exhaust supply in a unit test. The test name is somewhat misleading - it's really a consistency check.
• println! statements in test_parse_aggregated_public_inputs_succeeds and the emission simulation test - consider removing for cleaner test output (minor, these are test-only).

---

Summary

Severity	Issue	Action Needed
Critical	Event scanning re-processes all block events per extrinsic	Snapshot event count in prepare, filter in post_dispatch
High	Weight undercharge for proof recording	Restore per-transfer weight estimation or use post-dispatch correction
Medium	Potential double-recording for Minted events	Verify no overlap with on_finalize proof recording
Low	CI format check failing	Run cargo +nightly fmt --all

The audit remediations are solid in intent and the test coverage is impressive. The critical issue with duplicate proof recording from event scanning needs to be fixed before merging.

[n13]

PR Review: #471 (Eiger Wormhole Audit)

Hey! This is a really solid PR overall — great job tackling 11 audit items across the wormhole pallet, mining rewards, runtime config, and transaction extensions. The direction is excellent, the test coverage is impressive, and there are some nice cleanups (especially the config trait refactor and DRY test helpers in primitives/wormhole). 👍

Critical Issue (Must Fix Before Merge)

The refactor in WormholeProofRecorderExtension (EQ-QNT-WORMHOLE-F-05) introduces a duplicate proof recording bug.

record_proofs_from_events() uses frame_system::Pallet::<Runtime>::read_events_no_consensus(), which returns all events accumulated for the current block. Events are only cleared in the next block's on_initialize. This means:

• Tx1 emits Transfer → post_dispatch records proof feat: Simple ranked collective #1
• Tx2 emits Transfer → post_dispatch re-scans all events → records proof for Tx1 again + Tx2
• Every subsequent tx re-records previous ones

This inflates TransferCount and creates duplicate ZK leaves, which could corrupt wormhole state.

Recommended fix (as suggested in the review):

Change type Pre = (); to type Pre = u32; (or EventIndex), snapshot frame_system::Pallet::<Runtime>::event_count() in prepare(), and filter events in post_dispatch to only process those with index >= snapshot.

High Priority

Weight underestimation in WormholeProofRecorderExtension::weight(). It now returns a fixed reads(2) regardless of how many transfers occur. The old call-matching approach charged per-transfer. Each proof does storage R/W + Poseidon hashing.

Consider restoring call inspection for weight estimation or using PostDispatchInfo correction.

Medium

Potential double-recording of Minted events. The extension now listens for Balances::Minted and Assets::Issued, while mining-rewards already calls record_transfer_proof directly from on_finalize. Need to ensure no overlap (timing looks okay since on_finalize runs after extrinsic post-dispatch, but worth verifying).

Also, confirm AssetMintingAccount aligns with the actual pallet_assets minter account.

Low / Quick Wins

• Formatting: CI is failing (two spots in pallets/wormhole/src/tests.rs:437 and runtime/src/configs/mod.rs:564). Running cargo +nightly fmt --all fixes it cleanly.
• Minor test nits: A couple println!s in tests and the zero_reward_does_not_record_proof test name could be clarified.

What Looks Really Good

• Config cleanup using explicit associated types (NativeBalance, AssetId, etc.) — much cleaner than the old where clauses.
• Updated benchmarks with real aggregated proof data + split pre_validate_proof / verify_aggregated_proof.
• Early weight correction on proof validation failure.
• Pass-by-reference improvements in mint_reward.
• Shared test helpers in primitives/wormhole (excellent DRY adherence!).
• Thorough edge case tests for malformed proofs, replay protection, etc.
• Dead code removal.

The PR follows our principles well: concise changes where possible, good DRY improvements, and minimal comments.

[n13]

Critical issue addressed here : #472

[n13]

Now I have a thorough picture of the PR. Here's my review:

---

PR #471 Review: Eiger Wormhole Audit

Overall this is a really strong PR - the audit remediations are well-implemented, the test coverage is impressive, and the DRY improvements in shared test helpers are excellent. The critical bug fix from PR #472 (event count snapshot) is correctly integrated.

Status of Previously Identified Issues

Critical (Duplicate Proof Recording) - FIXED

The event count snapshot approach is correctly implemented:

	type Pre = u32;
	// ...
	fn prepare(
		self,
		_val: Self::Val,
		_origin: &sp_runtime::traits::DispatchOriginOf<RuntimeCall>,
		_call: &RuntimeCall,
		_info: &sp_runtime::traits::DispatchInfoOf<RuntimeCall>,
		_len: usize,
	) -> Result<Self::Pre, TransactionValidityError> {
		Ok(frame_system::Pallet::<Runtime>::event_count())
	}

And the regression test at line 789 (no_duplicate_proofs_across_transactions_in_same_block) thoroughly validates this with a 3-tx scenario.

High (Weight Underestimation) - FIXED

Weight estimation was restored to per-transfer call inspection:

	fn weight(&self, call: &RuntimeCall) -> Weight {
		let n = Self::count_transfers(call);
		if n > 0 {
			T::DbWeight::get().reads_writes(n, 2 * n)
		} else {
			Weight::zero()
		}
	}

Low (CI Formatting) - PARTIALLY FIXED

The uncommitted local changes are exactly the formatting fixes. These 3 files need to be committed:

• pallets/wormhole/src/tests.rs - line break placement in multiplication
• runtime/src/configs/mod.rs - line break placement in match arm
• runtime/src/transaction_extensions.rs - | placement in match arms

Remaining Concern: Weight for Minted Events

count_transfers only inspects direct transfer calls (Balances/Assets transfer variants and utility batches). But record_proofs_from_events_since also processes Balances::Minted and Assets::Issued events. If a sudo or governance call triggers a mint, the weight estimation returns 0 while post_dispatch still records a proof. In practice this is a very low-probability edge case (mints via sudo/governance are rare), but worth being aware of.

Medium: Mining Rewards Double-Recording

After careful analysis, this is not an issue in the current design:

• Mining rewards run in on_finalize, which executes after all extrinsics' post_dispatch
• The Minted events from on_finalize won't be seen by any extrinsic's event scanner
• Events are cleared in the next block's on_initialize
• Mining rewards also call record_transfer_proof directly - this is the only recording path for rewards

Minor Nits

1. println! in tests - test_parse_aggregated_public_inputs_succeeds (line 298-305 of pallets/wormhole/src/tests.rs) and the emission simulation test both have println! statements. These create noise in test output but don't affect correctness.

2. MINTING_ACCOUNT sentinel difference - Production uses [1u8; 32] (native) / [2u8; 32] (assets), test mock uses [3u8; 32]. This is fine for test isolation.

What Looks Great

• Config trait cleanup (explicit associated types NativeBalance, AssetId, AssetBalance) - much cleaner than where clause gymnastics
• Real benchmark data with split pre_validate_proof / verify_aggregated_proof weights
• Early weight correction returning pre_validate_proof weight on pre-validation failure - nice block capacity optimization
• Shared test helpers (TestMiner, account_id(), MINTING_ACCOUNT) in primitives/wormhole - excellent DRY adherence
• MockProofRecorder in mining-rewards now actually tracks proofs for verification - much better test fidelity
• Edge case test coverage - malformed proofs, replay protection, nullifier reuse, multiple-tx-in-block regression

Summary

Severity	Issue	Status
Critical	Duplicate event scanning	Fixed (PR #472)
High	Weight underestimation	Fixed
Medium	Mining reward double-recording	Not an issue (verified)
Low	Formatting	Fix is uncommitted locally
Low	Weight for Minted events	Acceptable edge case

The PR is in good shape. The uncommitted formatting changes should be committed, and then this should be ready to merge. Want me to commit those formatting fixes?

[n13]

Looks good