Harden FRAME primitives against audit findings

[illuzen]

Summary

This PR addresses a batch of security and correctness issues in vendored FRAME code and pallet-assets. The changes are intentionally minimal and localized: each fix targets a specific failure mode, adds regression tests where appropriate, and preserves existing behavior for valid configurations.

Block import & consensus

• Validate zk_tree_root on block import — frame-executive now asserts the imported header's zk_tree_root matches the value recomputed from state in both final_checks and try_execute_block, closing a gap where a block producer could forge the ZK Merkle root while keeping state_root honest.

Macro & genesis hardening

• Fix dynamic_params inner precedence — outer runtime_params / params_pallet arguments are only injected into bare or empty inner #[dynamic_pallet_params] attributes, honoring documented inner-over-outer precedence.
• Cap genesis dev account derivation — pallet-balances rejects num_accounts > MAX_DEV_ACCOUNTS (10_000) before expensive sr25519 key derivation, bounding work reachable via GenesisBuilder::build_state.

Runtime upgrade / migration safety

• Bound RemovePallet / RemoveStorage — both helpers now require a Limit: Get<u32> and pass it to clear_prefix, preventing unbounded mandatory deletion during runtime upgrades.
• Add move_prefix_bounded — new cursor/limit API for staging attacker-growable prefix moves across blocks; move_prefix preserved as an all-at-once wrapper with documentation warnings.

Token & balance accounting

• Check issuance headroom in Balanced::deposit — rejects Exact deposits that would overflow total_issuance; caps BestEffort deposits to remaining headroom, preventing supply desync via saturating debt-drop handlers.
• Propagate actual transfer debit — generic fungibles::Mutate::transfer and fungible::Mutate::transfer now credit and report the amount actually removed from the source (including reaping dust), keeping total_supply == Σ balances.
• Burn reaping dust on approved transfers — do_transfer_approved sets burn_dust: true so delegates cannot move more than the approved amount to their chosen destination when the owner's account is reaped.
• Fix ration denominator overflow — halve ratio parts when first + second would overflow u32, preventing a saturated denominator from sending the entire imbalance to the first leg.
• Reject invalid SplitTwoWays configs at compile time — PART1 + PART2 == 0 or overflow is a const-eval panic (build failure), not a runtime abort during fee/slash handling.

Freeze, nonce, and decode hardening

• Fix over-freeze helper semantics — ensure_frozen only checks freezable balance when the call would increase the freeze; decrease_frozen (fungibles) routes through set_freeze instead of set_frozen(Polite), allowing partial release of legitimately over-freeze balances.
• Reject nonce wrap to zero — CheckNonce rejects transactions at nonce == MAX instead of wrapping stored nonce back to 0, preventing replay of archived low-nonce transactions.
• Require full SCALE consumption — Callback::curry and all typed_attribute* readers use decode_all, rejecting prefix-compatible decodes with trailing attacker-controlled bytes.

Investigated, not changed

• Wormhole unsigned tx + ReversibleTransactionExtension — report is incorrect for the actual submission path. Bare extrinsics use bare_validate() (a no-op default), bypassing the signed-only validate() check. Wormhole txs work as intended; no code change.

Test plan

• frame-executive — block_import_of_bad_zk_tree_root_fails
• frame-support-procedural — dynamic params precedence (3 unit tests)
• pallet-balances — dev account cap, deposit issuance headroom, freeze tests
• frame-support — migration bounds, move_prefix_bounded, freeze helpers, imbalance ration/split, decode_all, curry
• frame-system — nonce overflow rejection
• pallet-assets — reaped-dust transfer accounting, approved-transfer dust burn
• Full runtime integration smoke test on a dev node
• Confirm no runtime migration uses unbounded RemovePallet/RemoveStorage/move_prefix against user-growable storage without switching to bounded/chunked variants

Stats

Branch: illuzen/frame-fixes (11 commits, +981 / −110 across 23 files)

---

Note

High Risk
Touches consensus-critical block import (zk_tree_root), transaction validity (CheckNonce), and runtime-upgrade deletion APIs (breaking RemovePallet/RemoveStorage type arity); runtimes must adopt the new Limit parameter and verify migration configs.

Overview
Hardens vendored FRAME primitives against several audit-style failure modes: forged header fields, unbounded upgrade work, token accounting edge cases, and nonce replay.

Block import — frame-executive now recomputes and asserts the header zk_tree_root matches state in final_checks and (when state checks are on) try_execute_block, so a producer cannot lie about the ZK Merkle root while keeping state_root honest. qp-header is a normal dependency; regression test block_import_of_bad_zk_tree_root_fails.

Macros — #[dynamic_params] only injects outer runtime_params / params_pallet into bare or empty inner #[dynamic_pallet_params]; explicit inner args win.

Migrations — RemovePallet and RemoveStorage take a Limit: Get<u32> and pass it to clear_prefix, capping keys removed per upgrade (warn if more remain). New move_prefix_bounded with cursor/limit for chunked moves; move_prefix wraps it with docs warning on unbounded user-growable prefixes.

Tokens & genesis — Balanced::deposit checks total_issuance headroom before crediting (Exact errors, BestEffort caps). pallet-balances rejects num_accounts > MAX_DEV_ACCOUNTS (10_000) in derive_dev_account before sr25519 derivation.

Transactions — CheckNonce rejects nonce == MAX and refuses to wrap stored nonce to zero on increment, blocking replay of old low-nonce txs.

