Skip to content

fix(escrow): return closed offer and vault rent to the maker; verify system program, offer owner and canonical bump - #4

Open
SwineCoder101 wants to merge 2 commits into
mainfrom
fix/escrow-rent-to-maker
Open

fix(escrow): return closed offer and vault rent to the maker; verify system program, offer owner and canonical bump#4
SwineCoder101 wants to merge 2 commits into
mainfrom
fix/escrow-rent-to-maker

Conversation

@SwineCoder101

Copy link
Copy Markdown
Owner

Bug: take_offer pays the maker's rent to the taker (plus unchecked system program / non-canonical bump)

This is a bug, not a style change. In the native and pinocchio escrow variants, take_offer closes the vault and the offer account but sends their rent (~0.0037 SOL per offer) to the taker and to a payer account the taker picks, even though both accounts were funded by the maker in make_offer. Any taker skims that amount from every maker (medium). Two smaller checks were also missing: native closes the offer by assigning it to whatever account is passed in the system_program slot, letting a taker squat a maker's (maker, id) offer slot under their own program (low), and pinocchio's make_offer accepts any valid non-canonical bump, letting one maker hold several live offers under one (maker, id) (low). The anchor variant is correct (close = maker, vault CloseAccount destination is the maker, canonical bump enforced) and was used as the reference.

Affected

  • tokens/escrow/native/program/src/instructions/take_offer.rs
  • tokens/escrow/native/program/src/instructions/refund_offer.rs
  • tokens/escrow/native/tests/instruction.ts (client: maker is now writable in buildTakeOffer)
  • tokens/escrow/pinocchio/program/src/instructions/take_offer.rs
  • tokens/escrow/pinocchio/program/src/instructions/refund_offer.rs
  • tokens/escrow/pinocchio/program/src/instructions/make_offer.rs
  • tokens/escrow/pinocchio/tests/instruction.ts (client: maker is now writable in buildTakeOffer)

Functionality

make_offer creates the offer PDA [b"offer", maker, id] and its vault ATA, both paid for by the maker, and moves the maker's token A into the vault. take_offer sends the taker's token B to the maker, releases the vaulted token A to the taker, and closes the vault and the offer account. refund_offer returns the vaulted token A to the maker and closes both accounts. Whoever funded the closed accounts (the maker) should get their rent back, and a closed offer must be handed back to the system program so the maker can reuse the (maker, id) slot.

The bug

  1. Rent to the wrong party (native take_offer.rs:188-204 before the fix, pinocchio take_offer.rs:136-148): the vault CloseAccount destination was taker, and the offer account's lamports were credited to payer - the take instruction's own payer, which the taker chooses. The pinocchio comment "returning its rent to the payer that funded it" was wrong: that payer is take_offer's, not make_offer's. Net effect: every take transfers the maker's offer + vault rent (1,677,360 + 2,039,280 = 3,716,640 lamports on a default litesvm/mainnet rent schedule) to the taker.

  2. Unverified system program (native take_offer.rs:212 and refund_offer.rs:114 before the fix): offer_info.assign(system_program.key) uses the caller-supplied account key without checking it, and neither instruction checked offer_info.owner == program_id. A taker can pass their own program in the system_program slot (both receiving ATAs already exist, so nothing else in the instruction touches the account) and, in the same transaction, top the emptied account back up to rent exemption. The maker's offer PDA is then permanently owned by the taker's program, so the maker can never create an offer with that id again. Pinocchio's close() hardcodes the system program owner, so only the owner check was missing there.

  3. Non-canonical bump (pinocchio make_offer.rs:45-57 before the fix): the client-supplied data[24] bump was checked with create_program_address, which accepts any bump that yields an off-curve address. Roughly half of all bumps do, so a maker can create several live offers under one (maker, id), which take_offer/refund_offer and the anchor/native variants treat as unique. Native uses find_program_address; anchor enforces the canonical bump.

