Skip to content

Fix code inconsistencies across various programs#12

Merged
mikemaccana merged 8 commits into
quicknode:mainfrom
mikemaccana-edwardbot:fix-code-inconsistencies-2026-04-22
May 11, 2026
Merged

Fix code inconsistencies across various programs#12
mikemaccana merged 8 commits into
quicknode:mainfrom
mikemaccana-edwardbot:fix-code-inconsistencies-2026-04-22

Conversation

@mikemaccana
Copy link
Copy Markdown
Collaborator

@mikemaccana mikemaccana commented May 11, 2026

Fixes the 11 code inconsistencies flagged in the 2026-04-22 audit across various program examples. One commit per logically-grouped fix.

Fixes

  1. tokens/token-swap - TutorialError::InvalidMint was defined but never emitted (Anchor has_one already handles the mint-mismatch check upstream). Remove the dead variant. (66d2dee)
  2. tokens/token-swap - migrate token::transfer to token::transfer_checked across deposit_liquidity, swap_exact_tokens_for_tokens, and withdraw_liquidity so the mint and decimals are validated on every transfer. (66d2dee)
  3. tokens/token-swap - silent clamp-to-balance broke slippage protection for callers built on top. Return a new InsufficientBalance error when the requested amount exceeds the available balance. (66d2dee)
  4. basics/counter (native + pinocchio) - unknown / empty instruction bytes used to fall through to Ok(()). Both variants now return ProgramError::InvalidInstructionData. (2cf93df)
  5. basics/counter/native - the program accepted any 8-byte account as the counter. Add an owner check (account.owner == program_id) that returns ProgramError::IncorrectProgramId on mismatch. (2cf93df)
  6. compression/cnft-burn, compression/cnft-vault - bubblegum_program was an UncheckedAccount with no program id check. Pin it via #[account(address = MPL_BUBBLEGUM_ID)] (the constant already lives in the crate). (f90ac8e)
  7. tokens/escrow/anchor - no way to cancel an offer, abandoned offers locked maker funds forever. Add a cancel_offer instruction: maker-signed, returns vault tokens to the maker, closes the vault, closes the offer account (rent refunded to maker via close = maker). (b0da596)
  8. tokens/escrow/anchor - init_if_needed on maker_token_account_b on the take side meant the taker paid the maker's rent. Move the init to make_offer with payer = maker, treat as plain mut on take_offer. (b0da596)
  9. tokens/token-extensions/default-account-state/native - update_default_account_state was signed by payer, but the freeze authority recorded on the mint is mint_authority. The CPI only worked by coincidence when the caller used the same account for both. Sign with mint_authority to make the requirement explicit. (6a1c095)
  10. basics/repository-layout/anchor - error.rs was empty and every refusal branch in the carnival handlers logged a message and returned Ok(()). Tests batched valid + invalid attempts through .unwrap() and "passed" because rejections silently succeeded. Add a CarnivalError enum (NotEnoughTickets, RiderTooShort), return real errors from the refusal branches, and split the tests into one-success-or-one-rejection cases that actually assert is_err() on rejections. (d2f0320)
  11. tokens/token-extensions/nft-meta-data-pointer/anchor-example - the unaudited session-keys crate is used without any warning. Add a comment block above declare_id! explicitly flagging it as educational only and pointing at what to harden before any production use. (88f0380)

Verification

For every program touched I ran the local LiteSVM / node:test suites and confirmed they pass before pushing:

  • tokens/token-swap - cargo test: 4 passed.
  • basics/counter/native - pnpm build-and-test: 3 passed (existing 2 + new test_unknown_instruction_fails and test_wrong_owner_fails).
  • basics/counter/pinocchio - pnpm build-and-test: 2 passed (existing + new test_unknown_instruction_fails).
  • compression/cnft-burn, compression/cnft-vault - cargo check (no Rust test suite in repo).
  • tokens/escrow/anchor - cargo test: 4 passed (test_make_offer, test_take_offer, new test_cancel_offer, new test_cancel_offer_rejects_non_maker).
  • tokens/token-extensions/default-account-state/native - pnpm build-and-test: 1 passed (existing TS suite still green).
  • basics/repository-layout/anchor - cargo test: 8 passed (success + rejection cases per instruction).
  • tokens/token-extensions/nft-meta-data-pointer/anchor-example - comment-only; pre-existing build failure of the session-keys crate is unchanged (not introduced by this PR).

Note

Medium Risk
Medium risk: changes touch on-chain token/CPI flows (escrow, token-swap, cNFT CPI) and alter failure semantics, which can impact client expectations and fund movement if mis-specified.

Overview
Improves correctness and safety of several Solana program examples by turning previously silent/ambiguous behaviors into explicit failures and tightening CPI/account validation.

In basics/counter (native + pinocchio), empty/unknown instruction data now returns InvalidInstructionData, native increment additionally enforces counter_account.owner == program_id, and tests now assert success/failure cases (including unknown discriminants and wrong-owner accounts).

In the Anchor carnival example, introduces CarnivalError and changes ride/game/food “refusal” branches to return real errors instead of Ok(()), with tests split to explicitly expect either success or rejection.

Hardens compressed NFT CPI examples by constraining bubblegum_program to MPL_BUBBLEGUM_ID in cnft-burn and cnft-vault withdraw instructions.

Extends the Anchor escrow example with a maker-signed cancel_offer that returns vault funds and closes vault/offer, and shifts initialization of the maker’s token-B ATA to make_offer (so the maker—not the taker—pays rent); tests updated accordingly.

