Skip to content

Vesting again - #642

Closed
illuzen wants to merge 11 commits into
mainfrom
illuzen/vesting-again
Closed

Vesting again#642
illuzen wants to merge 11 commits into
mainfrom
illuzen/vesting-again

Conversation

@illuzen

@illuzen illuzen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Time-based vesting with repurchasable genesis endowments

Adds vested pre-mine endowments to the genesis presets, backed by a fork of FRAME
pallet-vesting that unlocks against wall-clock time instead of block numbers and
supports an optional per-schedule repurchaser that can reclaim unvested funds.

What's in here

1. Inline pallet-vesting (v45.0.0) into pallets/vesting

Vendored as-is from upstream and wired into the runtime (Vesting at pallet index 22),
with Config, benchmarks, and genesis plumbing. MinVestedTransfer = 1 UNIT;
UnvestedFundsAllowedWithdrawReasons = TRANSACTION_PAYMENT so locked accounts can
always pay fees (including for vest itself).

2. Fork it to vest against wall-clock time

Same approach as our scheduler: schedules are denominated in milliseconds since the
unix epoch, read from pallet_timestamp via a TimeProvider (Config::Moment = u64).
VestingInfo fields are now locked, per_ms, start (was locked, per_block,
starting_block). Vesting therefore tracks real time regardless of block production —
important for a PoW chain where block rate can vary.

3. Vested endowments in the genesis presets

Genesis vesting entries are (who, begin, length, liquid, repurchaser). All three
presets (dev, heisenberg, planck) give their endowed accounts a schedule starting at
GENESIS_VESTING_START_MS (currently 2026-08-05 UTC) that vests linearly over 365 days,
leaving 1 UNIT liquid for fees. Const helpers make the dates readable and are
compile-time checked:

const GENESIS_VESTING_START_MS: VestingMoment = utc_ms(2026, 8, 5);
const GENESIS_VESTING_LENGTH_MS: VestingMoment = days_ms(365);

Cliff-style terms ("nothing for a year, then linear") are just a future start.
A real per-allocation table (amounts, varying terms) is deliberately left for a
follow-up PR.

4. Optional per-schedule repurchaser

Each schedule may name a repurchaser account, stored in VestingInfo itself so it
survives schedule removals and merges:

  • force_remove_vesting_schedule is now callable by Root or the schedule's
    repurchaser. When the repurchaser repurchases a schedule, the still-unvested
    balance (locked_at(now)) is transferred to them; the already-vested portion stays
    with the holder. Root removal keeps the old behavior (schedule removed, all funds
    stay, unlocked).
  • Two schedules can only be merged if their repurchasers match (new
    RepurchaserMismatch error), so a repurchaser can't be washed away by merging;
    the merged schedule keeps it.
  • vested_transfer preserves a repurchaser set on the passed schedule. Schedules
    created through the VestingSchedule trait by other pallets have no repurchaser.
  • All three presets set the treasury (multisig) as repurchaser for the genesis
    endowments; pass None for schedules that only Root should touch.

Notes for reviewers

  • The vesting lock is only recomputed when vest / vest_other is called
    (upstream behavior, unchanged). vest_other is permissionless, so a keeper bot
    can unlock on users' behalf later — out of scope here.
  • Removing a schedule from an account with no vesting now errors NotVesting
    instead of InvalidScheduleParams (the call loads schedules to check the
    repurchaser anyway).
  • The force_remove_vesting_schedule benchmark still measures the Root path; the
    repurchase path adds one currency transfer not reflected in the weight. Flagged
    for a follow-up weight regeneration.
  • Storage migration note: this chain has no live networks with the upstream
    block-based VestingInfo encoding, so no migration is provided for the changed
    struct layout.

