security: fix broken take_offer invariant + add missing RefundOffer instruction - #668
Open
NikkiAung wants to merge 3 commits into
Open
security: fix broken take_offer invariant + add missing RefundOffer instruction#668NikkiAung wants to merge 3 commits into
NikkiAung wants to merge 3 commits into
Conversation
…nstruction Escalated the same security-audit methodology from PR solana-foundation#667 (missing signer checks in basics/) to tokens/escrow - native, pinocchio, and anchor. Escrow/vault programs are the highest-value target for this kind of audit: they hold other people's funds in program custody, and this repo's version is what learners copy into real token-swap escrows. Bug (native only): take_offer's post-transfer invariant check compared the maker's token-B balance against the wrong variable - `taker_amount_a_before_transfer` instead of `maker_amount_b_before_transfer` (a copy-paste of the line above, half-edited). The real SPL Token CPI always moves the correct amount, so the assert only happened to pass when the maker's token-B account balance was exactly 0 before the trade. That's not guaranteed, and is trivially breakable by a third party: anyone can create the maker's token-B ATA (permissionless) and send it 1 base unit (also permissionless, no signature required from the recipient) before a take_offer call. From that point on, every take_offer for that offer panics and reverts. Gap (all three implementations): there was no way for a maker to reclaim a deposited offer - not in native, not in pinocchio, not even in the anchor reference version. Only MakeOffer/TakeOffer existed. This is what turns the assert bug from "annoying" into "permanent fund loss": once an offer is bricked (or its counterparty simply never shows up, the ordinary non-adversarial case), the maker's tokens were locked in the vault with no recovery path. Fixes: - One-line fix to the comparison in native's take_offer.rs. - New RefundOffer instruction in all three implementations: only the maker (verified against the offer's stored `maker` field) can call it. Returns the vault's current token-A balance to the maker's own account, closes the vault and offer accounts, rent to the maker. Structurally mirrors each implementation's existing take_offer vault-drain-and-close logic (same seeds, same CPI shape), just redirecting the destination and dropping the token-B leg. Every new test was verified to fail before its corresponding fix (confirming it actually exercises the bug, not a false negative) and pass after: - native: regression test pre-funds the maker's token-B account before Take Offer to reproduce the exact condition that broke the old assert; RefundOffer happy-path and non-maker-cannot-refund tests. - pinocchio: same two RefundOffer tests (never had the assert bug). - anchor: same two RefundOffer tests added to litesvm.test.ts. Also extended native's and pinocchio's test `createValues()` helper to actually respect all the overridable defaults its own type signature (TestValuesDefaults) already promised - it previously only read `programId`/`id` from the passed defaults and silently regenerated maker/taker/mint keypairs regardless, which made it impossible to create a second offer against the same already-funded accounts and already-deployed program.
Contributor
Greptile SummaryThe PR adds maker-authorized offer refunds across all three escrow implementations and corrects the native take-offer balance invariant. It also incorporates the requested security and test-helper fixes from the previous review.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported refund validation, revert assertion, mint override, and substitute-vault issues are fixed in the current code. Important Files Changed
Reviews (3): Last reviewed commit: "fix(#668): address Greptile review findi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+66
to
+76
| &token_instruction::transfer( | ||
| token_program.key, | ||
| vault.key, | ||
| maker_token_account_a.key, | ||
| offer_info.key, | ||
| &[offer_info.key], | ||
| vault_amount_a, | ||
| )?, | ||
| &[vault.clone(), maker_token_account_a.clone(), offer_info.clone(), token_program.clone()], | ||
| &[offer_signer_seeds], | ||
| )?; |
Contributor
There was a problem hiding this comment.
When the maker supplies another executable account as token_program, both CPIs can report success without transferring or closing the vault, after which the handler destroys the offer account and leaves its deposited tokens without a recovery path.
How this was verified: The caller-supplied program key is used for both CPIs, and the offer is then closed without independently checking that the vault was drained.
Knowledge Base Used: Tokens Directory Overview
Comment on lines
+28
to
+35
| export const expectRevert = async (promise: Promise<unknown>) => { | ||
| try { | ||
| await promise; | ||
| throw new Error('Expected a revert'); | ||
| } catch { | ||
| return; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
When the supplied promise resolves, expectRevert throws Expected a revert inside the try and immediately catches that same error, so the new non-maker test passes even when the refund unexpectedly succeeds.
Suggested change
| export const expectRevert = async (promise: Promise<unknown>) => { | |
| try { | |
| await promise; | |
| throw new Error('Expected a revert'); | |
| } catch { | |
| return; | |
| } | |
| }; | |
| export const expectRevert = async (promise: Promise<unknown>) => { | |
| let reverted = false; | |
| try { | |
| await promise; | |
| } catch { | |
| reverted = true; | |
| } | |
| if (!reverted) { | |
| throw new Error('Expected a revert'); | |
| } | |
| }; |
Knowledge Base Used: Tokens Directory Overview
Comment on lines
+174
to
+181
| // Making sure tokens are in the right order | ||
| const mintAKeypair = await generateKeyPairSigner(); | ||
| let mintBKeypair = await generateKeyPairSigner(); | ||
| while (isLessThan(addressEncoder.encode(mintBKeypair.address), addressEncoder.encode(mintAKeypair.address))) { | ||
| mintBKeypair = await generateKeyPairSigner(); | ||
| let mintAKeypair = defaults?.mintAKeypair; | ||
| let mintBKeypair = defaults?.mintBKeypair; | ||
| if (!mintAKeypair || !mintBKeypair) { | ||
| mintAKeypair = mintAKeypair ?? (await generateKeyPairSigner()); | ||
| mintBKeypair = mintBKeypair ?? (await generateKeyPairSigner()); | ||
| while (isLessThan(addressEncoder.encode(mintBKeypair.address), addressEncoder.encode(mintAKeypair.address))) { | ||
| mintBKeypair = await generateKeyPairSigner(); |
Contributor
There was a problem hiding this comment.
When only mintBKeypair is supplied and the generated mint A sorts after it, the ordering loop replaces the caller-provided mint B. Tests requesting reuse of that mint then derive accounts and offers for a different mint, which can exercise the wrong setup; the Pinocchio helper has the same behavior.
…ests Two separate tsc --noEmit errors, both only caught by CI's typecheck step (tsx strips types without checking, so the local mocha run passed regardless): - native: svm.getAccount() returns a union type where .data only exists on the exists:true variant. Four new call sites decoded .data without narrowing first - added the same assert(x.exists, ...) guard the rest of this file already uses before every decode. - anchor: the non-maker-refund test intentionally passes mismatched accounts (that's what it's proving the program rejects), which trips TypeScript's excess-property check against the IDL-derived account type. Split it into a separately-typed Record<string, PublicKey> and pass it through an explicit `as any`, since strict typing doesn't apply to a deliberately-invalid input.
| return Err(ProgramError::InvalidSeeds); | ||
| } | ||
|
|
||
| let vault_amount = TokenAccount::from_account_view(vault)?.amount(); |
Contributor
There was a problem hiding this comment.
When the maker signs a refund with another token-A account owned by the offer PDA, the handler drains and closes that substitute account and then destroys the offer. Because it never verifies that vault is the offer's canonical ATA, the genuine funded vault remains inaccessible.
Knowledge Base Used: Tokens Directory Overview
Four real issues, all verified before/after: 1. native/pinocchio refund_offer.rs (P1, security): neither checked that `vault` is actually the offer's canonical ATA. A substitute token-A account with owner = the offer PDA (creatable by anyone, no cooperation needed from the PDA - owner is just a stored field, never requires the owner to sign at account-creation time) could be passed instead, draining that decoy and then closing the real offer anyway - permanently stranding the genuine vault's funds. Added the same assert_is_associated_token_account check make_offer already does at vault creation time. New adversarial test (native) creates exactly such a decoy and confirms refund now rejects it - verified it fails without the fix, passes with it. 2. native refund_offer.rs (P1, security): the caller-supplied token_program was used directly as the CPI target with no check that it's the real SPL Token program. A substituted fake program could report success on both the transfer and close without moving anything, after which the offer gets destroyed anyway. Added spl_token_interface::check_program_account(), the same function the SPL crate's own instruction builders call internally - now enforced explicitly up front instead of implicitly deep in the CPI. (Pinocchio's Transfer/CloseAccount builders already hardcode the real program ID rather than trusting the passed account, so this class of bug doesn't apply there.) 3. pinocchio/tests/utils.ts expectRevert (P2): the re-throw meant to signal "the operation unexpectedly succeeded" happened inside the same try block as the operation itself, so its own catch swallowed it - the helper returned normally regardless of whether the promise resolved or rejected. This made "Refund Offer rejects a non-maker signer" pass vacuously (verified: it kept passing even before this fix, silently proving nothing). Restructured per Greptile's suggestion so success is tracked in a flag checked after the try/catch, outside where the operation's own rejection is caught. The non-maker test's underlying security check was never actually broken - just its ability to prove that. 4. native/pinocchio tests/utils.ts createValues (P2): my earlier fix for making defaults overridable had a bug of its own - if the caller supplied only one of mintAKeypair/mintBKeypair and the ordering check needed a redo, it could regenerate and discard the one the caller DID supply. Rewrote so only the mint not present in the original defaults is ever regenerated; if both were supplied and are genuinely out of order, throws instead of silently picking one to discard.
Contributor
Author
|
Thanks for the thorough review — all 4 findings were real and fixed in
|
Contributor
Author
|
Hey @dev-jodee — bumping this for review when you get a chance. CI is green and Greptile came back clean (5/5, no blocking findings). Thanks! |
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
1 participant
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
Escalated the same security-audit methodology from #667 (missing signer checks in
basics/) totokens/escrow— native, pinocchio, and anchor. Escrow/vault programs are the highest-value target for this kind of audit: they hold other people's funds in program custody, and this repo's version is what learners copy into real token-swap escrows.Bug (native only): broken post-transfer invariant lets a third party permanently freeze any offer
take_offer's post-transfer sanity check compared the maker's token-B balance against the wrong variable:taker_amount_a_before_transferis a copy-paste of the line above, only half-edited — it should bemaker_amount_b_before_transfer. The real SPL Token CPI always moves the correct amount, so this assert only happened to pass when the maker's token-B balance was exactly 0 before the trade. That's not guaranteed, and is trivially breakable by any third party, no cooperation needed: create the maker's token-B ATA (permissionless) and send it 1 base unit (also permissionless — no signature required from the recipient) before atake_offercall. From that point on, everytake_offerfor that offer panics and reverts.Gap (all three implementations): no way for a maker to reclaim their deposit
This is what turns the bug above from "annoying" into permanent fund loss: there was no
RefundOffer/cancel instruction anywhere — not in native, not in pinocchio, not even in the anchor reference version. OnlyMakeOffer/TakeOfferexisted. Once an offer is bricked (by the bug above, or simply because the counterparty never shows up — the ordinary non-adversarial case), the maker's deposited tokens were locked in the vault with no recovery path.Fixes
native/program/src/instructions/take_offer.rs.RefundOfferinstruction in all three implementations: only the maker (verified against the offer's storedmakerfield) can call it. Returns the vault's current token-A balance to the maker's own account, closes the vault and offer accounts, rent to the maker. Structurally mirrors each implementation's existingtake_offervault-drain-and-close logic — same seeds, same CPI shape — just redirecting the destination and dropping the token-B leg entirely (a refund never touches token B).Test plan
Every new test was verified to fail against the unpatched/pre-feature code first (confirming it actually exercises the bug/gap, not a false negative), then pass after the fix — same discipline as #667:
Take Offerto reproduce the exact condition that broke the old assert (confirmed panic attake_offer.rs:171before the fix);RefundOfferhappy-path and non-maker-cannot-refund tests. 6/6 passing.RefundOffertests (never had the assert bug — itstake_offerhas no equivalent check). 4/4 passing.RefundOffertests added tolitesvm.test.ts. 4/4 passing. (The separate validator-basedescrow.test.ts— pre-existing, untouched — couldn't be executed in this sandbox due to a localsolana-test-validatorstartup issue unrelated to this change; the identical on-chain logic is already fully exercised vialitesvm.test.tsagainst the same compiled program.)cargo fmt --checkandcargo clippy -- -D warningsclean for the root-workspace crates (native + pinocchio);prettier --checkclean for all touched TS files. (tokens/escrow/anchoris its own nested Cargo workspace, outside the root workspacecargo fmt/clippycover — consistent with the rest of the repo's*/anchor/programs.)#[allow(clippy::enum_variant_names)]to native'sEscrowInstructionenum — adding the thirdRefundOffervariant tripped the lint (it only fires at ≥3 variants), and the sharedOfferpostfix is intentional domain vocabulary matching the existingMakeOffer/TakeOffernaming, not something to rename.Incidental fix
Extended native's and pinocchio's test
createValues()helper to actually respect all the overridable defaults its own type signature (TestValuesDefaults) already promised — it previously only readprogramId/idfrom the passed defaults and silently regeneratedmaker/taker/mint keypairs regardless, which made it impossible to create a second offer reusing the same already-funded accounts and already-deployed program (needed for the new tests, which create additional offers under the same setup).