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

tokens/escrow/native/program/src/instructions/refund_offer.rs

@@ -38,6 +38,19 @@ impl RefundOffer {
         //
         spl_token_interface::check_program_account(token_program.key)?;
 
+        // the offer is closed by assigning it to the system program below, so
+        // that account must really be the system program
+        //
+        if !solana_system_interface::program::check_id(system_program.key) {
+            return Err(ProgramError::IncorrectProgramId);
+        }
+
+        // only this program's own offer accounts may be refunded
+        //
+        if offer_info.owner != program_id {
+            return Err(ProgramError::IllegalOwner);
+        }
+
         // get the offer data
         //
         let offer = Offer::try_from_slice(&offer_info.data.borrow()[..])?;

tokens/escrow/native/program/src/instructions/take_offer.rs

@@ -44,6 +44,19 @@ impl TakeOffer {
             return Err(ProgramError::MissingRequiredSignature);
         }
 
+        // the offer is closed by assigning it to the system program below, so
+        // that account must really be the system program
+        //
+        if !solana_system_interface::program::check_id(system_program.key) {
+            return Err(ProgramError::IncorrectProgramId);
+        }
+
+        // only this program's own offer accounts may be taken
+        //
+        if offer_info.owner != program_id {
+            return Err(ProgramError::IllegalOwner);
+        }
+
         // get the offer data
         //
         let offer = Offer::try_from_slice(&offer_info.data.borrow()[..])?;
@@ -183,25 +196,25 @@ impl TakeOffer {
         solana_program::msg!("Maker B Balance After Transfer: {}", maker_amount_b);
         solana_program::msg!("Taker B Balance After Transfer: {}", taker_amount_b);
 
-        // close the vault account
+        // close the vault account, rent to the maker who funded it
         //
         invoke_signed(
             &spl_token_interface::instruction::close_account(
                 token_program.key,
                 vault.key,
-                taker.key,
+                maker.key,
                 offer_info.key,
                 &[],
             )?,
-            &[vault.clone(), taker.clone(), offer_info.clone()],
+            &[vault.clone(), maker.clone(), offer_info.clone()],
             &[offer_signer_seeds],
         )?;
 
-        // Send the rent back to the payer
+        // Send the rent back to the maker
         //
         let lamports = offer_info.lamports();
         **offer_info.lamports.borrow_mut() -= lamports;
-        **payer.lamports.borrow_mut() += lamports;
+        **maker.lamports.borrow_mut() += lamports;
 
         // Realloc the account to zero
         //

tokens/escrow/native/tests/instruction.ts

@@ -94,7 +94,7 @@ export function buildTakeOffer(props: {
             { address: props.taker_token_a, role: AccountRole.WRITABLE },
             { address: props.taker_token_b, role: AccountRole.WRITABLE },
             { address: props.vault, role: AccountRole.WRITABLE },
-            { address: props.maker, role: AccountRole.READONLY },
+            { address: props.maker, role: AccountRole.WRITABLE },
             { address: props.taker.address, role: AccountRole.WRITABLE_SIGNER, signer: props.taker },
             { address: props.payer.address, role: AccountRole.WRITABLE_SIGNER, signer: props.payer },
             { address: TOKEN_PROGRAM_ADDRESS, role: AccountRole.READONLY },

tokens/escrow/native/tests/test.ts

@@ -1,5 +1,8 @@
 import {
+    AccountRole,
+    type Address,
     appendTransactionMessageInstruction,
+    appendTransactionMessageInstructions,
     createTransactionMessage,
     generateKeyPairSigner,
     type Instruction,
@@ -17,7 +20,7 @@ import {
     getTokenSize,
     TOKEN_PROGRAM_ADDRESS,
 } from '@solana-program/token';
-import { getCreateAccountInstruction } from '@solana-program/system';
+import { getCreateAccountInstruction, getTransferSolInstruction } from '@solana-program/system';
 import { assert } from 'chai';
 import { FailedTransactionMetadata, LiteSVM } from 'litesvm';
 import { offerDecoder } from './account';
@@ -55,6 +58,23 @@ describe('Escrow!', () => {
         assert(!(result instanceof FailedTransactionMetadata), `transaction failed: ${result.toString()}`);
     }
 
+    // Sends the instructions as one transaction and returns the raw result,
+    // for tests that expect the transaction to be rejected.
+    async function trySendTransaction(instructions: Instruction[]) {
+        const transactionMessage = pipe(
+            createTransactionMessage({ version: 0 }),
+            m => setTransactionMessageFeePayerSigner(payer, m),
+            m => svm.setTransactionMessageLifetimeUsingLatestBlockhash(m),
+            m => appendTransactionMessageInstructions(instructions, m),
+        );
+        const signedTx = await signTransactionMessageWithSigners(transactionMessage);
+        return svm.sendTransaction(signedTx);
+    }
+
+    function balanceOf(address: Address): bigint {
+        return svm.getBalance(address) ?? 0n;
+    }
+
     it('mint tokens to maker and taker', async () => {
         // mint token a to maker account
         await mintingTokens({
@@ -484,4 +504,220 @@ describe('Escrow!', () => {
         const result = svm.sendTransaction(signedTx);
         assert(result instanceof FailedTransactionMetadata, 'expected a non-maker refund to fail');
     });
+
+    it('Take Offer returns the vault and offer rent to the maker, not the taker', async () => {
+        // The maker funded both the offer account and the vault in Make
+        // Offer, so closing them on take must hand that rent back to the
+        // maker. The taker (and whatever `payer` the taker chooses) must not
+        // be able to pocket it.
+        const offerValues = await createValues({
+            programId: values.programId,
+            maker: values.maker,
+            taker: values.taker,
+            mintAKeypair: values.mintAKeypair,
+            mintBKeypair: values.mintBKeypair,
+            id: 6n,
+        });
+
+        await sendTransaction(
+            buildMakeOffer({
+                id: offerValues.id,
+                maker: offerValues.maker,
+                maker_token_a: offerValues.makerAccountA,
+                offer: offerValues.offer,
+                token_a_offered_amount: offerValues.amountA,
+                token_b_wanted_amount: offerValues.amountB,
+                vault: offerValues.vault,
+                mint_a: offerValues.mintAKeypair.address,
+                mint_b: offerValues.mintBKeypair.address,
+                payer,
+                programId: offerValues.programId,
+            }),
+        );
+
+        const offerInfo = svm.getAccount(offerValues.offer);
+        assert(offerInfo.exists, 'offer account not created');
+        const vaultInfo = svm.getAccount(offerValues.vault);
+        assert(vaultInfo.exists, 'vault account not created');
+        const closedRent = offerInfo.lamports + vaultInfo.lamports;
+
+        const makerBalanceBefore = balanceOf(offerValues.maker.address);
+        const takerBalanceBefore = balanceOf(offerValues.taker.address);
+
+        await sendTransaction(
+            buildTakeOffer({
+                maker: offerValues.maker.address,
+                offer: offerValues.offer,
+                vault: offerValues.vault,
+                mint_a: offerValues.mintAKeypair.address,
+                mint_b: offerValues.mintBKeypair.address,
+                maker_token_b: offerValues.makerAccountB,
+                taker: offerValues.taker,
+                taker_token_a: offerValues.takerAccountA,
+                taker_token_b: offerValues.takerAccountB,
+                payer,
+                programId: offerValues.programId,
+            }),
+        );
+
+        assert(!svm.getAccount(offerValues.offer).exists, 'offer account not closed');
+        assert(!svm.getAccount(offerValues.vault).exists, 'vault account not closed');
+        assert.strictEqual(
+            balanceOf(offerValues.maker.address),
+            makerBalanceBefore + closedRent,
+            'the offer and vault rent should be returned to the maker',
+        );
+        assert.strictEqual(
+            balanceOf(offerValues.taker.address),
+            takerBalanceBefore,
+            'the taker should not receive any of the closed accounts rent',
+        );
+    });
+
+    it('Take Offer rejects a bogus system program account', async () => {
+        // take_offer closes the offer by assigning it to whatever account is
+        // passed in the `system_program` slot without checking its key. A
+        // taker can pass their own program there and, in the same
+        // transaction, top the emptied account back up to rent exemption so
+        // it survives - leaving the maker's (maker, id) offer slot
+        // permanently owned by the taker's program.
+        const offerValues = await createValues({
+            programId: values.programId,
+            maker: values.maker,
+            taker: values.taker,
+            mintAKeypair: values.mintAKeypair,
+            mintBKeypair: values.mintBKeypair,
+            id: 7n,
+        });
+
+        await sendTransaction(
+            buildMakeOffer({
+                id: offerValues.id,
+                maker: offerValues.maker,
+                maker_token_a: offerValues.makerAccountA,
+                offer: offerValues.offer,
+                token_a_offered_amount: offerValues.amountA,
+                token_b_wanted_amount: offerValues.amountB,
+                vault: offerValues.vault,
+                mint_a: offerValues.mintAKeypair.address,
+                mint_b: offerValues.mintBKeypair.address,
+                payer,
+                programId: offerValues.programId,
+            }),
+        );
+
+        // Both receiving token accounts already exist, so take_offer makes
+        // no CPI that would touch the system program before the close.
+        svm.expireBlockhash();
+        await sendTransaction(
+            getCreateAssociatedTokenIdempotentInstruction({
+                payer,
+                ata: offerValues.makerAccountB,
+                owner: offerValues.maker.address,
+                mint: offerValues.mintBKeypair.address,
+            }),
+        );
+        svm.expireBlockhash();
+        await sendTransaction(
+            getCreateAssociatedTokenIdempotentInstruction({
+                payer,
+                ata: offerValues.takerAccountA,
+                owner: offerValues.taker.address,
+                mint: offerValues.mintAKeypair.address,
+            }),
+        );
+
+        const bogusSystemProgram = (await generateKeyPairSigner()).address;
+        const ix = buildTakeOffer({
+            maker: offerValues.maker.address,
+            offer: offerValues.offer,
+            vault: offerValues.vault,
+            mint_a: offerValues.mintAKeypair.address,
+            mint_b: offerValues.mintBKeypair.address,
+            maker_token_b: offerValues.makerAccountB,
+            taker: offerValues.taker,
+            taker_token_a: offerValues.takerAccountA,
+            taker_token_b: offerValues.takerAccountB,
+            payer,
+            programId: offerValues.programId,
+        });
+        ix.accounts[ix.accounts.length - 1] = { address: bogusSystemProgram, role: AccountRole.READONLY };
+
+        const result = await trySendTransaction([
+            ix,
+            getTransferSolInstruction({
+                source: payer,
+                destination: offerValues.offer,
+                amount: svm.minimumBalanceForRentExemption(0n),
+            }),
+        ]);
+        assert(result instanceof FailedTransactionMetadata, 'expected a take with a bogus system program to fail');
+
+        const offerInfo = svm.getAccount(offerValues.offer);
+        assert(offerInfo.exists, 'offer account should be untouched');
+        assert.strictEqual(
+            offerInfo.programAddress,
+            offerValues.programId,
+            'offer account should still be program-owned',
+        );
+        assert.strictEqual(offerDecoder.decode(offerInfo.data).id, offerValues.id, 'offer data should be untouched');
+    });
+
+    it('Refund Offer rejects a bogus system program account', async () => {
+        // Same unchecked `system_program` slot as in take_offer.
+        const offerValues = await createValues({
+            programId: values.programId,
+            maker: values.maker,
+            taker: values.taker,
+            mintAKeypair: values.mintAKeypair,
+            mintBKeypair: values.mintBKeypair,
+            id: 8n,
+        });
+
+        await sendTransaction(
+            buildMakeOffer({
+                id: offerValues.id,
+                maker: offerValues.maker,
+                maker_token_a: offerValues.makerAccountA,
+                offer: offerValues.offer,
+                token_a_offered_amount: offerValues.amountA,
+                token_b_wanted_amount: offerValues.amountB,
+                vault: offerValues.vault,
+                mint_a: offerValues.mintAKeypair.address,
+                mint_b: offerValues.mintBKeypair.address,
+                payer,
+                programId: offerValues.programId,
+            }),
+        );
+
+        const bogusSystemProgram = (await generateKeyPairSigner()).address;
+        const ix = buildRefundOffer({
+            offer: offerValues.offer,
+            mint_a: offerValues.mintAKeypair.address,
+            maker_token_a: offerValues.makerAccountA,
+            vault: offerValues.vault,
+            maker: offerValues.maker,
+            programId: offerValues.programId,
+        });
+        ix.accounts[ix.accounts.length - 1] = { address: bogusSystemProgram, role: AccountRole.READONLY };
+
+        const result = await trySendTransaction([
+            ix,
+            getTransferSolInstruction({
+                source: payer,
+                destination: offerValues.offer,
+                amount: svm.minimumBalanceForRentExemption(0n),
+            }),
+        ]);
+        assert(result instanceof FailedTransactionMetadata, 'expected a refund with a bogus system program to fail');
+
+        const offerInfo = svm.getAccount(offerValues.offer);
+        assert(offerInfo.exists, 'offer account should be untouched');
+        assert.strictEqual(
+            offerInfo.programAddress,
+            offerValues.programId,
+            'offer account should still be program-owned',
+        );
+        assert.strictEqual(offerDecoder.decode(offerInfo.data).id, offerValues.id, 'offer data should be untouched');
+    });
 });

tokens/escrow/pinocchio/program/src/instructions/make_offer.rs

@@ -44,17 +44,16 @@ pub fn make_offer(program_id: &Address, accounts: &mut [AccountView], data: &[u8
     let token_b_wanted_amount = read_u64(data, 16)?;
     let bump = *data.get(24).ok_or(ProgramError::InvalidInstructionData)?;
 
-    // Verify the supplied offer account is the canonical PDA for these seeds.
+    // Verify the supplied offer account and bump are the canonical PDA for
+    // these seeds; any other bump would let a second offer live under the
+    // same (maker, id).
     let id_bytes = id.to_le_bytes();
-    let bump_bytes = [bump];
-    let offer_pda = Address::create_program_address(
-        &[Offer::SEED_PREFIX, maker.address().as_ref(), &id_bytes, &bump_bytes],
-        program_id,
-    )
-    .map_err(|_| ProgramError::InvalidSeeds)?;
-    if offer_account.address() != &offer_pda {
+    let (offer_pda, canonical_bump) =
+        Address::find_program_address(&[Offer::SEED_PREFIX, maker.address().as_ref(), &id_bytes], program_id);
+    if offer_account.address() != &offer_pda || bump != canonical_bump {
         return Err(ProgramError::InvalidSeeds);
     }
+    let bump_bytes = [bump];
 
     // Create the offer account, signed by the offer PDA itself.
     let lamports = Rent::get()?.try_minimum_balance(Offer::LEN)?;

tokens/escrow/pinocchio/program/src/instructions/refund_offer.rs

@@ -29,6 +29,10 @@ pub fn refund_offer(program_id: &Address, accounts: &mut [AccountView], _data: &
         return Err(ProgramError::MissingRequiredSignature);
     }
 
+    if !offer_account.owned_by(program_id) {
+        return Err(ProgramError::IllegalOwner);
+    }
+
     // Load the recorded offer terms (the borrow is released at the block's end).
     let offer = {
         let offer_data = offer_account.try_borrow()?;