Test plan

  • cargo test -p pallet-vesting — 41 tests, including new coverage for:
    repurchaser can repurchase / owner and strangers cannot, repurchaser receives
    exactly the unvested amount (partial-vesting case), Root removal moves no funds,
    no-repurchaser schedules stay Root-only, vested_transfer preserves the
    repurchaser, merge with mismatched repurchasers fails, merged schedule keeps the
    repurchaser, genesis builds schedules with repurchasers.
  • cargo test -p quantus-runtime --lib — includes utc_ms/days_ms date math tests.
  • cargo test -p pallet-vesting --features runtime-benchmarks
  • cargo +nightly fmt --all -- --check and clippy clean of new errors.
  • Manual: launch a dev node, confirm endowed accounts show the vesting lock, and
    exercise a treasury repurchase end-to-end.

illuzen and others added 8 commits August 5, 2026 19:01
Vendors upstream FRAME vesting (45.0.0) as-is so we can build pre-mine lockups on top without changing pallet behavior yet.

Co-authored-by: Cursor <cursoragent@cursor.com>
Lock most of the standard genesis endowments behind a one-year linear schedule while leaving fee seeds and the wormhole test account fully liquid.

Co-authored-by: Cursor <cursoragent@cursor.com>
Schedules are now (locked, per_ms, start) in unix milliseconds via Config::TimeProvider (pallet_timestamp), mirroring the scheduler fork, so vesting durations hold under variable PoW block times. Genesis presets denominate vesting in ms with an explicit launch-time start.

Co-authored-by: Cursor <cursoragent@cursor.com>
utc_ms(year, month, day) and days_ms(n) let presets express vesting start/length as human-readable dates instead of raw millisecond literals, checked at compile time.

Co-authored-by: Cursor <cursoragent@cursor.com>
Each vesting schedule can name a canceller (set at genesis or on
vested_transfer) who may force_remove it, receiving the still-unvested
balance; Root removal keeps funds with the holder. Schedules only merge
when cancellers match. Genesis presets set the treasury as canceller.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The canceller field, accessors, error variant, and docs now use
"repurchaser"/"repurchase" terminology: the repurchaser receives the
still-unvested funds when removing a schedule.

Co-authored-by: Cursor <cursoragent@cursor.com>
@illuzen

illuzen commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

v12 audit plz

@v12-auditor

v12-auditor Bot commented Aug 6, 2026

Copy link
Copy Markdown

Warning

V12 could not find a connected workspace for this repository. Make sure the repository owner has signed in to V12 and connected their GitHub account (or that the V12 GitHub App is installed for the owning organization), then comment again.

illuzen and others added 2 commits August 7, 2026 00:01
Adds transfer_vesting_schedule so Root or the schedule's repurchaser can
move a schedule to another account without changing its terms (e.g. lost
wallet or multisig switch). Still-unvested funds move with it; already-
vested funds stay with the source. Shared auth/detach helpers dedupe the
repurchase and transfer paths.

