V12 high security accounts

[illuzen]

Security review fixes (v12)

Addresses the items from the security review. Each item was first verified as a real
issue, then covered by a red (failing) test where applicable, fixed, and re-verified
green. One item was acknowledged without a code fix and one required only
documentation (details at the bottom).

Recovery pallet

• close_recovery fund loss (dac2bd0c): close_recovery discarded
  repatriate_reserved errors and deleted the active-recovery state anyway, silently
  losing the rescuer's deposit. The repatriation is now infallible best-effort: on a
  shortfall the remainder is unreserved back to the rescuer, and errors are logged
  instead of corrupting state. Also documented that the recovery link survives account
  reaping (9e34e3d1).
• poke_deposit phantom deposits (3ac6acef): on an unreserve shortfall the call
  recorded the new (larger) deposit anyway, overstating claims on shared reserves. It
  now fails transactionally with BadState, reverting everything.
• set_recovered consumer reference (df70c6db): root-created proxies skipped the
  consumer reference that claim_recovery takes, so the rescuer account could be
  reaped while holding an active proxy. set_recovered now takes the same reference
  (once per proxy, maintained across replacement).

Balances pallet

• can_reserve / reserve disagreement (467cb548): can_reserve used a simpler
  rule than reserve and mispredicted it in provider/consumer edge cases.
  ensure_can_reserve now exactly models reserve's behavior.
• Genesis collisions with dev accounts (d7e1ffa2): explicitly endowed genesis
  balances that collided with derived dev accounts silently overwrote them; genesis now
  panics on the collision.
• Benchmarks assumed zero ED (5a8f3c25): benchmark assertions were driven by the
  insecure_zero_ed feature rather than the configured ExistentialDeposit;
  expectations are now derived from the actual ED and real reaping rules.
• Genesis dev accounts missing from TotalIssuance (a38c8a2a):
  derive_dev_account minted free balance without updating TotalIssuance, breaking
  the issuance invariant at genesis. Dev-account creation now maintains the delta and is
  opt-in for tests.
• ensure_upgraded failsafe mint uncounted (a75d4025): the legacy-account ED
  top-up minted funds without recording them in TotalIssuance; now recorded.
• derive_dev_account panics (570594d2): internal assert!s became structured
  errors; genesis surfaces them with a descriptive panic message.
• Inactive-issuance migration duplicates (d209bccc): migrate_v0_to_v1 summed
  total_balance per list entry, so a duplicated account ID permanently overstated
  InactiveIssuance. Accounts are now deduplicated before summing.

Scheduler pallet

• Non-atomic reschedule (f26c8331): a failed destination placement in
  do_reschedule(_named) could destroy the source task. Placement is now attempted
  first; source state is only vacated on success.
• Orphaned retry config (c13f698d): the PermanentlyOverweight terminal path
  removed the task but left its Retries entry behind; it is now cleaned up like the
  unavailable-call path.
• Invalid retry periods (c30cbac7): set_retry(_named) accepted zero periods
  (retry clone silently discarded) and timestamp periods not aligned to
  TimestampBucketSize (retry lands in an agenda key the servicing loop never visits,
  holding its preimage forever). Both are now rejected with InvalidRetryPeriod.

Utility pallet

• as_derivative weight overcharge (2efdd309, c7aa17d1): every call charged
  for the first-use pseudonym reveal (two storage writes) even though repeat uses skip
  it. Repeat uses now refund the reveal writes post-dispatch, and the benchmark
  measures the repeat-use path so the reveal cost is not also baked into the base
  weight (which would have double-charged first use).

Runtime

• Undercharged wormhole proof weight (94510c34): the proof-recording transaction
  extension did not recurse into Utility::if_else (now charges the worst-case branch)
  or Utility::dispatch_as_fallible, letting wrapped transfers evade the per-transfer
  weight surcharge.
• Reversible-transfer weight vs ZK-tree depth (4cf2ef6c): execute_transfer had
  a fixed weight that ignored depth-dependent ZK-tree insertion costs; its weight now
  scales with the tree's current depth, mirroring pallet-wormhole.
• recover_funds atomicity (8fd7b255): a failure in the final transfer_all
  sweep rolled back the entire extrinsic — re-arming the very pending transfers the
  guardian was cancelling on a compromised account. The sweep is now best-effort:
  cancellations and hold releases stick, a failed sweep emits RecoverySweepFailed,
  and the guardian can retry.

Treasury / mining rewards

• Treasury defaulted to the keyless minting sentinel (4f258855): the genesis
  default set treasury_account to [1u8; 32] — the same keyless sentinel used as
  the minting source — so a chain spec omitting the treasury section silently sent
  every treasury payout to an unspendable address. The default now configures nothing
  (FRAME requires the Default impl to exist and build); an unconfigured treasury
  fails loudly on first use and a half-configured genesis is rejected at build. All
  production presets already set distinct treasury accounts explicitly.

Node

• Miner result-channel DoS (de3c79ca): all miner connections share one bounded
  result channel drained only while the mining loop waits for results; a blocking
  send let one flooding miner park every other connection handler, cutting honest
  miners off from jobs and seal submission. Forwarding now uses try_send (dropping
  overflow), and a connection that keeps overflowing an already-full channel is
  disconnected.