In token examples, fixes default-account-state to sign update_default_account_state with mint_authority, adds a warning comment about unaudited session-keys usage, and updates token-swap to remove dead InvalidMint, fail fast with InsufficientBalance (no balance clamping), and use transfer_checked for mint/decimal validation in deposit/swap/withdraw.

Reviewed by Cursor Bugbot for commit 26e37b0. Bugbot is set up for automated code reviews on this repo. Configure here.

…cient balance, remove unused InvalidMint

Three related issues in the token-swap program:

- Migrate token::transfer to token::transfer_checked. transfer_checked passes
  the mint and decimals through the CPI, which guards callers from
  decimal-mismatch bugs and is the modern recommended path.
- In deposit_liquidity and swap_exact_tokens_for_tokens, fail with the new
  TutorialError::InsufficientBalance instead of silently clamping the requested
  amount to the user's available balance. The silent clamp broke slippage
  protection for callers building on top of this program: they expected their
  input amount to be the amount actually used, and their min_output_amount was
  computed against that input. Clamping let trades succeed on worse terms than
  the caller asked for.
- Remove the unused TutorialError::InvalidMint variant. Mint mismatches are
  already handled by Anchor's has_one = mint_a / has_one = mint_b constraints
  on the pool, so emitting a custom error from program code would never trigger.
  Dead error variants are confusing for readers.
…ative)

Issues 4 and 5 from the 2026-04-22 audit:

- Unknown instruction discriminants previously logged a message and returned
  Ok(()) in both basics/counter/native and basics/counter/pinocchio. Return
  ProgramError::InvalidInstructionData instead so callers actually see a
  failure. Also reject empty instruction data explicitly rather than panicking
  via split_at(1).
- basics/counter/native had no owner check on the counter account, so the
  program would happily decode any 8 bytes of data as a Counter even when the
  account belonged to a different program. Add an owner check
  (counter_account.owner == program_id) and return IncorrectProgramId on
  mismatch.

Tests:

- Add test_unknown_instruction_fails for both native and pinocchio variants.
- Add test_wrong_owner_fails for native.
- Tighten the existing happy-path test to actually assert the transaction
  succeeded (it previously discarded the result via `let _ = ...is_ok();`).
- Refactor program_bytes into a module-level PROGRAM_SO const to avoid
  repeating the include_bytes! path.
…legum id

The bubblegum_program account in cnft-burn::BurnCnft and in cnft-vault's
Withdraw / WithdrawTwo was declared as UncheckedAccount with no constraint on
its address. Each program already defines a MPL_BUBBLEGUM_ID constant and the
account is forwarded into a CPI, so a caller could substitute a different
account and steer the cross-program invocation somewhere unexpected.

Add #[account(address = MPL_BUBBLEGUM_ID)] to the relevant fields. The mpl-
bubblegum crate is not in scope as a dependency in this workspace, so the
address constraint with the existing constant is the lowest-touch fix - it
preserves the UncheckedAccount shape used by the rest of the example.
…t_b rent to maker

Two related issues in the escrow anchor program:

- Add a cancel_offer instruction (issue 7). Without it, an abandoned offer
  would leave the maker's token-A locked in the vault forever and the offer
  account's rent unrecoverable. cancel_offer requires the maker's signature,
  returns vault tokens to the maker, closes the vault, and closes the offer
  account (rent refunded to the maker via close = maker).
- Shift the rent burden for the maker's token-B ATA from the taker to the
  maker (issue 8). It used to be init_if_needed on the take_offer side with
  payer = taker, meaning the taker paid the maker's rent. Move the
  init_if_needed onto make_offer with payer = maker, and treat the account as
  an existing (mut) ATA on take_offer. The party who chose to open the offer
  now pays its rent.

Tests:

- Update existing make_offer and take_offer tests to pass the new
  maker_token_account_b on the make side.
- Add test_cancel_offer (maker cancels and gets tokens + rent back).
- Add test_cancel_offer_rejects_non_maker (the has_one = maker + seed-based
  PDA constraints reject a non-maker signer).
…e_default_account_state with mint_authority

The update_default_account_state CPI was signed by `payer` but the freeze
authority on the mint is `mint_authority` (set as Some(mint_authority.key) in
initialize_mint just above). The CPI only succeeded when the caller happened
to pass the same pubkey for both the payer and the mint authority slots.
Anyone passing a separate payer would have been silently rejected.

Fix by signing the update with mint_authority (the actual freeze authority
recorded on the mint). The requirement that the freeze authority sign is now
explicit in the code rather than implicit through identical pubkeys.
…ervice

The carnival example used to log a 'sorry' message and return Ok(()) for every
refusal branch (not enough tickets, rider too short), and error.rs was empty.
Callers and tests could not tell a successful ride/game/snack from a refusal,
and the existing tests batched a list of mixed valid and invalid attempts
through .unwrap() - they were 'passing' precisely because rejections silently
succeeded.

Changes:

- Add a CarnivalError enum with NotEnoughTickets and RiderTooShort.
- Return CarnivalError variants from the three handlers on the refusal
  branches (eat_food, get_on_ride, play_game).
- Rewrite test_carnival.rs to test the success path and each rejection path
  separately, asserting is_err() on the rejection paths.
…ag unaudited session-keys crate near declare_id

This example depends on the third-party `session-keys` crate, which has not
been independently audited. Anyone reading the example landing on declare_id!
should be told that up front, so add an explicit warning comment right above
declare_id! explaining that this is for education only and pointing at what
to do before considering it for production.
@mikemaccana mikemaccana merged commit 68fa7ea into quicknode:main May 11, 2026
19 checks passed
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