Validate pushes to users in BeginSettle

Cargo.toml

@@ -25,6 +25,7 @@ solana-address = "2"
 solana-address-lookup-table-interface = "3"
 solana-hash = "3"
 solana-instruction = "3"
+solana-instructions-sysvar = "3"
 solana-program-error = "3"
 solana-pubkey = "3"
 solana-sdk = "3"

interface/src/instruction/settle/finalize.rs

@@ -12,6 +12,11 @@ use crate::{SettlementError, SettlementInstruction};
 
 use super::{recover_counterpart, INSTRUCTIONS_SYSVAR_ID, SPL_TOKEN_PROGRAM_ID};
 
+/// The number of fixed accounts every `FinalizeSettle` carries before its push
+/// accounts: the instructions sysvar, the settlement state PDA, and the token
+/// program.
+pub const FINALIZE_FIXED_ACCOUNTS: usize = 3;
+
 /// Builder for a `FinalizeSettle` instruction pushing the funds described by the
 /// parallel lists:
 /// - `source_buffers[i]` is the buffer token account the funds come from,
@@ -223,10 +228,21 @@ mod tests {
     use hex_literal::hex;
     use solana_address::Address;
 
-    /// The fixed accounts every `FinalizeSettle` carries before its push
-    /// accounts: the instructions sysvar, the settlement state PDA, and the
-    /// token program.
-    const FIXED_ACCOUNTS: usize = 3;
+    #[test]
+    fn finalize_fixed_accounts_matches_builder() {
+        // A no-push finalize carries exactly the fixed accounts, so the constant
+        // must equal the account count the builder emits with no pushes.
+        let ix = Instruction::from(FinalizeSettle {
+            program_id: Pubkey::new_unique(),
+            state_pda: Pubkey::new_unique(),
+            begin_ix_index: 0,
+            source_buffers: &[],
+            destinations: &[],
+            bumps: &[],
+            amounts: &[],
+        });
+        assert_eq!(ix.accounts.len(), FINALIZE_FIXED_ACCOUNTS);
+    }
 
     #[test]
     fn expected_encoding_finalize_settle_no_pushes() {
@@ -496,14 +512,14 @@ mod tests {
         ];
 
         // Too few: only one push account follows the fixed accounts.
-        let mut too_few = fake_sequential_accounts::<{ FIXED_ACCOUNTS + 1 }>();
+        let mut too_few = fake_sequential_accounts::<{ FINALIZE_FIXED_ACCOUNTS + 1 }>();
         assert_eq!(
             FinalizeSettleInput::parse(&data, &mut too_few).err(),
             Some(SettlementError::AccountCountNotMatchingPushCount.into()),
         );
 
         // Too many: three push accounts follow the fixed accounts.
-        let mut too_many = fake_sequential_accounts::<{ FIXED_ACCOUNTS + 3 }>();
+        let mut too_many = fake_sequential_accounts::<{ FINALIZE_FIXED_ACCOUNTS + 3 }>();
         assert_eq!(
             FinalizeSettleInput::parse(&data, &mut too_many).err(),
             Some(SettlementError::AccountCountNotMatchingPushCount.into()),
@@ -514,7 +530,7 @@ mod tests {
     fn finalize_settle_input_rejects_partial_push() {
         // Four trailing bytes: not a whole number of 9-byte pushes (a bump plus a
         // `u64` amount), so the body can't be parsed into the push layout.
-        let mut accounts = fake_sequential_accounts::<FIXED_ACCOUNTS>();
+        let mut accounts = fake_sequential_accounts::<FINALIZE_FIXED_ACCOUNTS>();
         let data = ix_data![
             [SettlementInstruction::FinalizeSettle.discriminator()],
             [13, 37],                 // begin index

interface/src/instruction/settle/mod.rs

@@ -10,7 +10,7 @@ mod begin;
 mod finalize;
 
 pub use begin::{BeginSettle, BeginSettleInput, Pull, SettledOrder};
-pub use finalize::{FinalizeSettle, FinalizeSettleInput, Push, Pushes};
+pub use finalize::{FinalizeSettle, FinalizeSettleInput, Push, Pushes, FINALIZE_FIXED_ACCOUNTS};
 
 /// Reads the first two bytes of a byte slice (instruction data) and
 /// interprets them as a big-endian u16, returning it together with the

interface/src/lib.rs

@@ -116,6 +116,14 @@ pub enum SettlementError {
     /// data: each push contributes a source buffer and a destination account,
     /// so the count must be twice the number of push amounts.
     AccountCountNotMatchingPushCount = 19,
+    /// `BeginSettle`: the number of pushes carried by the paired `FinalizeSettle`
+    /// doesn't equal the number of settled orders. Each order must be paid by
+    /// exactly one push.
+    SettledOrderPushCountMismatch = 20,
+    /// `BeginSettle`: a paired `FinalizeSettle` push doesn't send its proceeds
+    /// to the order's buy token account; its destination differs from the
+    /// `buy_token_account` in the order's intent.
+    PushDestinationMismatch = 21,
 }
 
 impl From<SettlementError> for u32 {

programs/settlement/Cargo.toml

@@ -26,6 +26,7 @@ proptest.workspace = true
 settlement-client.workspace = true
 settlement-interface = { workspace = true, features = ["test-fixtures"] }
 solana-address-lookup-table-interface = { workspace = true, features = ["bincode"] }
+solana-instructions-sysvar.workspace = true
 solana-sdk.workspace = true
 solana-system-interface.workspace = true
 

programs/settlement/src/settle/begin.rs

@@ -5,15 +5,19 @@ use std::ops::Deref;
 use pinocchio::{
     cpi::{Seed, Signer},
     error::ProgramError,
-    sysvars::{clock::Clock, instructions::Instructions, Sysvar},
+    sysvars::{
+        clock::Clock,
+        instructions::{Instructions, IntrospectedInstruction},
+        Sysvar,
+    },
     AccountView, Address, ProgramResult,
 };
 use pinocchio_token::{instructions::Transfer, state::Account as TokenAccount};
 use settlement_interface::{
     data::order::EncodedOrderAccount,
     instruction::{
         create_buffer::SPL_TOKEN_PROGRAM_ID,
-        settle::{BeginSettleInput, SettledOrder},
+        settle::{BeginSettleInput, SettledOrder, FINALIZE_FIXED_ACCOUNTS},
         InstructionInputParsing,
     },
     pda::{order::order_pda_signer_seeds, state::state_pda_seeds},
@@ -59,16 +63,44 @@ pub fn process_begin_settle(
         input.finalize_ix_index,
     )?;
 
-    pull_funds(
+    let finalize_ix = instructions.load_instruction_at(usize::from(input.finalize_ix_index))?;
+
+    settle_orders(
         program_id,
         input.token_program_account,
         input.state_pda_account,
         input.orders.iter(),
+        &finalize_ix,
     )?;
 
     Ok(())
 }
 
+/// The destination address of each push carried by the paired `FinalizeSettle`,
+/// seen through instruction introspection, in order and stopping at the first
+/// missing account.
+///
+/// The push structure isn't validated here: the paired `FinalizeSettle` re-parses
+/// the same instruction from its own data and rejects a dangling source buffer or
+/// a push count that disagrees with its accounts. The caller pairs these
+/// destinations with the settled orders one-to-one, which is what catches a count
+/// mismatch.
+fn push_destinations<'a>(
+    instruction: &'a IntrospectedInstruction<'a>,
+) -> impl Iterator<Item = &'a Address> {
+    // Each push occupies a `[source_buffer, destination]` meta pair after the
+    // fixed accounts, so the destinations are every second meta beginning at the
+    // first push's destination. The first index with no meta ends the list.
+    (FINALIZE_FIXED_ACCOUNTS + 1..)
+        .step_by(2)
+        .map_while(|destination_index| {
+            instruction
+                .get_instruction_account_at(destination_index)
+                .ok()
+                .map(|account| &account.key)
+        })
+}
+
 /// Reject a `BeginSettle` whose pair encloses another settlement: no
 /// `BeginSettle`/`FinalizeSettle` of this program may appear strictly between
 /// `current_index` and `finalize_ix_index`. The bounds themselves are excluded.
@@ -111,19 +143,26 @@ fn validate_no_nested_settlement<T: Deref<Target = [u8]>>(
     Ok(())
 }
 
-/// Validate and pull funds for each order, requiring:
+/// Validate each order against its push, and pull user funds. This requires:
 /// - the legacy SPL Token program;
 /// - the canonical state PDA, which signs each transfer as the user's delegate;
 /// - orders strictly increasing by address, rejecting duplicates.
 ///
-/// Further validation and the actual transfers are processed through
+/// Each order is paid by exactly one push. The orders and the finalize's pushes
+/// are both laid out sorted by order PDA, so order `i` is paid by push `i`, and
+/// that push's destination must be order `i`'s buy token account. Pairing them in
+/// a single pass (one push consumed per order, with none left over) rejects any
+/// count mismatch without counting the orders up front.
+///
+/// Further validation and the actual pulls are processed through
 /// [`process_order`].
 #[must_use = "ignoring the output may lead to an unintended on-chain state"]
-fn pull_funds<'a>(
+fn settle_orders<'a>(
     program_id: &Address,
     token_program_account: &AccountView,
     state_pda_account: &AccountView,
     orders: impl IntoIterator<Item = SettledOrder<'a>>,
+    finalize_ix: &IntrospectedInstruction,
 ) -> ProgramResult {
     if token_program_account.address() != &SPL_TOKEN_PROGRAM_ID {
         return Err(ProgramError::IncorrectProgramId);
@@ -147,26 +186,47 @@ fn pull_funds<'a>(
 
     let now = Clock::get()?.unix_timestamp;
 
+    // Pull one push destination per order; running out mid-loop means fewer pushes
+    // than orders. A leftover push (more pushes than orders) is caught after.
+    let mut destinations = push_destinations(finalize_ix);
+
     for order in orders {
         let order_pda = order.order_pda;
         if previous.is_some_and(|previous| order_pda.address() <= previous) {
             return Err(SettlementError::OrdersNotStrictlyIncreasing.into());
         }
         previous = Some(order_pda.address());
 
-        process_order(program_id, order, now, state_pda_account, &state_pda_signer)?;
+        let push_destination = destinations
+            .next()
+            .ok_or(SettlementError::SettledOrderPushCountMismatch)?;
+
+        process_order(
+            program_id,
+            order,
+            push_destination,
+            now,
+            state_pda_account,
+            &state_pda_signer,
+        )?;
+    }
+
+    if destinations.next().is_some() {
+        return Err(SettlementError::SettledOrderPushCountMismatch.into());
     }
 
     Ok(())
 }
 
-/// Validate a single order and process its pulls.
-/// This checks that the order is valid and settleable. Once the order passes
-/// those checks, its pulls are executed.
+/// Validate a single order, process its pulls, and confirm its push pays it.
+/// This checks that the order is valid, settleable, and that `push_destination`
+/// matches the buy token account. Once the order passes those checks, its pulls
+/// are executed.
 #[must_use = "ignoring the output may lead to an unintended on-chain state"]
 fn process_order(
     program_id: &Address,
     order: SettledOrder<'_>,
+    push_destination: &Address,
     now: i64,
     state_account: &AccountView,
     state_pda_signer: &Signer,
@@ -209,6 +269,11 @@ fn process_order(
         return Err(SettlementError::OrderExpired.into());
     }
 
+    // The push paying this order must send to the order's buy token account.
+    if !address_matches_pubkey(push_destination, &intent.buy_token_account) {
+        return Err(SettlementError::PushDestinationMismatch.into());
+    }
+
     // The sell token account must be the one named in the intent, owned by
     // the intent owner: an order can only sell funds its own owner controls.
     if !address_matches_pubkey(sell_token_account.address(), &intent.sell_token_account) {
@@ -245,3 +310,119 @@ fn process_order(
 fn address_matches_pubkey(address: &Address, pubkey: &Pubkey) -> bool {
     address.as_array() == &pubkey.to_bytes()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use proptest::prelude::*;
+    use settlement_interface::instruction::fixtures::fake_account;
+    use settlement_interface::instruction::settle::{FinalizeSettle, FinalizeSettleInput};
+    use settlement_interface::instruction::InstructionInputParsing;
+    use solana_instruction::{BorrowedAccountMeta, BorrowedInstruction, Instruction};
+
+    /// Strategy producing `count` random pushes as the parallel
+    /// `(source_buffers, destinations, bumps, amounts)` lists the `FinalizeSettle`
+    /// builder takes.
+    fn arb_pushes(
+        count: impl Into<prop::collection::SizeRange>,
+    ) -> impl Strategy<Value = (Vec<Pubkey>, Vec<Pubkey>, Vec<u8>, Vec<u64>)> {
+        prop::collection::vec(
+            (
+                any::<[u8; 32]>().prop_map(Pubkey::new_from_array),
+                any::<[u8; 32]>().prop_map(Pubkey::new_from_array),
+                any::<u8>(),
+                any::<u64>(),
+            ),
+            count,
+        )
+        .prop_map(|pushes| {
+            let source_buffers = pushes.iter().map(|&(source, ..)| source).collect();
+            let destinations = pushes.iter().map(|&(_, dest, ..)| dest).collect();
+            let bumps = pushes.iter().map(|&(.., bump, _)| bump).collect();
+            let amounts = pushes.iter().map(|&(.., amount)| amount).collect();
+            (source_buffers, destinations, bumps, amounts)
+        })
+    }
+
+    /// Encode `ix` as the introspected instruction from the instructions
+    /// sysvar.
+    fn introspected_instruction(ix: &Instruction) -> IntrospectedInstruction<'static> {
+        // From the Solana docs for `BorrowedInstruction`: "This struct is
+        // used by the runtime when constructing the instructions sysvar."
+        let borrowed = BorrowedInstruction {
+            program_id: &ix.program_id,
+            accounts: ix
+                .accounts
+                .iter()
+                .map(|meta| BorrowedAccountMeta {
+                    pubkey: &meta.pubkey,
+                    is_signer: meta.is_signer,
+                    is_writable: meta.is_writable,
+                })
+                .collect(),
+            data: &ix.data,
+        };
+        // From the Solana docs for this function: "construct the account data
+        // for the instructions sysvar."
+        let instructions_sysvar_data =
+            solana_instructions_sysvar::construct_instructions_data(&[borrowed]);
+        // SAFETY: from Pinocchio's docs for `new_unchecked`: "this function is
+        // unsafe because it does not check if the provided data is from the
+        // Sysvar Account."
+        // We built the data using `construct_instructions_data`, so we know the
+        // data is correctly built.
+        // https://docs.rs/pinocchio/0.11.1/pinocchio/sysvars/instructions/struct.Instructions.html#method.new_unchecked
+        // https://docs.rs/solana-instructions-sysvar/3.0.0/src/solana_instructions_sysvar/lib.rs.html#85-141
+        let instructions = unsafe { Instructions::new_unchecked(instructions_sysvar_data) };
+        // Leak the buffer so the returned view can borrow it for the rest of the
+        // test process.
+        let instructions: &'static Instructions<Vec<u8>> = Box::leak(Box::new(instructions));
+        instructions
+            .load_instruction_at(0)
+            .expect("the finalize is the only instruction, at index 0")
+    }
+
+    proptest! {
+        /// `BeginSettle` reads a paired `FinalizeSettle`'s push destinations by
+        /// introspection through `push_destinations`, while `FinalizeSettle`
+        /// reads its own pushes via `FinalizeSettleInput` (off the instruction
+        /// data + accounts).
+        /// For any well-formed finalize the two must recover the same push
+        /// count and the same destination for every push.
+        #[test]
+        fn finalize_pushes_agrees_with_finalize_settle_input(
+            program_id in any::<[u8; 32]>(),
+            state_pda in any::<[u8; 32]>(),
+            begin_ix_index in any::<u16>(),
+            (source_buffers, destinations, bumps, amounts) in arb_pushes(0..=16usize),
+        ) {
+            let count = source_buffers.len();
+
+            let ix = Instruction::from(FinalizeSettle {
+                program_id: Pubkey::new_from_array(program_id),
+                state_pda: Pubkey::new_from_array(state_pda),
+                begin_ix_index,
+                source_buffers: &source_buffers,
+                destinations: &destinations,
+                bumps: &bumps,
+                amounts: &amounts,
+            });
+
+            let introspected = introspected_instruction(&ix);
+            let introspected_destinations: Vec<Address> =
+                push_destinations(&introspected).copied().collect();
+
+            let mut accounts: Vec<AccountView> =
+                ix.accounts.iter().map(|account| fake_account(account.pubkey)).collect();
+            let parsed = FinalizeSettleInput::parse(&ix.data, &mut accounts)
+                .expect("a well-formed finalize parses");
+            let parsed_destinations: Vec<Address> =
+                parsed.pushes.iter().map(|push| *push.destination.address()).collect();
+
+            prop_assert_eq!(introspected_destinations.len(), count);
+            prop_assert_eq!(parsed.pushes.iter().count(), count);
+            prop_assert_eq!(&introspected_destinations, &destinations);
+            prop_assert_eq!(&parsed_destinations, &destinations);
+        }
+    }
+}