• Unauthenticated peer-topology RPC (7286c0ac): peer_getBasicInfo returned the
  node's peer ID, connected peers, and external/listen addresses to any caller who
  could reach the RPC listener — reconnaissance data for eclipse/partition attacks.
  The handler now enforces the per-connection DenyUnsafe policy (local connections
  or --rpc-methods unsafe only), matching upstream's treatment of system_peers.
  Renamed to peer_getNetworkInfo to describe what it returns — breaking for any
  external tooling still calling the old method name.

Acknowledged without a code fix

• Timestamp inherent wall-clock drift: check_inherent validates block timestamps
  against the importing node's local clock with a tight drift bound, which can reject
  valid blocks on skewed clocks. Acked as inherent to distributed timekeeping; no
  change.
• Header hash commits only 32 bits of the block number (344df456): unreachable
  with the runtime's BlockNumber = u32, and committing the full width would change
  every block hash and the wormhole circuit layout. Documented the constraint at the
  truncation site instead; the generic header must not be instantiated with a wider
  number type.

Test plan

• Red test written before each fix where applicable; verified green after
• Full test suites pass for every touched crate (pallet-recovery,
  pallet-balances, pallet-scheduler, pallet-utility,
  pallet-reversible-transfers, pallet-treasury, quantus-runtime, quantus-node,
  qp-header)

[n13]

Reviewed the full diff with the PR checked out, tracing each fix into the surrounding code. Every claim in the description matches the implementation, and each fix ships with a test that actually exercises the failure mode it claims to prevent. Details of what I verified, then findings (all minor, none blocking).

Verified against the code, beyond the diff hunks:

• Balances genesis: TotalIssuance::put(total) from the explicit balances list runs before derive_dev_account, so the per-account TI deltas correctly add on top rather than being overwritten (pallets/balances/src/lib.rs:587 → :610). The pre-existing duplicate-balances assert still runs before the new dev-collision assert, so a plain duplicate in balances is caught with the right message.
• ensure_can_reserve: the three checks (free sufficiency, consumer-ref availability when reserved == 0 && frozen == 0, and the ConsumerRemaining path when the reserve strips the last free-balance provider) mirror exactly what try_mutate_account's provider/consumer bookkeeping enforces, and the new boundary test pins the agreement in both directions (extra provider ref flips both answers).
• as_derivative: the declared weight annotation (pallets/utility/src/lib.rs:283-290) still charges the first-use reveal (reads_writes(1, 2)), the post-dispatch path refunds only the two writes on repeat use, and the benchmark change (pre-seeding KnownDerivatives) is what keeps the reveal out of the generated base — the three pieces are consistent, and the refund test asserts the exact writes(2) delta.
• if_else worst-case charging: sound, because the nested main.dispatch(origin) runs in its own storage layer — a failed main rolls back, including any proof-recording effects, so max(main, fallback) covers the actual recorded transfers.
• count_transfers coverage: with dispatch_as_fallible and if_else added, every call-wrapping extrinsic reachable by signed origins is now covered (batch variants sum, single-inner wrappers recurse, as_recovered included); ScheduleOrigin is EnsureRoot and there is no sudo pallet, so scheduler-wrapped calls are not an open evasion path. Non-statically-countable wrappers are reconciled post-dispatch per the existing note.
• Depth-scaled execute_transfer weight: pallet_zk_tree::insert_leaf_db_ops() exists and reads the live depth; the depth-blind () impl prices at MAX_TREE_DEPTH and the test asserts it dominates SubstrateWeight at any depth.
• Treasury: all runtime presets set treasury_account/treasury_portion explicitly; account_id()/portion() panic with a clear message when unconfigured; half-configured genesis is rejected at build.
• Miner flood fix: try_send + consecutive-drop bound with reset-on-success is the right shape — an honest miner losing occasional results to someone else's flood can never accumulate to a disconnect (tested).
• RPC gating: check_if_safe on the jsonrpsee extensions matches upstream's treatment of system_peers; the CLI help and query_peers.sh document the breaking rename and the new --rpc-methods unsafe requirement.

Minor findings (non-blocking):

1. pallets/recovery/src/lib.rs:481 — set_recovered maps the inc_consumers failure to BadState, but the reachable cause is the rescuer account simply not existing (no provider refs), which is a caller-input error rather than corrupted pallet state. A dedicated error (or DispatchError::NoProviders passthrough) would make the root-caller diagnosis clearer. Fine to leave if you'd rather not grow the error enum.
2. pallets/scheduler/src/lib.rs:860 / :970 — the reordered reschedule now emits Scheduled (from place_task) before Canceled, the reverse of the previous order. Purely cosmetic, but indexers that reconstruct task lifecycles from event order will see the flip; worth a mention in release notes alongside the peer_getBasicInfo rename.
3. pallets/reversible-transfers/src/weights.rs:66 — TREE_KEY_POV = 2600 duplicates the same hand-picked figure in pallet-wormhole's weights by copy. Exporting one shared constant from pallet-zk-tree would keep the two from drifting when the tree layout changes.
4. The Build & Test matrix was still pending at review time; format/clippy/doc checks pass. Merge should wait for the matrix as usual.

Verdict: approve. The fixes are correct, conservatively designed (over-charge rather than under-charge, best-effort only where rollback would be strictly worse, fail-loud genesis), and the test coverage is exemplary — red-test-first shows in how precisely the tests pin the failure modes.