Audit fixes: critical security bugs, truthful docs, and skill-standards sweep#67
Open
mikemaccana wants to merge 27 commits into
Open
Audit fixes: critical security bugs, truthful docs, and skill-standards sweep#67mikemaccana wants to merge 27 commits into
mikemaccana wants to merge 27 commits into
Conversation
…-dash and onchain spelling fixes - CONTRIBUTING.md: describe the real test setup (Rust LiteSVM for Anchor, QuasarSVM for Quasar) instead of the TypeScript/tsx instructions that contradicted every project's 'test = cargo test' - compression READMEs: the three anchor variants claimed 'no tests/' while shipping full LiteSVM suites; the three quasar twins claimed real tests while shipping comment-only files. Both now tell the truth - token-swap/anchor: fix B-to-A swap underpayment (trader paid 'output' of token B instead of 'input_amount'); add B-to-A conservation and invariant tests so both directions of the symmetric flow are covered - remove dead anchor migrations/deploy.ts scripts, escrow .mocharc.json and register.js, and account-data's ts-mocha script pointing at a nonexistent file - delete the '[registry] section removed' history comment from every Anchor.toml; rewrite removal-history comments as present-state ones - replace em-dashes with regular dashes; 'onchain' spelling; 'Token Extensions' instead of 'Token-2022' in prose; fix 'succesfully' typos - add missing root READMEs (betting-market, nft-operations), root README Tools section for shank-and-codama; fix cnft-vault fixtures README copy-paste https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…es under instructions/admin - fee = ceil(gross * fee_bps / 10_000): flooring leaked up to 1 minor unit of quote per fill to the maker, which many tiny fills could industrialise. Add a rounding-edge regression test (gross = 501 at 10 bps must charge 1, not 0) and update the README walkthrough numbers - fix pre-existing settle_funds test assertions that compared raw token balances against lot counts (failed at HEAD; CI path filters hid it) - correct the order_book::remove comment that claimed an O(log N) tree lookup above what is actually a linear arena scan https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…nd unit-scaling bugs in both variants Anchor variant: - contribution window was inverted (contributions only allowed AFTER the deadline); refund window was inverted the other way. Tests hid both by using duration 0. Windows now read: contribute while elapsed < duration, refund once elapsed >= duration and the target was not met - one major unit is 10^decimals minor units: minimum contribution used 1^decimals (always 1) and the minimum target used 3^decimals - all balance arithmetic is checked_*; state updates before transfer CPIs - migrated to token_interface types and transfer_checked throughout - checker now compares the state-tracked current_amount (a direct vault donation can no longer trigger early release) and closes the vault token account so its rent is recovered - tests rewritten with nonzero duration and Clock warping on both sides of every deadline boundary; account structs renamed *AccountConstraints Quasar variant: - refund's contributor_account was not bound to the signer: anyone could pass a victim's Contributor record with their own token account and drain the vault. It is now PDA-bound to (fundraiser, contributor) - contribute's fundraiser/contributor_account/vault were unconstrained and unlinked; now has_one(vault), has_one(maker), and PDA-derived addresses - no instruction ever created a Contributor account (the program was unusable onchain); contribute now init(idempotent)s it - time_started was stored as 0 and duration never read; real Clock-based window logic added, plus the missing target-not-met check on refund - named error enum replaces bare ProgramError::Custom values - tests rewritten to drive the real initialize -> contribute -> refund / check_contributions flows through the generated client with clock warping, including decoy-vault and cross-contributor attack regressions https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…rphaned test suite - deposit/withdraw accepted arbitrary mints despite the Strategy storing usdc_mint/asset_mint_a/asset_mint_b: junk mints with empty vaults understated NAV and minted inflated shares. has_one on all three mints in both structs, and on usdc_mint in invest/rebalance - fee_bps now capped at MAX_FEE_BPS at init per the config-validation rule - strategy.swap_router was stored but never checked; invest/rebalance now require the passed router program to match it - the test file sat at anchor/tests/ where no cargo package compiles it, so cargo test ran nothing; moved into programs/vault-strategy/tests/, fixed three latent bugs that had never compiled, and added negative tests for wrong mints, excessive fee, and unregistered router - boxed large accounts in constraint structs to clear build-sbf stack overflow warnings https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…ing authority check - the Ethereum-signature authorization signed an arbitrary client-supplied 32-byte message committing to nothing: one valid signature over anything authorized unlimited transfers of any amount to any destination, forever. The message is now reconstructed onchain as keccak256(program id || user account || amount || recipient || stored nonce), verified against the stored Ethereum address, and the nonce increments on success so each signature executes exactly once - quasar variant never compared the transaction authority to user_account.authority; the Solana-side authority check now accompanies the signature check in both variants - anchor variant hashed pubkey.to_bytes()[1..] (63 bytes) but Secp256k1Pubkey::to_bytes() is already the prefix-less 64-byte key, so it derived a wrong Ethereum address; now hashes all 64 bytes - migrated to token_interface + transfer_checked; named error enum in the quasar variant; *AccountConstraints struct names - transfer_tokens previously had zero test coverage; both variants now test valid transfer + nonce increment, replay rejection, wrong-amount, wrong-recipient, and wrong-authority failures with a fixed secp256k1 key - new root README documenting the signed-message format and nonce semantics https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…straints names, discriminator-based space - replace .checked_add(1).unwrap() with .ok_or(MathOverflow)? and a named error enum in all four variants (anchor + quasar of each example) - PageVisits::increment lived as a method on the account struct; the logic now sits in the instruction handler - space = 8 + INIT_SPACE replaced with DISCRIMINATOR.len() + INIT_SPACE (the one project in the repo still using the magic 8) - account-constraint structs renamed with the AccountConstraints suffix https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…floors, named errors - the deposit ratio clamp branched on reserve sizes instead of which user amount is binding, so it could scale a side UP past the user's stated amount (and past the balance check). Replaced with the try-A-then-B pattern used by the anchor variant; amounts now only ever scale down - all u128-to-u64 narrowings of money math use try_from with a named error instead of truncating 'as u64' casts - deposit gained minimum_lp_tokens_out, withdraw gained minimum_token_a_out and minimum_token_b_out, enforced before any CPI; swap's existing floor now reverts with the named SlippageExceeded - new AmmError enum (codes from 6000, matching the anchor variant) replaces ProgramError::Custom(4)/(5)/(6) and generic error reuse; claim_admin_fees gained the NothingToClaim guard for parity - tests assert exact clamp-down amounts on both sides, slippage rejections with untouched balances, and conservation in both swap directions against exact constant-product quotes https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…offer, bind quasar accounts Native: - new cancel_offer instruction (maker-only): abandoned offers no longer lock the maker's tokens forever - take_offer sent vault rent to the taker and offer lamports to an unvalidated 'payer' account; both now close to the maker, moving all lamports with checked arithmetic - take_offer lazily created the MAKER's ATA with the TAKER paying rent; make_offer now creates it maker-paid, take_offer requires it to exist - conservation check compared the maker's token B balance against the TAKER's token A pre-balance (wrong variable) and used panicking assert_eq!; fixed variable, named errors instead of panics - new README Anchor: - take_offer closed the vault to the taker; now closes to the maker - README claimed 'pnpm test' with no package.json and carried a history section about removed TypeScript artifacts; now describes cargo test - touched account structs renamed *AccountConstraints Quasar: - offer and vault both closed to the taker; now close to the maker - offer's token_mint_a/token_mint_b/vault were never validated against offer state: has_one bindings added, vault recorded in state - seeds aligned with the anchor variant ([b"offer", maker, id]); the undocumented one-offer-per-maker divergence is gone - tests assert rent recovery, wrong-mint and wrong-vault rejection https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
… real quasar tests - the vault PDA signed Bubblegum transfers for any caller with an arbitrary new_leaf_owner: anyone could withdraw any cNFT to any address. Both variants now have an initialize_vault instruction that stores an authority on the Vault account, and both withdraw handlers require that authority as a signer (has_one) before the CPI. The README 'Limitations' admission is gone because the limitation is gone - anchor withdraw_two_cnfts passed the client-supplied proof_1_length to split_at unchecked (panic on adversarial input); both variants now require proof_1_length + proof_2_length == remaining accounts, with a named ProofLengthMismatch error, and the quasar variant's silent .min() clamp and ignored proof_2_length are fixed - quasar variant's placeholder tests replaced with a full QuasarSVM port of the LiteSVM harness loading the same mainnet-dumped Bubblegum / Account Compression fixtures: 6 tests covering authority enforcement, replay, two-tree withdraw, and both proof-length failure modes - structs renamed *AccountConstraints; named error enums in both variants https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…e, runtime hashing
cutils (anchor):
- mint/verify were implemented as methods on the account structs with
empty validate() stubs wired through #[access_control]; they are now
plain handle_* functions, structs renamed *AccountConstraints
- verify computed sha256("global:verify_leaf") at runtime per call; the
discriminator is a precomputed constant with its derivation documented,
and the sha2 program dependency is gone
- dead Data state struct removed (both variants)
- '/// CHECK: unsafe' comments replaced with truthful descriptions
cnft-burn (anchor):
- burn_cnft handler and accounts struct moved from lib.rs into
instructions/; struct renamed BurnCnftAccountConstraints
- the merkle_tree '/// CHECK' comment claimed the account is never
written; the burn CPI rewrites the leaf and root - comment now true
- raw 32-byte program ID arrays replaced with pubkey!() constants
https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…ling, ID alignment) Checkpoint of agent work still being verified: shank-and-codama and close-account signer/owner checks, token decimal-scaling fixes, quasar program-ID alignment, and basics account-struct renames. Final verified states land in the following commits. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
… no longer brick at 32 lifetime bets - Bet accounts were never closed (claim paths only set a claimed flag) and User.bets was append-only, so 32 lifetime bets bricked a wallet permanently. Bets now close (rent to the bettor) at every resolution site, each close removes the index entry, and MAX_BETS_PER_USER caps concurrent open positions instead of lifetime bets - new close_losing_bet instruction: losers could never call the claim paths, so losing bets would still have accumulated forever - the claimed flag and AlreadyClaimed error are gone; account closure is what prevents double claims - payout and stake math converted to checked arithmetic; touched structs renamed *AccountConstraints - tests assert exact User.bets contents through each lifecycle, prove the 33rd concurrent bet fails, and prove a freed slot accepts a new bet https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…cks, full-lamport closes, oracle validation shank-and-codama/native: - pick_up_car/return_car never checked payer.is_signer while deriving the rental PDA from payer.key: anyone could flip anyone's rental status. Signer, owner, and canonical-PDA checks added with named errors - status transitions enforced (Created -> PickedUp -> Returned); assert! account validation replaced with proper errors; IDL and generated client updated; attack-path TS tests added close-account: - native and pinocchio closed accounts by moving lamports minus minimum_balance(0), stranding ~890k lamports at an unsignable PDA, with no signer check and no PDA-ownership check: anyone could close anyone's account. All lamports now move, payer must sign, and the target must be the payer's own User PDA - pinocchio additionally trusted a client-supplied bump and panicked on short instruction data; bump verified, data bounds-checked - quasar twin had the same unbound-account hole in close_user; PDA-bound - native TS suite migrated from bankrun (cannot execute current platform-tools binaries, pre-existing) to LiteSVM transfer-sol: - quasar's doc comment claimed an owner constraint that did not exist; the real constraint is now enforced, and all three variants use checked lamport arithmetic with named errors and rejection tests pyth: - quasar accepted any account with plausible bytes as a price update; owner now checked against the Pyth Receiver program - both variants gained a 60-second freshness check on publish_time (the only freshness signal the Pyth message carries; commented per the oracle rules) with boundary tests in both suites https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
Snapshot so agent work survives container recycling; verified states land in the following commits. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
… AccountConstraints renames in counter, hello-solana, transfer-sol https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
Snapshot so agent work survives container recycling; verified states land in the following commits. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
Snapshot so agent work survives container recycling; verified states land in the following commits. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
Snapshot so agent work survives container recycling; verified states land in the following commits. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
…3 anchor and quasar variants Every #[derive(Accounts)] struct now carries the AccountConstraints suffix; handler names, generated instruction-data names, and program IDs unchanged. Each project rebuilt and its tests run green after the rename. https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
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
2 participants
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.
Fixes from a full audit of the examples against the solana-claude-skill rules plus general Solana correctness. Every changed project was rebuilt and its test suite run green; security fixes come with new regression tests that exercise the branch the bug lived in. Companion skill PR: quicknode/solana-claude-skill#24.
Critical security fixes
outputof token B from the trader instead ofinput_amount, so traders underpaid on every B-to-A swap. Every existing test usedinput_is_token_a: true; new B-to-A conservation and invariant tests now cover both directions. The quasar variant's deposit ratio clamp branched on reserve sizes and could scale a deposit UP past the user's stated amount; replaced with the try-A-then-B pattern, plus slippage floors on deposit/withdraw and a named error enum replacingProgramError::Custom(4)-style codes.duration: 0. Minimum amounts used1u64.pow(decimals)(always 1) and3.pow(decimals)instead of major-unit scaling. The quasar variant additionally let anyone drain the vault by passing a victim's Contributor record with their own token account (no seeds binding), never created Contributor accounts onchain at all, and storedtime_started: 0. Both variants rewritten with bound accounts, real clock logic, checked arithmetic, state-before-CPI,transfer_checked, vault closure on success, and clock-warping tests on both sides of every deadline.transfer_tokenshandler now has replay, wrong-amount, wrong-recipient, and wrong-authority tests in both variants.has_oneadded everywhere,fee_bpscapped, and the stored-but-never-checkedswap_routeris now enforced. The whole test suite turned out to be orphaned (sitting where no cargo target compiles it, socargo testran nothing); it now lives inprograms/vault-strategy/tests/, with three latent bugs fixed and negative tests added.payeraccount, lazily created the maker's ATA at the taker's expense, and asserted conservation against the wrong variable; it also had no cancel instruction, locking abandoned offers forever. The anchor and quasar variants refunded the maker's rent to the taker on close. All fixed; quasar offer accounts are now mint/vault-bound and use the same[b"offer", maker, id]seeds as the anchor variant.proof_1_length; both variants now validate proof lengths with a named error. The quasar placeholder tests are replaced with a real QuasarSVM suite loading the mainnet-dumped fixture programs.pick_up_car/return_carnever checkedpayer.is_signer, so anyone could flip anyone's rental status; signer, owner, canonical-PDA, and status-transition checks added,assert!validation replaced with named errors, attack tests added.close_losing_bet), freeing index slots; the cap applies to concurrent positions.Correctness and structure
amount * 10u64.pow(decimals)) removed from transfer-tokens, token-minter, and pda-mint-authority handlers; amounts are minor units, scaling happens in tests/clients. create-token and pda-mint-authority quasar variants no longer accept-and-ignore theirdecimalsparameter.validate/actuate) pattern, deadDatastate structs, runtime SHA-256 of a constant discriminator, and a false/// CHECKcomment claiming the merkle tree is never written..checked_add(1).unwrap()replaced with named errors; the lastspace = 8 + ...magic number replaced withDISCRIMINATOR.len().Standards sweep (repo-wide, every project rebuilt and tested)
#[derive(Accounts)]struct in the repo now ends inAccountConstraints(was 16 of ~150).2222...placeholders or mismatches).[registry] section removedcomment stamped into 49 Anchor.toml files, and removal-history comments are gone.migrations/deploy.tsin 25 anchor projects, escrow's mocha configs, a ts-mocha script pointing at a nonexistent file.Truthful documentation
test = "cargo test"; it now documents the real Rust LiteSVM / QuasarSVM flow.pnpm testwith no package.json; missing READMEs added for escrow/native, betting-market, nft-operations, external-delegate-token-master; root README gained the Tools section.Audit corrections
Two findings from the original audit turned out wrong and were handled differently: the native/pinocchio mocha test scaffolding is live in CI (
pnpm build-and-test), not stale, so it stays (a follow-up could migrate it to LiteSVM TS, as was done for close-account where bankrun could not execute current platform-tools binaries); and theAccountConstraintscount was stale (token-swap and order-book already complied).Remaining follow-ups (not in this PR)
https://claude.ai/code/session_01VPj6WLMxD5KL6NwvUvuz1K
Generated by Claude Code