Frame fixes medium low#617
Conversation
…in halt These OnRuntimeUpgrade helpers cleared an entire prefix with clear_prefix(.., None) in a mandatory upgrade hook. For a prefix whose key count users can inflate, the first post-upgrade block performs unbounded deletion work, never fits the block weight limit, and re-runs every attempt, halting the chain. Add a `Limit: Get<u32>` parameter and clear at most `Limit` keys per upgrade so the work is bounded. Excess keys remain (harmless orphaned storage) and are logged; raise the limit or use a multi-block migration to remove the rest. Co-authored-by: Cursor <cursoragent@cursor.com>
move_prefix drains and re-writes every key below a prefix in one call with no limit, cursor, or progress feedback. Used in a mandatory runtime-upgrade hook on a user-growable prefix, an attacker can inflate the prefix beforehand and force the first post-upgrade block to perform unbounded reads/deletes/writes, stalling the upgrade. Add `move_prefix_bounded(from, to, limit, maybe_cursor) -> MovePrefixResult` mirroring `clear_storage_prefix`'s bounded/cursored pattern, so a multi-block migration can move keys in bounded chunks and charge weight per moved key. `move_prefix` now delegates to it unbounded, preserving existing behaviour, and its docs (plus `move_storage_from_pallet`) warn about the unbounded call. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… balance check Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…gs at compile time Co-authored-by: Cursor <cursoragent@cursor.com>
Credit the full amount removed from the source in generic transfer helpers so expendable reaping cannot desync issuance from balances. Burn reaping dust in transfer_approved so delegates cannot move more than the approved amount to their chosen destination. Co-authored-by: Cursor <cursoragent@cursor.com>
…ime checks Co-authored-by: Cursor <cursoragent@cursor.com>
…nicking Co-authored-by: Cursor <cursoragent@cursor.com>
…tors Both the pallet-level Task::iter and the aggregated RuntimeTask::iter collected every task into heap vectors before yielding the first item. Task lists can be derived from attacker-growable storage, so eager materialization allowed unbounded allocation in off-chain workers. Co-authored-by: Cursor <cursoragent@cursor.com>
before_all_runtime_migrations pre-seeded the on-chain storage version for any pallet with an empty prefix, running before the runtime's custom migrations. A version-gated migration importing state into a new pallet prefix (rename/split/import) then saw the version as already current and was silently skipped, stranding state under the old namespace. The version is now initialized after the pallet's own on_runtime_upgrade, and only if the version key is still absent. Co-authored-by: Cursor <cursoragent@cursor.com>
… them FunctionAttr::parse consumed only the leading value of weight/call_index/ feeless_if/authorize/weight_of_authorize attributes and returned without checking the buffers were exhausted, so malformed attributes compiled with trailing tokens silently dropped. This could silently truncate a call's weight metadata and underprice a dispatchable. Co-authored-by: Cursor <cursoragent@cursor.com>
…ion check The shared ViewFunction::execute path used plain DecodeAll, bypassing the mem-tracked decode derived on the generated request structs. View functions are reachable via the RuntimeViewFunction runtime API with attacker-supplied input, so decoding is now bounded by MAX_VIEW_FUNCTION_DECODE_MEM to prevent decode-bomb allocation, while still requiring the whole input be consumed. Co-authored-by: Cursor <cursoragent@cursor.com>
InitializedField::eq approximated serde's camelCase rename with an underscore-insensitive, case-insensitive match that was not injective, so distinct genesis fields like a_b (=> aB) and ab (=> ab) compared equal and retain_initialized_fields could keep an unselected default field. The comparison now uses serde's exact camelCase rule so distinct identifiers never collide. Co-authored-by: Cursor <cursoragent@cursor.com>
child::take conditioned key removal on a successful typed decode, so a corrupted (undecodable) explicit entry returned None but was left in storage. This broke the take_or*/take_or_default/take_or_else contract that no explicit entry remains on return. take() now removes any explicit entry, decodable or not. Co-authored-by: Cursor <cursoragent@cursor.com>
The legacy storage_kill host call only reports backend deletions, so overlay-resident child keys it also deletes were reported as zero unique/ loops. Callers using those counters for weight or cleanup accounting could be told no work occurred. clear_storage now counts visible keys before and after and folds overlay-only removals into unique/loops. Co-authored-by: Cursor <cursoragent@cursor.com>
move_prefix only guarded the from==to case. If to_prefix nests inside from_prefix (or vice versa), keys written during the drain fall back inside the iterator's traversal domain, causing the move to reprocess its own output (unbounded work) or terminate leaving values under the source prefix. The prefixes must now be disjoint (neither a prefix of the other). Co-authored-by: Cursor <cursoragent@cursor.com>
The bounded try_append helpers treated a missing decode_len as an empty collection. For ValueQuery storage with a non-empty OnEmpty default, the effective value may already be at capacity, so appends could bypass the bound (and the default's membership assumptions). StorageTryAppend now exposes the value length, and all try_append paths (value, map, double map, n-map, counted map) fall back to the effective query-kind default when no explicit entry exists. Co-authored-by: Cursor <cursoragent@cursor.com>
StorageAppend is now a behavior trait instead of a pure marker. Vec and Digest keep the raw sp_io append, but BTreeSet decodes the stored set, inserts the item, and writes the canonical representation back. Raw appends could previously store duplicate entries that a decoded set silently deduplicates, letting repeated appends of the same element grow state and decode cost without changing the logical set. Co-authored-by: Cursor <cursoragent@cursor.com>
A None cursor from clear_prefix only means the selected partial prefix is exhausted, not that the whole map is empty. Killing the counter reported a non-empty map as count()==0, letting quota/emptiness checks be bypassed. Subtract the removed entries instead. Co-authored-by: Cursor <cursoragent@cursor.com>
WrapperKeepOpaque advertised a MaxEncodedLen derived from T but stored an unbounded Vec<u8>, so attacker-controlled bytes could exceed the max_size used for bounded-storage and PoV accounting. Decode now rejects opaque data longer than T::max_encoded_len(), a checked try_from_encoded constructor is added, and the unchecked from_encoded is deprecated. Co-authored-by: Cursor <cursoragent@cursor.com>
StorePreimage::bound notes a preimage for oversized calls before the scheduler runs its fallible checks. If resolve_time or place_task failed, do_schedule/do_schedule_named returned an error without undoing the note, leaving an unowned system-requested preimage as state bloat. Both now drop the bounded call's preimage on every failure path, mirroring cancellation cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
Callback stores only (pallet_index, call_index) and resolves them against the call layout present when curry runs, so persisting a caller-provided callback across a runtime upgrade can decode into a different call. No in-repo consumer currently persists such callbacks, so this documents the constraint for future StatementOracle implementations rather than changing the (unused) type's encoding. Co-authored-by: Cursor <cursoragent@cursor.com>
LoneFreezeConsideration/LoneHoldConsideration encode no ticket state, so a zero-footprint ticket leaves no freeze/hold marker yet its drop/burn clears the whole reason bucket. A stale zero ticket could therefore thaw, release, or burn a newer paid ticket's collateral. new/update now reject a zero converted amount so every live ticket corresponds to a non-empty bucket. Co-authored-by: Cursor <cursoragent@cursor.com>
`transfer_on_hold` documents that in `Restriction::OnHold` mode the destination account must already exist, but the default implementation never checked this: `can_deposit(.., Extant)` only concerns issuance accounting, so a held balance could be credited to a non-existent account, creating orphan hold state with no underlying account. Both the `fungible` and `fungibles` default implementations now reject `OnHold` transfers to non-existent destinations with `TokenError::CannotCreate`. Co-authored-by: Cursor <cursoragent@cursor.com>
…ights `check_non_zero_sender()` and `check_nonce()` return constant weights while their implementations do work proportional to the encoded signer length (scanning the encoded `AccountId` and encoding it into transaction-pool dependency tags). That work is bounded by `AccountId::max_encoded_len()`, so the constant model is only sound when the weights are benchmarked against the runtime's actual `AccountId` with a maximal-length signer. Document this requirement on `WeightInfo` so runtimes with variable-length account identifiers regenerate their weights instead of reusing values benchmarked with short signers. Co-authored-by: Cursor <cursoragent@cursor.com>
`BlockWeightsBuilder::build` derived `max_extrinsic` from a class's `max_total` minus the average initialization cost and the class base extrinsic weight, but ignored `base_block`, which every block consumes before any extrinsic runs. For classes whose `max_total` equals `max_block` (e.g. the operational class from the default builders), a transaction at the advertised cap passed pool validation yet could never be included: `base_block + base_extrinsic + weight` exceeded `max_block` and overflowed into (and past) the reserved allowance, creating a valid-but-unincludable transaction class. The derivation now subtracts `base_block` as well, keeping the validation-time cap consistent with the block-building capacity model. Co-authored-by: Cursor <cursoragent@cursor.com>
`set_code`, `set_code_without_checks` and `apply_authorized_upgrade` tried to consume the rest of the block by returning `max_block` as post-dispatch `actual_weight`, but `calc_actual_weight` caps the post-dispatch weight at the static pre-dispatch weight (~0.17s of ref-time), so the returned value was silently ignored and further transactions could still be included after the code update in the same block — executing old-runtime transactions on top of an accepted upgrade and letting an attacker shape the migration starting state. The upgrade dispatchables now register the remaining block weight directly via `register_extra_weight_unchecked`, so the block really is drained regardless of the static call weight. Co-authored-by: Cursor <cursoragent@cursor.com>
…ss counter `as_derivative` pseudonym accounts receive tracked credits but never sign an extrinsic themselves (their controller signs), so their nonce stays zero forever: every credit to them was added to `PotentialWormholeBalance` and nothing ever subtracted it. An attacker could bounce fixed capital between derivative addresses to inflate the wormhole soundness pool without bound, weakening the `TotalWormholeExits <= PotentialWormholeBalance` backstop. Mirror the multisig treatment: on the first use of a pseudonym, `as_derivative` now reveals it via a new narrow `qp_wormhole::AddressRevealer` hook (deducting its balance from the pool) and records it in `KnownDerivatives`; the runtime's `NonWormholeAccounts` then excludes known pseudonyms so later receipts are not counted either. Co-authored-by: Cursor <cursoragent@cursor.com>
`RuntimeMetadataPrefixed` used a derived `Decode`, so an invalid blob was only rejectable after the entire `(u32, RuntimeMetadata)` tuple — potentially an attacker-sized structure of nested vectors, maps and strings — had been fully materialized. Decode now checks the reserved 'meta' magic after the first four bytes and fails fast on a mismatch. Also document on the type that metadata decoding has no schema-level caps beyond input-length proportionality, so callers decoding untrusted blobs must bound the input size up front. Co-authored-by: Cursor <cursoragent@cursor.com>
…-maps in duplicate check Co-authored-by: Cursor <cursoragent@cursor.com>
…ead of fabricating progress Co-authored-by: Cursor <cursoragent@cursor.com>
… key existence transitions Co-authored-by: Cursor <cursoragent@cursor.com>
…hrough key and value iterators Co-authored-by: Cursor <cursoragent@cursor.com>
…ncounted proof-recording work post-dispatch Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
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 8fc3a01. Configure here.
| pub fn move_prefix(from_prefix: &[u8], to_prefix: &[u8]) { | ||
| // Drive the bounded mover to completion in one shot. `u32::MAX` keys is effectively no bound, | ||
| // preserving the historical all-at-once behaviour for callers that know the prefix is small. | ||
| move_prefix_bounded(from_prefix, to_prefix, u32::MAX, None); |
There was a problem hiding this comment.
move_prefix stops at u32 max
Low Severity
move_prefix now delegates to move_prefix_bounded with a limit of u32::MAX. If more than that many keys share the source prefix, the helper stops after one pass and returns without error, leaving the remainder under the old prefix. The previous implementation drained the entire prefix in one call.
Reviewed by Cursor Bugbot for commit 8fc3a01. Configure here.
n13
left a comment
There was a problem hiding this comment.
Review — V12 medium-severity audit fixes
Went through all 67 files. Overall this is careful, well-tested work: each fix is narrowly targeted at the audited failure mode, and nearly every behavioral change ships with a regression test that would fail without the fix. Issues below, ordered by importance.
Blocking
1. CI is red — cargo fmt failures skip the entire test matrix.
The Fast Checks (Format) job fails on pallets/scheduler/src/tests.rs, pallets/wormhole/src/lib.rs, and runtime/src/transaction_extensions.rs, so Build & Test and Clippy never ran — nothing in this PR has CI test coverage yet. Needs a fmt pass and re-push.
Should fix / discuss
2. child::clear_storage now walks the whole child trie twice (frame/support/src/storage/child.rs).
The new key_count helper enumerates every key via next_key both before and after the kill to attribute overlay-only removals. A call with maybe_limit = Some(small) now does O(total keys) host calls — the same "user-growable state forces unbounded work" pattern this audit batch fixes elsewhere (e.g. RemovePallet). No in-repo callers today, so it's latent, but consider bounding the count or at least documenting the cost on the function.
3. Wormhole weight model not updated for the new KnownDerivatives read (runtime/src/transaction_extensions.rs / runtime/src/configs/mod.rs).
NonWormholeAccounts::contains now also reads Utility::KnownDerivatives (via is_derivative), so is_ambiguous_account costs one more read than the model assumes. Both per_transfer_weight() (4 reads) and the signer reveal weight (reads_writes(4, 1)) should become 5 reads, or the comments should say why the extra read is ignored.
Minor / questions
KeyPrefixIteratordrain does an extraget_rawper key even whenOnRemoval = ()(frame/support/src/storage/mod.rs). Every plainiter_keys().drain()on a non-counted map now pays a value read per key that only exists to feed the counter hook. Fine for correctness; could be specialized away if it ever matters somewhere hot.- New-pallet detection changed from "no keys under the pallet prefix" to "no storage-version key" (
hooks.rs). A pallet holding data that never wrote:__STORAGE_VERSION__:now gets its in-code version seeded after migrations, where before it was left untouched. I believe that's fine (arguably more correct) for this chain — just confirming it's intentional. BTreeSetappend is now decode-insert-rewrite, i.e. O(n) per append instead of an O(1) raw append. Correct fix for the dedup issue; worth remembering for large sets.RemovePallet/RemoveStoragegained a mandatoryLimittype param — breaking for downstream users of these helpers (none in-repo).
Checked and found sound
zk_tree_rootre-derivation on import (both theexecute_blockstate-root-gated path andfinal_checks), with the forged-root regression test.- Storage-version seeding order: in
Executive::execute_on_runtime_upgrade, custom migrations (COnRuntimeUpgrade) run beforeAllPalletsWithSystem::on_runtime_upgrade, so version-gated migrations targeting a new prefix correctly observe version 0, and the pallet hook doesn't clobber a version a migration set. - Scheduler preimage ownership: the scheduler already drops the preimage after execution and on cancel, so dropping on failed placement (including the referenda v3-shim path, where the proposal is handed off at approval) is consistent and closes the leak without double-drops.
- Counted map/N-map counter reconciliation (
try_append,migrate_key(s),set, partialclear_prefix, all drain iterators), including the hasher-collapse cases. - Fungible(s)
transfercrediting the actual debit: matches assets' reap semantics (decrease_balancecan return amount + dust there); for pallet-balances dust still routes throughhandle_dust, so no double count.transfer_approvedburning reap dust instead of over-crediting the delegate's destination is the right call. Balanced::depositissuance-headroom check mirrorsmint_into;Exactfails cleanly,BestEffortcaps at headroom, and the debt drop no longer silently saturates.set_codeweight drain viaregister_extra_weight_unchecked— no double counting withCheckWeight's refund sinceactual_weightis capped at the static weight.unhashed::clear_prefixcursor resumption,move_prefix_boundedcursor + disjointness assert (equal prefixes still early-return before the assert), nonce-MAX rejection,max_extrinsicbase_block subtraction, view-function 16 MiB decode bound, serde-exact camelCase genesis retention, and trailing-token rejection in pallet attributes.- The two acknowledged items (#93370 Callback upgrade-instability, #93448 constant-weight signer extensions) are reasonable as documentation-only given in-repo usage.
Verdict
Approve after fixes. The substance is sound and test coverage is strong, but this shouldn't merge until (1) fmt is fixed so the full CI matrix actually runs green, and ideally (2) the wormhole read count is bumped for the KnownDerivatives lookup. Item 2 (child::clear_storage cost) can be a follow-up since the function currently has no callers, but a doc warning now would be cheap.
The pre/post key counts used to attribute overlay-only removals walked the entire child trie twice, so a call with a small limit still paid O(total keys) next_key host calls — the same user-growable-state pattern the limit exists to prevent. Cap both walks at limit + 1024; when the cap saturates, overlay attribution degrades to zero (documented under-reporting) while the backend count stays exact. Unbounded clears keep exact counting since the host call is already O(total keys). Co-authored-by: Cursor <cursoragent@cursor.com>
NonWormholeAccounts::contains now also checks Utility::KnownDerivatives (via is_derivative), so is_ambiguous_account costs one more storage read than the model assumed. Bump per_transfer_weight and the signer reveal weight from 4 to 5 reads and update the read breakdowns in the comments. Co-authored-by: Cursor <cursoragent@cursor.com>
The OnRemoval hook forced every drain to read the removed value even for hooks that ignore it (the no-op default and the counted-map counter update), costing a get_raw host call per key on plain iter_keys drains. Add a NEEDS_VALUE associated const so such hooks opt out of the read. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # frame/support/src/migrations.rs # frame/support/src/storage/migration.rs # frame/support/src/traits/tokens/fungible/regular.rs # frame/support/src/traits/tokens/fungibles/regular.rs # pallets/balances/src/lib.rs
Both main and this branch added block_import_of_bad_zk_tree_root_fails, and the merge auto-resolution kept both copies. Keep the variant that pins the expected panic message. Co-authored-by: Cursor <cursoragent@cursor.com>


Harden FRAME and runtime against V12 medium-severity audit findings
Summary
This PR addresses 27 Medium-severity findings from the V12 audit (
frame-medium-low.md). Each item was triaged: 25 were confirmed and fixed, 2 were acknowledged and documented (no code change). Fixes are intentionally minimal — targeted at the specific failure mode, with regression tests where appropriate.The branch stacks on earlier
illuzen/frame-fixeswork (zk root validation, genesis bounds, bounded migrations, token accounting, etc.). Againstmainthis is 41 commits across 67 files (~3k insertions).Disposition
Medium fixes
Macro, metadata, and codegen
get_call_namesreturns an empty slice for unknown modules instead of panicking.#[pallet::weight(...)]are rejected at compile time instead of silently ignored.WrapperKeepOpaqueenforcesMaxEncodedLenon the wrapped type.META_RESERVEDis validated before decoding metadata payloads.CountedStorageNMapcounter prefixes are included in the procedural duplicate-prefix check.Storage, migrations, and counted maps
take().child::clear_storagecounters account for overlay-only removals.move_prefixrejects overlappingfrom/toprefixes to avoid self-reprocessing.CountedStorageMap::try_appendenforces bounds against the effectiveOnEmptydefault, not just stored length.BTreeSetstorage appends deduplicate entries.clear_prefixsubtracts only the removed entries instead of killing the global counter.unhashed::clear_prefixwalks from the supplied cursor and makes real progress.try_append,migrate_key/migrate_keys, andCountedStorageNMap::setreconcile counters against actual key existence.iter_keys,iter_prefix_values, etc.) wire throughOnRemovalCounterUpdate;KeyPrefixIteratorgains anOnRemovalhook.Scheduler, preimage, and tickets
Token / fungible traits
OnHoldtransfers enforce destination account existence.Frame-system and block limits
max_extrinsiccaps subtractbase_block.Runtime / wormhole
as_derivativepseudonym reveals it to the soundness counter (subtracting its balance fromPotentialWormholeBalance).as_derivative-wrapped transfers are statically counted;post_dispatchregisters extra block weight for proof-recording work that static analysis cannot see (e.g.Multisig::execute,recover_funds).Acknowledged (documented, not changed)
Callbackstores(pallet_index, call_index)and resolves against the call layout atcurrytime. No in-repo consumer persists callbacks across upgrades; constraint documented for future oracle work.check_nonce/check_non_zero_senderuse constant weights. Sound whenAccountIdhas a fixedmax_encoded_len()(true for Quantus); documented so future variable-length account types regenerate weights.Earlier fixes on this branch (from
illuzen/frame-fixes)These predate the medium batch but are included when merging this branch to
main:zk_tree_rooton block import (frame-executive)dev_accountsderivation (pallet-balances)RemovePallet/RemoveStoragedeletion; try-runtime tolerates intentional remaindersmove_prefix_boundedfor staged prefix movesBalanced::depositchecks issuance headroom before creditingCheckNoncerejectsMAXnonce instead of wrapping to zeroensure_frozen/decrease_frozenidempotency for permitted over-freezesDecodeAllforCallback::curryand NFTtyped_attributeImbalance::rationoverflow handling;SplitTwoWayscompile-time validationfungibles::transfer+do_transfer_approved)dynamic_paramsinner-attribute precedenceTest plan
cargo test -p frame-support --lib(228 tests)cargo test -p quantus-runtime --lib(36 tests, including wormhole extension lifecycle tests)cargo check --workspace --all-targetscargo test --releasedoctests (defensive trait docs fixed tono_run)Focused regression areas
try_append,migrate_key,set,clear_prefix, iterator drainsunhashed::clear_prefixcursor resumptionas_derivativeweight counting, post-dispatch weight reconciliation, derivative revealNote
High Risk
Touches block import (ZK header field), runtime upgrades/migrations, counted storage invariants, and fungible issuance/transfer/nonce paths—errors could brick sync, skew counters, or break accounting; breadth across executive, support, balances, assets, and frame-system warrants careful CI and runtime upgrade testing.
Overview
Hardens FRAME and related pallets against a batch of V12 medium-severity audit issues: bounded decoding/enumeration, storage counter correctness, migration safety, and token/accounting edge cases, plus ZK tree root validation on block import.
Block import & headers:
frame-executivere-derives and assertszk_tree_rooton import (with a regression test for forged roots).qp-headeris wired in as a normal dependency.Codegen & metadata: Unknown
get_call_namesmodules return[]instead of panicking. Runtime/pallet task iterators are lazy.#[pallet::weight]and related attributes reject trailing tokens. New-pallet storage version is seeded after migrations, not inbefore_all_runtime_migrations. Genesis field matching uses serde-exact camelCase.RuntimeMetadataPrefixedchecksMETA_RESERVEDbefore decoding payloads.WrapperKeepOpaqueenforcesMaxEncodedLenon decode/construct. Counted N-map counter prefixes participate in duplicate-prefix checks.dynamic_paramsonly injects outer args when the inner attribute has none.Storage & migrations: Child
takeclears corrupt entries;clear_storage/unhashed::clear_prefixfix overlay accounting and cursor resumption.move_prefixis bounded viamove_prefix_boundedand panics on overlapping prefixes.RemovePallet/RemoveStoragetake a per-upgrade key Limit. Counted map/N-map:try_appendrespectsOnEmptydefaults and key cardinality;set/migrate_*/partialclear_prefixand drain iterators keep counters in sync viaKeyPrefixIteratorOnRemoval hooks.BTreeSet::appenddeduplicates; boundedtry_appenduses effective absent length.Tokens & system:
OnHoldtransfers require an existing destination.depositchecks issuance headroom; expendable transfer credits reaped dust (assetstransfer_approvedusesburn_dust). Freeze/hold consideration rejects zero-footprint tickets;ensure_frozen/decrease_frozenidempotency fixes.CheckNoncerejects max nonce (no wrap).max_extrinsicvalidation uses derived caps. View functions decode with a 16 MiB mem limit and full input consumption. NFT/callback paths useDecodeAllwhere prefix decode was unsafe.MAX_DEV_ACCOUNTScaps genesis dev derivation.Docs only (acknowledged):
Callbackupgrade instability and constant-weight signer extensions are documented, not behavior-changed.Reviewed by Cursor Bugbot for commit 8fc3a01. Configure here.