Reproduce

cd tokens/escrow/native && pnpm install --frozen-lockfile && pnpm build-and-test
cd tokens/escrow/pinocchio && pnpm install --frozen-lockfile && pnpm build-and-test

Tests added (all fail against the unmodified programs):

  • native: Take Offer returns the vault and offer rent to the maker, not the taker, Take Offer rejects a bogus system program account, Refund Offer rejects a bogus system program account
  • pinocchio: Take Offer returns the vault and offer rent to the maker, not the taker, Make Offer rejects a non-canonical offer bump

Failing output before the fix (native; pinocchio's rent test fails identically):

  1) Escrow!
       Take Offer returns the vault and offer rent to the maker, not the taker:

      the offer and vault rent should be returned to the maker
      + expected - actual

      -3716640n
      +7433280n

  2) Escrow!
       Take Offer rejects a bogus system program account:
     AssertionError: expected a take with a bogus system program to fail

  3) Escrow!
       Refund Offer rejects a bogus system program account:
     AssertionError: expected a refund with a bogus system program to fail

  2) Escrow (Pinocchio)
       Make Offer rejects a non-canonical offer bump:
     AssertionError: expected a make with a non-canonical bump to fail

Fix

  • native + pinocchio take_offer: the vault CloseAccount destination and the offer account's lamports now go to maker, which the instruction already binds to offer.maker. maker must therefore be writable; both TS clients (buildTakeOffer) pass it as WRITABLE now, matching the anchor variant's #[account(mut)] maker.
  • native take_offer/refund_offer: system_program is checked against solana_system_interface::program::check_id before being used as the close target (IncorrectProgramId otherwise), and the offer account must be owned by this program (IllegalOwner otherwise).
  • pinocchio take_offer/refund_offer: added the same offer_account.owned_by(program_id) check.
  • pinocchio make_offer: the offer PDA is derived with find_program_address; the supplied account must match it and the supplied bump must equal the canonical bump (InvalidSeeds otherwise). The instruction data layout is unchanged.

Verification

cd tokens/escrow/native && pnpm build-and-test      # 11 passing
cd tokens/escrow/pinocchio && pnpm build-and-test   # 9 passing
cargo clippy -p escrow-native-program -p escrow-pinocchio-program -- -D warnings
  Escrow!
    ✔ Take Offer returns the vault and offer rent to the maker, not the taker
    ✔ Take Offer rejects a bogus system program account
    ✔ Refund Offer rejects a bogus system program account
  11 passing (56ms)

  Escrow (Pinocchio)
    ✔ Take Offer returns the vault and offer rent to the maker, not the taker
    ✔ Make Offer rejects a non-canonical offer bump
  9 passing (52ms)

… misses account checks

- native + pinocchio: Take Offer must return the closed vault's and offer
  account's rent to the maker who funded them, and the taker's balance
  must be unchanged
- native: take_offer/refund_offer with a substitute account in the
  system_program slot must be rejected, and the offer must remain
  program-owned
- pinocchio: make_offer with a valid but non-canonical bump for an
  existing (maker, id) must be rejected
…system program, offer owner and canonical bump

- native + pinocchio take_offer: the vault CloseAccount destination and
  the offer account's lamports now go to the maker who funded them in
  make_offer, not to the taker / the taker's chosen payer. maker is now
  writable in both TS clients, matching the anchor variant.
- native take_offer/refund_offer: verify the system_program account
  before assigning the closed offer to it, and require the offer account
  to be owned by this program.
- pinocchio take_offer/refund_offer: require the offer account to be
  owned by this program.
- pinocchio make_offer: derive the offer PDA with find_program_address
  and reject any bump other than the canonical one, so only one offer
  can exist per (maker, id).
@SwineCoder101
SwineCoder101 force-pushed the fix/escrow-rent-to-maker branch from 97cfb45 to 30ddb00 Compare August 27, 2026 12:23
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.

1 participant