^{Reviewed by Cursor Bugbot for commit 8ef8b60. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 8ef8b60. Configure here.}

[n13]

Review

Reviewed all 14 commits, tracing each fix into the underlying primitives (Unbalanced::decrease_balance, pallet-assets prep_debit/prep_credit, pallet-balances dust & freeze plumbing, PrefixIterator internals), and ran the test suites for all six touched crates locally (frame-support incl. try-runtime feature, frame-executive, frame-system, pallet-balances, pallet-assets, frame-support-procedural) — all green, consistent with the passing CI matrix.

Verified correct

• zk_tree_root on import — mirrors the state_root pattern in final_checks, and is correctly gated behind state_root_check in try_execute_block (the root is state-derived). frame_system::finalize() recomputes it from ZkTreeRoot storage, so honest headers always match and forged roots panic on import. No retroactive sync risk: historical blocks execute under their original runtime.
• CheckNonce — both validate_nonce_for_account and prepare_nonce_for_account reject nonce == MAX, so the stored nonce can never wrap to 0 and re-validate archived low-nonce transactions. Stale is the right error (such a tx can never become valid). Test covers the MAX-1 boundary too.
• RemovePallet/RemoveStorage bounding — clear_prefix(Some(Limit)) usage is correct, and the try-runtime hooks (updated in a0dd58e) tolerate a remainder exactly when pre_upgrade saw more than Limit keys — this resolves the earlier Bugbot finding. Nice detail committing keys to the backend in the tests, since the clear_prefix limit only bounds backend keys.
• move_prefix_bounded — cursor semantics check out: last_raw_key() works as a cursor even though that key was just drained (next_key operates on the underlying ordering), remainder detection peeks past the last key with a prefix check, and move_prefix is preserved as a u32::MAX-limit wrapper.
• Transfer debit propagation — correct for both implementation styles: pallet-assets' decrease_balance override returns amount + dust with no separate dust handling, so crediting the actual debit is exactly what restores supply == Σ balances; pallet-balances' default Unbalanced::decrease_balance still returns exactly amount (dust is routed through handle_dust → DustRemoval), so native-token transfer behavior is unchanged.
• Approved-transfer burn_dust: true — prep_credit burns the reap dust from supply instead of letting the delegate direct it to an arbitrary destination beyond the approval.
• Freeze fixes — the relaxation actually takes effect (pallet-balances' extend_freeze/set_freeze don't re-check freezable balance), and routing fungibles::decrease_frozen through set_freeze aligns it with the pre-existing fungible counterpart. Freeze increases remain gated.
• ration halving — a single halving suffices ((a>>1)+(b>>1) ≤ u32::MAX), ratio preserved up to rounding; SplitTwoWays::TOTAL turning invalid configs into post-monomorphization compile errors is a nice guard.
• decode_all hardening, dev-account cap, dynamic_params precedence — all sound; the precedence change matches the documented intent and has focused unit tests. No in-repo users are affected by any of the breaking changes (no RemovePallet/RemoveStorage/move_prefix/SplitTwoWays users in runtime/pallets/node).
• The wormhole "investigated, not changed" conclusion matches the code: wormhole extrinsics are gated by #[pallet::validate_unsigned], and bare extrinsics never enter the TransactionExtension pipeline that ReversibleTransactionExtension::validate participates in.

Findings

1. fungibles::Balanced::deposit didn't get the mirrored headroom check (should fix; fine as a fast follow-up). frame/support/src/traits/tokens/fungibles/regular.rs deposit still credits before any issuance check, and its OnDropDebt is the same saturating IncreaseIssuance. Note ItemOf routes the patched fungible::Balanced::deposit into this unpatched default. Practical in-repo impact is ~zero because pallet-assets' can_increase(.., increase_supply: true) already rejects supply overflow at deposit time, but as a hardening of the vendored generic primitive the fix is only half-applied — recommend mirroring the same match precision block.
2. Headroom check is best-effort against outstanding debts (doc note). Issuance only grows when a Debt drops, so two deposits with live debts can each pass checked_add individually yet jointly saturate on drop. Not fixable without reserving headroom; worth a sentence in the comment so it isn't read as a hard invariant.
3. Self-transfer now reports amount + dust (minor). In both patched Mutate::transfer fns the source == dest early-return now returns actual even though nothing is debited and no reap occurs (previously amount). Relatedly, the can_deposit(dest, actual, ..) pre-check includes dust that pallet-balances-style impls never credit (their decrease_balance returns amount) — an overflow-edge-only over-restriction. Suggest returning amount on the self path.
4. Nits: block_import_of_bad_zk_tree_root_fails would be tighter with #[should_panic(expected = "ZK tree root")]; count_prefixed_keys_up_to duplicates the test helper count_prefixed_keys in the same file; the misc.rs doctest should_panic → no_run flips are unrelated to the audit batch and reduce those doctests to compile-only (deserves a mention in the PR description); MAX_DEV_ACCOUNTS = 10_000 still allows seconds of sr25519 derivation per build_state call — bounded, but arguably could be lower.

Verdict

Approve. Every fix is correct as implemented and regression-tested, the consensus-critical zk_tree_root and CheckNonce changes behave exactly as described, and the breaking API changes are documented with no in-repo fallout. Finding 1 is the only substantive gap and is safe to land as an immediate follow-up (or a quick addition here), since pallet-assets' internal supply check covers the practical case.

[illuzen]

v12 pls audit