Audit fixes: harden pool init against PDA pre-funding griefing#2
Merged
Conversation
Addresses the medium + low findings from the post-merge security audit. Medium — init griefing: the pool (["pool", mint]) and vault (["token_vault", pool]) PDAs are deterministic, so anyone could send 1 lamport to them before the real authority initializes; system create_account fails on a non-zero balance, permanently bricking pool creation for that mint. initialize.rs now uses a create_pda_account helper that tolerates a pre-funded PDA (fund-to-rent + allocate + assign under the PDA seeds — operations a griefer cannot perform) and adds explicit re-init guards. Adds an init_griefing ProgramTest that pre-funds both PDAs and asserts initialization still succeeds. Low — total_residual_unpaid: documented as legacy-only (the always-close change means current code never increments it; kept for layout compat + draining pre-upgrade residual balances). Low — member_count: documented as best-effort (accurate only when the optional metadata account is supplied on stake AND every close path; display-only, never affects funds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…paths
Previously the metadata account was optional on the count-changing instructions,
so member_count could drift (e.g. a full unstake that closed the account without
passing metadata never decremented). Make the metadata PDA a REQUIRED account on
every member-count-changing instruction — Stake, StakeOnBehalf, Unstake,
CompleteUnstake, CloseStakeAccount — and route all updates through a shared
state::update_pool_member_count helper that:
- rejects a non-canonical metadata account (InvalidPDA), so the correct PDA is
always supplied, and
- tolerates an uninitialized account (pool that never created metadata) by
skipping the update — so metadata-less pools keep working.
This makes the counter exact (+1 on a new stake, -1 on close). Unstake and
CompleteUnstake also now require the system program positionally (it precedes
metadata and is used for legacy realloc).
Breaking ABI change: clients must pass the (derived) metadata PDA on these
instructions. TS helpers updated to always pass it; lib.rs account tables and
idl.json updated; legacy_migration complete test passes the metadata account.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sign ctx.stake now always passes the metadata PDA, so staking on a metadata pool increments member_count. Repurpose the old "stake without metadata is backwards compatible" test (which asserted member_count stayed 0) to assert it increments to 1 — matching the now-required metadata account. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the findings from the post-merge security audit. No critical/high issues were found; this PR fixes the one Medium and both Low items.
Medium — Pool/vault init griefing (fixed)
The pool (
["pool", mint]) and vault (["token_vault", pool]) PDAs are deterministic, andsystem_instruction::create_accountfails if the destination already holds lamports. So anyone could send 1 lamport to a not-yet-created pool/vault PDA and permanently blockInitializePoolfor that mint (no theft, but a denial of pool creation).Fix (
initialize.rs):create_pda_accounthelper that tolerates a pre-funded PDA: if the target already holds lamports, it tops up to rent-exemption thenallocate+assigns under the PDA's own seeds — operations a griefer cannot perform.data_is_empty): a griefer can only add lamports, never allocate/assign, so empty data reliably means "not yet initialized".Low —
member_countaccuracy (fixed)member_countcould drift upward because the metadata account was optional on the count-changing instructions (e.g. a full unstake that closed the account without passing metadata never decremented).Fix: the metadata PDA is now a required account on every member-count-changing instruction —
Stake,StakeOnBehalf,Unstake,CompleteUnstake,CloseStakeAccount— and all updates go through a sharedstate::update_pool_member_counthelper that:InvalidPDA), guaranteeing the correct PDA is always supplied, andThe counter is now exact (+1 on a new stake, −1 on close).
Unstake/CompleteUnstakealso require the system program positionally (it precedes metadata; used for legacy realloc).Breaking ABI change: clients must pass the (derived) metadata PDA on these instructions.
lib.rsaccount tables andidl.jsonare updated accordingly; the reference TS client passes it automatically.Low —
total_residual_unpaid(documented)After the always-close-on-full-unstake change, current code never increments this field; it's only decremented to drain residual balances from pre-upgrade accounts. It can't be removed without breaking
StakingPool's binary layout, so it's documented as legacy-only.Tests
tests/init_griefing.rs(ProgramTest): pre-funds both PDAs with stray lamports, assertsInitializePoolstill succeeds (pool program-owned with correct state; vault is a token account whose authority is the pool PDA).tests/legacy_migration.rsupdated for the now-required metadata account onCompleteUnstake.init_griefing(1) +legacy_migration(2);tsc --noEmitclean;cargo build-sbfsucceeds.Informational audit items (FixStakeAccount PDA re-derivation, SyncPool rounding drift, take_fee_ownership account forwarding, set_metadata UTF-8 truncation) were reviewed and judged safe / not worth changing.
🤖 Generated with Claude Code