Co-authored-by: Cursor <cursoragent@cursor.com>
A fully-vested schedule has nothing left to move and would silently
vanish on arrival; fail with ScheduleFullyVested instead. Also mention
the transfer power in the vested_endowments docs.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread runtime/src/genesis_config_presets.rs Outdated
/// instead of raw millisecond counts. Uses the standard civil-calendar day count
/// (Howard Hinnant's `days_from_civil`); valid for any Gregorian date from 1970 onwards.
/// Invalid dates (month 0/13, day 0/32) fail compilation via the `assert!`s.
const fn utc_ms(year: u64, month: u64, day: u64) -> VestingMoment {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is weird, do we really need this function?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just define vesting schedules in a number of days

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or blocks

Comment thread runtime/src/genesis_config_presets.rs Outdated
let doy = (153 * mp + 2) / 5 + day - 1; // [0, 365]
let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; // [0, 146096]
let days_since_epoch = era * 146097 + doe - 719_468; // 719468 = days from 0000-03-01 to 1970-01-01

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a little difficult to figure out this magic numbers function

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fable review

Reviewed the fork delta against pristine upstream pallet-vesting 45.0.0 rather than the raw diff, so vendored-unchanged code is out of scope and the findings below are all in code this PR authored or newly depends on. Ten finder angles, per-candidate verification, and a gap sweep; refuted candidates (merge per_ms clamp acceleration, missing spec_version bump) are omitted.

The time-based rework itself is solid — locked_at, ending_time_as_balance, and the merge math survive the ms conversion correctly, and the per_ms >= 1 invariant is preserved on every creation path. The problems are concentrated in the new repurchase/transfer feature and the genesis presets.

Blocking

1. The repurchase payout breaks the wormhole soundness counter's core invariant — pallets/vesting/src/lib.rs:638-642

PotentialWormholeBalance is credited per transfer whenever the recipient is ambiguous (pallets/wormhole/src/lib.rs:1415-1422), and debited only when an account first signs, subtracting its current total_balance at that moment (:1316-1318). The soundness of that pairing rests on an assumption the runtime states in so many words at runtime/src/transaction_extensions.rs:296-300:

"an ambiguous account cannot spend (spending requires signing, which reveals it), so its total balance equals the sum of credits it received"

detach_schedule_and_pay_unvested violates exactly this. ensure_can_manage_schedule accepts ensure_signed_or_root and requires only schedule.repurchaser() == Some(signer) (lib.rs:612-620), so force_remove_vesting_schedule and transfer_vesting_schedule move funds out of an account that has never signed, driven by a signed origin. The recipient is either the treasury (in NonWormholeAccounts, runtime/src/configs/mod.rs:685-688) or an already-revealed attacker, so nothing is re-credited. The never-signed target's eventual reveal then subtracts only its depleted balance, and the difference stays in the pool as phantom headroom — loosening ensure!(exits_after <= potential_balance, SoundnessInvariantViolation) (pallets/wormhole/src/lib.rs:876-878) in the unsafe direction: more wormhole exits permitted than were ever backed.

This is permissionless and unbounded, not just a genesis artifact. repurchaser is an encoded field of the VestingInfo call argument, and MinVestedTransfer is one ED, so any account can loop vested_transfer to a fresh never-signed target naming itself repurchaser → force_remove_vesting_schedule to pull the funds straight back, inflating the pool by the transferred amount every cycle with the same capital and only fee cost.

The repo already treats this precise bug class as a vulnerability — counter_derivative_hops_do_not_inflate_pool (runtime/src/transaction_extensions.rs:1744-1747) exists because "an attacker could bounce fixed capital between derivative addresses, adding the amount to PotentialWormholeBalance on every hop while never subtracting anything." The vesting path reintroduces it. It is also genuinely new: force_vested_transfer and Balances::force_transfer are ensure_root, mining-rewards and treasury only credit, and the one pre-existing analogue (reversible_transfers::recover_funds) is reachable only for genesis-seeded high-security accounts, a set that is empty in every production preset. Consequently the doc-comment on reduce_potential_balance (pallets/wormhole/src/lib.rs:1327-1346) claiming all untracked paths are "privileged (Root/governance) today, and the error is in the safe direction" is no longer true after this PR. The likely fix is to call reveal_account on the target (or otherwise debit the pool) whenever a repurchase or schedule transfer drains a never-signed account.

2. Repurchase can be blocked indefinitely by the holder — pallets/vesting/src/lib.rs:640

The payout is all-or-nothing: T::Currency::transfer(who, dest, unvested, AllowDeath)? moves the full locked_at(now) or reverts the whole extrinsic. Holds bypass vesting locks entirely — MutateHold::hold checks reducible_balance(who, Protect, Force), and Force zeroes untouchable, so frozen is ignored (the repo's own test asserts this: pallets/balances/src/tests/fungible_and_currency.rs:131-135 holds 99 of 100 with 60 locked). Any signed account can park its balance via ReversibleTransfers::schedule_transfer_with_delay (pallets/reversible-transfers/src/lib.rs:464-483:820 hold), and validate_delay (:678-688) enforces only a minimum delay, no maximum.

So a holder who wants to keep their unvested funds schedules a maximally-delayed reversible transfer, dropping free balance below unvested. Every subsequent force_remove_vesting_schedule / transfer_vesting_schedule by the repurchaser fails and reverts. The holder can't spend the funds either, so this is griefing rather than theft — but the clawback right that motivates the whole feature is defeated at will, permanently. Consider paying out min(unvested, reducible_balance), or seizing via a hold/slash-style primitive that a competing hold can't front-run.

3. Repurchaser operations address a mutable positional index — pallets/vesting/src/lib.rs:611-618

ensure_can_manage_schedule resolves the target purely by schedules.get(schedule_index) and checks only schedule.repurchaser() == Some(signer); nothing binds the call to the schedule the caller intended. Meanwhile every vest path rewrites storage as a compacted vec that drops fully-vested schedules and renumbers the survivors (:788-798, :819-833), and pruning is lazy (no on_initialize), so completed schedules linger until someone touches the account. vest_other is permissionless (:363-367).

Concretely: target holds [C(completed, unpruned), A(repurchaser R), B(repurchaser R)]; R submits force_remove_vesting_schedule(target, 1) intending A; anyone front-runs with vest_other, storage becomes [A, B], and R's call passes the repurchaser check against B and drains B's unvested balance instead. transfer_vesting_schedule (:553) moves the wrong schedule the same way. The "two schedules, same repurchaser" precondition is cheap to manufacture, not incidental — see finding 5. Upstream gated this call behind ensure_root (5a771e9b:lib.rs:477), so exposing index addressing to a non-Root caller is what makes it exploitable. Pass the expected schedule (or a hash of it) and verify before acting.

4. Vesting-triggered transfers escape wormhole proof-recording weight — runtime/src/transaction_extensions.rs:128-166

count_transfers carries the comment "this must stay in sync with the events matched by record_proofs_from_events_since" and enumerates only the four Balances::transfer_* calls plus statically-visible wrappers, falling through to _ => 0. record_proofs_from_events_since (:188-207) matches any Balances::Event::Transfer regardless of emitting pallet, and vesting's Currency is Balances (runtime/src/configs/mod.rs:215) — so the transfers at pallets/vesting/src/lib.rs:640 (transfer_vesting_schedule, repurchase branch) and :720 (vested_transfer, force_vested_transfer) are recorded while nothing is pre-charged. The entire cost lands in post_dispatch's register_extra_weight_unchecked (:328-334), which frame-system documents as explicitly not enforcing the block weight limit (pallets/frame-system/src/lib.rs:1931-1938).

This is material: per_transfer_weight (:122-126) is ~1.1 ms ref_time at tree depth 0 rising to ~6.5 ms at MAX_TREE_DEPTH, versus vested_transfer's declared ~470 µs — a 2×–14× undercharge, multiplied by Utility::batch. It is also a genuinely new gap rather than an existing pattern: every pre-existing non-Balances path that emits a recordable Transfer already pays for the tree insert in its own declared weight (pallets/reversible-transfers/src/weights.rs:79-85, pallets/wormhole/src/weights.rs:175,191), whereas pallets/vesting/src/weights.rs is stock upstream Substrate output with no zk-tree awareness and pallets/vesting/Cargo.toml has no zk-tree dependency. Vesting is the first transfer-emitting pallet covered by neither mechanism.

5. RepurchaserMismatch removes upstream's only self-service escape from schedule-slot spam — pallets/vesting/src/lib.rs:472-475

vested_transfer authenticates only the sender; target is an unconsenting lookup, and the caller controls the repurchaser field (VestingInfo derives Decode/TypeInfo, and with_repurchaser is pub). do_vested_transfer (:698-712) validates only locked >= MinVestedTransfer and non-zero locked/per_msstart is entirely unvalidated, so a far-future start keeps locked_at(now) == locked forever and the schedule is never pruned.

An attacker fills a victim's 28 slots (MAX_VESTING_SCHEDULES, runtime/src/configs/mod.rs:224) using 28 throwaway repurchasers for 28 × MinVestedTransfer = 28 × ED = 0.028 UNIT plus fees, every planck of it reclaimable later via force_remove_vesting_schedule. Upstream's merge_schedules has no repurchaser check, so a spammed victim could always collapse the slots; with the new guard every pair mismatches, and removal is Root-or-repurchaser only. The victim permanently cannot receive further vested transfers absent a governance call. The pallet's own test merge_schedules_with_different_repurchasers_fails (tests.rs:1285-1296) is the attack primitive verbatim.

6. The planck faucet's entire endowment is vested — runtime/src/genesis_config_presets.rs:480-481

planck is documented as a public testnet with a live faucet (node/src/chain_spec.rs:76, docs/RUNTIME_SURFACE.md:236), and its only endowed account is the faucet. vested_endowments locks all but GENESIS_VESTING_LIQUID = 1 UNIT of its 100,000 UNIT. Before 2026-08-05 the faucet can dispense 1 UNIT total; after, ~274 UNIT/day and only once someone calls vest(). Everything else fails on the lock. The faucet almost certainly should not be in vested_accounts.

7. planck's designated repurchaser cannot fund the call it exists to make — runtime/src/genesis_config_presets.rs:482

Treasury signers are seeded exactly 1 UNIT each (signer_fee_seed), but reaching force_remove_vesting_schedule through the treasury multisig costs ~2.63 UNIT per signer: MultisigFee 0.6 UNIT to create (the multisig is never seeded in Multisigs at genesis — only create_multisig inserts), then ProposalFee 1 UNIT + 1%×3 signers = 1.03 UNIT burned KeepAlive, plus a 1 UNIT ProposalDeposit reserve (runtime/src/configs/mod.rs:554-557, pallets/multisig/src/lib.rs:641-657). The propose fee alone exceeds a signer's whole balance, so the repurchase mechanism is inert on planck as shipped. The 1 UNIT seed predates this PR, but this PR is what makes the treasury the sole non-Root actor for the new feature.

Should fix

8. UnvestedFundsAllowedWithdrawReasons is inert in this runtime — runtime/src/configs/mod.rs:208-210

The vendored balances pallet ignores lock reasons entirely: ensure_can_withdraw takes _reasons (underscore-prefixed, unused) and checks only new_balance >= frozen (pallets/balances/src/impl_currency.rs:333-343), while update_locks sets frozen = max(lock amounts) with no regard for reasons (pallets/balances/src/lib.rs:1207-1216). Fees go through FungibleAdapter (:464), which enforces the same frozen. So the carve-out buys nothing — a vested account can only ever spend free - frozen, i.e. the 1 UNIT liquid. The PR description's "locked accounts can always pay fees (including for vest itself)" does not hold; once the liquid margin is gone the account cannot even call vest() and needs a third party's vest_other. Either drop the config item as misleading or give genesis accounts a liquid margin sized deliberately.

9. Repurchase path is underweighted — pallets/vesting/src/lib.rs:519, benchmarking.rs:434

Acknowledged in the PR description, restating with the specifics: the benchmark dispatches RawOrigin::Root, which skips the payout branch entirely, so the extra Currency::transfer (two account mutations, possible account creation/reaping) is unmeasured in both the #[pallet::weight] and the returned PostDispatchInfo.

10. transfer_vesting_schedule weights are hand-composed despite a real benchmark existing in this PR — pallets/vesting/src/weights.rs:290

force_remove_vesting_schedule(l,s) + vested_transfer(l,s) double-counts both extrinsics' base weight and overlapping fixed costs, in a file whose header declares it auto-generated. The PR adds a proper transfer_vesting_schedule benchmark (benchmarking.rs:446) whose output is never wired in, and the call returns plain DispatchResult (lib.rs:548) so there's no actual-count refund the way force_remove_vesting_schedule has.

11. AllowDeath payout can reap the holder and burn their vested remainder — pallets/vesting/src/lib.rs:640

With a single schedule the lock is removed before the transfer, so a payout leaving 0 < free < ED dusts the remainder (pallets/balances/src/lib.rs:1159-1164) and reaps the account; DustRemoval = () (runtime/src/configs/mod.rs:196) burns it. That contradicts the doc two hundred lines up — "only the already-vested portion stays with target" (lib.rs:493-494) — and resets the holder's nonce. Bounded below ED, but silent. (The multi-schedule variant is safe: a remaining lock keeps a consumer ref, so the transfer fails and reverts cleanly rather than orphaning storage.)

12. Repurchase emits no distinguishing event — pallets/vesting/src/lib.rs:504

A repurchase is a signed third party seizing funds, yet it produces only VestingUpdated/VestingCompleted plus a bare Balances::Transfer — on-chain indistinguishable from a routine Root removal. The sibling transfer_vesting_schedule added in the same PR does get a VestingTransferred event. More broadly, folding a user-facing operation into a force_* call behind an origin branch is what produces findings 8 and 11 together; a dedicated repurchase_schedule extrinsic with its own call index, benchmark, and event would resolve both and leave force_remove_vesting_schedule exactly as upstream.

13. VestingTransferred.schedule_index is the stale source index — pallets/vesting/src/lib.rs:581

The emitted index refers to the source's pre-call slot, which the call itself just invalidated, and it generally won't match where the schedule lands on dest. do_add_vesting_schedule also emits VestingCreated on the destination, so a moved schedule is indistinguishable from a newly minted one. No test asserts events for either new extrinsic.

14. On dev and heisenberg the repurchaser is a multisig of the vested accounts themselves — runtime/src/genesis_config_presets.rs:139-141, 154-166

development_treasury_account and heisenberg_treasury_account are both 2-of-3 multisigs over dilithium_default_accounts() — exactly the set passed as vested_accounts. Any two of the three can repurchase each other's schedules and route the funds back, so vesting restrains nothing on those presets. Acceptable for test networks, but these presets are the template a real launch gets copied from.

15. utc_ms accepts non-existent dates — runtime/src/genesis_config_presets.rs:47

assert!(day >= 1 && day <= 31) doesn't validate against the month's real length, so utc_ms(2027, 2, 29) compiles and silently yields 2027-03-01. The assert!s are there to fail compilation on bad input; this class slips through. Relatedly, GENESIS_VESTING_START_MS = utc_ms(2026, 8, 5) is already in the past — the doc comment says to set it to the intended launch time, so it needs updating before any launch, and a chain launched a year late would have fully-liquid "vested" endowments.

Cleanup

  • DRY (~/.claude/CLAUDE.md: "Strictly follow DRY... Duplicate code must be avoided at all costs"): all three presets repeat the same vested_accounts construction + comment + vested_endowments(...) block (genesis_config_presets.rs:308/371/481 and 323/385/493); vested_accounts re-invokes dilithium_default_accounts() instead of reusing the list built one line above. The 5-tuple type is spelled out in both vested_endowments and genesis_template.
  • set_time is hand-rolled three times (benchmarking.rs:35, mock.rs:76 ×2) writing pallet_timestamp::Now directly, bypassing Timestamp::set_timestamp (pallets/timestamp/src/lib.rs:346) and its DidUpdate/OnTimestampSet semantics.
  • migrations.rs is dead — it migrates a pre-BoundedVec encoding no Quantus network has ever had, isn't in the runtime Migrations tuple, and now decodes with the 4-field VestingInfo, so if it ever ran for its stated purpose translate would silently delete every schedule it failed to decode.
  • Redundant reads in force_remove_vesting_schedule: decode_len (:510), then Vesting::get inside ensure_can_manage_schedule (:611), then again via remove_vesting_schedule — three decodes of the same up-to-28-entry vec. transfer_vesting_schedule likewise computes now/locked_at twice (:557, :633).
  • Comments (~/.claude/CLAUDE.md: "No comments except very minimal ones"): new narrative comment blocks at lib.rs:555-556, runtime/src/configs/mod.rs:216-217, and elsewhere in the fork delta.
  • VestingSchedule::remove_vesting_schedule (lib.rs:958) still ignores the repurchaser model — latent today (nothing in this repo consumes the trait) but it would let a future consumer pallet delete a repurchaser-backed schedule with no payout.
  • Nothing rejects repurchaser == holder, which turns force_remove_vesting_schedule into a self-unlock. The funder chooses it, so it's a footgun rather than an exploit, but worth an explicit check.
  • Test coverage for the new paths is happy-path only: no test exercises a blocked payout, an index shift between submission and execution, account reaping, or events.

Verdict: requesting changes — findings 1–7 are blocking, with finding 1 the most serious: the repurchase path gives a signed origin a way to drain a never-signed account, which permanently inflates the wormhole soundness ceiling and is exploitable in a loop with fixed capital.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kimi Review

Reviewed by diffing the vendored pallet against upstream pallet-vesting v45.0.0 (crates.io) to isolate the fork changes, plus the runtime/genesis diffs. Verified locally on this branch:

  • cargo test -p pallet-vesting — 43 passed
  • cargo test -p quantus-runtime --lib — 41 passed
  • cargo check -p pallet-vesting --features runtime-benchmarks — clean

Verdict: no blocking issues. Non-blocking notes below.

Non-blocking findings

  1. PR description vs code — the description says MinVestedTransfer = 1 UNIT and UnvestedFundsAllowedWithdrawReasons = TRANSACTION_PAYMENT, but runtime/src/configs/mod.rs:207-210 sets EXISTENTIAL_DEPOSIT (= MILLI_UNIT, runtime/src/lib.rs:102) and except(TRANSFER | RESERVE). The code is the sensible (upstream-standard) choice; the description should be fixed.

  2. pallets/vesting/src/migrations.rs — the V0→V1 migration now decodes old storage using the new VestingInfoOf<T> layout, so it would mis-decode any pre-fork data. Harmless today (pallet is freshly deployed and genesis writes V1, lib.rs:258), but it's dead, misleading code — consider deleting the module rather than leaving a broken migration around.

  3. Repurchase can be bricked by fee draindetach_schedule_and_pay_unvested (pallets/vesting/src/lib.rs) does transfer(who, dest, unvested), which fails if the holder's free balance fell below unvested. That's reachable: UnvestedFundsAllowedWithdrawReasons deliberately lets locked funds pay fees, so a holder burning their balance on fees can make repurchase/transfer_vesting_schedule fail until the account is topped up. Self-destructive for the holder (and fees partially flow back to treasury), so not a practical attack — but worth a doc comment, or a conscious decision to transfer min(unvested, free).

  4. transfer_vesting_schedule weight is hand-composed (weights.rs: force_remove + vested_transfer) even though a real benchmark was added in benchmarking.rs. It's an overestimate so it's safe, but since the benchmark exists, regenerate and paste the actual output — same follow-up already flagged for the repurchase path of force_remove_vesting_schedule.

  5. Genesis datesGENESIS_VESTING_START_MS = utc_ms(2026, 8, 5) is already in the past, so any network launched now starts partially vested (documented in the code — just make sure it's set per-launch). Also confirm the planck faucet being vested down to 1 UNIT liquid is intended: its payouts will be limited to the vested-so-far amount.

  6. Timestamp caveat — vesting now tracks pallet_timestamp, so miners can nudge unlock times within the timestamp rules (MinimumPeriod = 100ms here, plus consensus-level future-drift bounds). A fine tradeoff for a PoW chain, but worth one sentence in the pallet README so the security assumption is written down.

Nice touches: utc_ms/days_ms const helpers with real date-math tests, merge refusing mismatched repurchasers, and rejecting fully-vested schedule transfers instead of silently dropping them.

=====

Still blocked, see above

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Fable review

Schedules are configured in days, so the only absolute date is the
vesting start; hardcode its timestamp (with a regeneration recipe) and
drop the const calendar arithmetic. A sanity test guards the constant
against seconds-vs-ms and fat-finger errors.

Co-authored-by: Cursor <cursoragent@cursor.com>

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review

Request changes: the repurchaser paths introduce a critical wormhole double-spend and several state/weight accounting failures.

Blocking findings

  1. Critical — repurchasing enables a wormhole double-spend (pallets/vesting/src/lib.rs:640, runtime/src/transaction_extensions.rs:128-166)

    A caller can vested_transfer funds to a fresh wormhole address while naming itself as repurchaser. The balance transfer creates a valid wormhole leaf and increases PotentialWormholeBalance. The caller can then invoke force_remove_vesting_schedule and transfer the unvested funds back without the target ever signing, so the target is never revealed and its pool contribution is never removed. The original leaf remains exit-able, allowing the caller to reclaim the transparent funds and mint the same value through a wormhole exit. This can be repeated with the same capital. Repurchaser-driven outflows from ambiguous accounts must be forbidden or fully reconciled with the wormhole proof and soundness accounting, with an end-to-end regression test.

  2. High — a failed payout leaves the schedule removed and its funds unlocked (pallets/vesting/src/lib.rs:627-642)

    detach_schedule_and_pay_unvested calls remove_vesting_schedule before Currency::transfer, and neither dispatchable runs inside a storage transaction. Dispatch errors do not automatically roll back pallet writes. If the transfer returns FundsUnavailable or BelowMinimum, the extrinsic reports failure after the schedule and lock were already removed. This is reachable, for example, because a holder can move frozen free balance onto a reversible-transfer hold, or because transfer_vesting_schedule can target a nonexistent account after the remaining unvested amount falls below ED. Make the detach, payout, and reattach sequence atomic and add storage-noop tests for both failure modes.

  3. High — vesting transfers bypass the wormhole proof-recording weight charge (runtime/src/transaction_extensions.rs:128-166, pallets/vesting/src/lib.rs:640,720)

    count_transfers does not match any RuntimeCall::Vesting, while post-dispatch records every emitted Balances::Transfer. The shortfall is registered with register_extra_weight_unchecked, which does not enforce the block limit. A Utility::batch of vesting transfers can therefore perform ZK-tree writes that were absent from pre-dispatch weight. Count every vesting call that can emit a transfer, charging the worst-case branch where origin-dependent.

  4. High — the pallet weights still describe the old storage layout (pallets/vesting/src/weights.rs:90-293)

    The checked-in file is the upstream 2025 output and still models proof growth as s * 36 with Vesting::Vesting max_size = 1057. Runtime schedules are now 73 bytes each (u128 + u128 + u64 + Option<AccountId32>), so every call touching the schedule vector underestimates proof size. The new transfer_vesting_schedule benchmark exists but its generated result is replaced by a hand-composed estimate, and the signed repurchase payout is not benchmarked. Regenerate all weights against the final runtime and benchmark the actual worst-case branches.

  5. Medium — multiple genesis schedules are under-locked (pallets/vesting/src/lib.rs:266-288)

    Every genesis entry appends a schedule but calls set_lock(VESTING_ID, who, locked, ...), overwriting the same lock with only the latest schedule's amount. For duplicate who entries, storage reports the sum of schedules while the balance lock covers only the last one until a later vest operation recomputes it. Accumulate each account's current unvested total and set the lock to that total; extend generates_multiple_schedules_from_genesis_config to assert usable balance.

Validation

  • cargo test -p pallet-vesting — 43 passed
  • cargo test -p quantus-runtime --lib — 41 passed
  • cargo test -p pallet-vesting --features runtime-benchmarks — 53 passed
  • cargo +nightly fmt --all -- --check — passed
  • Focused Clippy could not reach this pallet because vendored frame-support-procedural-tools fails -D warnings on pre-existing lints.
  • GitHub's Clippy/doc job failed before checkout due to an Actions service outage; Linux build/tests and formatting passed.

These findings are blocking; no approval is recommended until they are resolved.

@illuzen illuzen closed this Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants