diff --git a/README.md b/README.md index f7cc44e..afa4f40 100644 --- a/README.md +++ b/README.md @@ -106,6 +106,15 @@ wrapped life — escrow is set once at mint (via the wrapper's `TransferPortfoli tag 72) and released only at burn (via `UnwrapEscrowedPortfolio` tag 82). An NFT transfer moves only the bearer token; the underlying position stays escrowed. +The hook's one state write is `PositionNftV16.last_holder`, recorded whenever a +transfer provably moved the token — a direct Token-2022 transfer, or one +Token-2022 is executing on behalf of another program (marketplace, orderbook, +multisig). It is detected via Token-2022's own in-flight `transferring` flag, so +a spoofed direct `Execute` cannot forge it. `ReconcileBurnedNft` uses this field +as the sole authorisation for releasing an escrowed portfolio after an +out-of-band burn, so it must track the current holder, and extra-meta entry [5] +must stay writable for every such transfer. + ## Build and Test ```bash diff --git a/src/instruction.rs b/src/instruction.rs index 9ebfe41..9dfc59c 100644 --- a/src/instruction.rs +++ b/src/instruction.rs @@ -118,8 +118,9 @@ pub const TAG_EMERGENCY_BURN: u8 = 5; /// Current flags (see EXTRA_META_ENTRY_FLAGS in processor.rs): /// [5] PositionNft PDA — WRITABLE. #105 removed the f_snap_at_mint write, but /// #152/#153 added a new one: the hook writes `nft_state.last_holder` on -/// every genuine Token-2022 transfer, so this must stay writable or such a -/// transfer fails. +/// every transfer that provably moved the token — a genuine direct +/// Token-2022 transfer AND a marketplace/orderbook CPI transfer — so this +/// must stay writable or such a transfer fails. /// [6] Portfolio — read-only. The hook performs no invoke/invoke_signed /// and never mutably borrows it; the pre-#105 B-3 ownership CPI that once /// required write access moved to mint/burn. diff --git a/src/processor.rs b/src/processor.rs index 7a003c8..3b4a2b5 100644 --- a/src/processor.rs +++ b/src/processor.rs @@ -249,9 +249,11 @@ fn registry_registers_program(data: &[u8], program_id: &Pubkey) -> bool { /// live NFT's metas into a shape the hook cannot use. /// /// Entry 5 (PositionNft PDA) must stay writable: #105 removed the f_snap_at_mint write, -/// but #152/#153 added `nft_state.last_holder = new_owner` in process_transfer_hook -/// (transfer_hook.rs:542-545), which runs on every genuine Token-2022 transfer. A -/// read-only meta there makes each such TransferChecked fail. +/// but #152/#153 added `nft_state.last_holder = new_owner` in `process_execute`, which +/// runs on every transfer that provably moved the token -- a genuine direct Token-2022 +/// TransferChecked AND a marketplace/orderbook CPI transfer. A read-only meta there +/// makes each such transfer fail. Note this set is WIDER than it once was: the write +/// was previously skipped for CPI transfers, which is what left `last_holder` stale. /// /// Entry 6 (Portfolio) is read-only: the hook performs no invoke/invoke_signed and never /// mutably borrows `portfolio` — it only reads it to check the NFT PDA binding. @@ -1875,18 +1877,18 @@ mod extra_meta_flag_tests { use super::*; /// Entry 5 is the PositionNft PDA and the transfer hook WRITES it - /// (`nft_state.last_holder = new_owner`, transfer_hook.rs:542-545, gated on - /// `is_genuine_token2022_transfer`). Flipping it read-only makes every genuine - /// Token-2022 TransferChecked fail, and because RepairExtraMetas is permissionless - /// that is a free brick-any-NFT vector. This pins the flag so the regression cannot - /// return silently — the 34 pre-existing tests all passed with it read-only. + /// (`nft_state.last_holder = new_owner` in `process_execute`, gated on a genuine + /// direct transfer OR Token-2022's in-flight `transferring` flag). Flipping it + /// read-only makes every such transfer fail — direct and marketplace-CPI alike — + /// and because RepairExtraMetas is permissionless that is a free brick-any-NFT + /// vector. This pins the flag so the regression cannot return silently. #[test] fn position_nft_pda_meta_entry_is_writable() { assert_eq!( EXTRA_META_ENTRY_FLAGS[0], (false, true), "entry 5 (PositionNft PDA) must be non-signer and WRITABLE: the transfer hook \ - writes last_holder to it on every genuine Token-2022 transfer" + writes last_holder to it on every transfer that moved the token, including marketplace/orderbook CPI transfers" ); } diff --git a/src/transfer_hook.rs b/src/transfer_hook.rs index 33b6451..83e3f14 100644 --- a/src/transfer_hook.rs +++ b/src/transfer_hook.rs @@ -98,25 +98,31 @@ const TOKEN_IX_TRANSFER_CHECKED_WITH_FEE: u8 = 26; /// `Ok(true)` — outer instruction IS Token-2022 TransferChecked/WithFee; /// this is a direct wallet→wallet transfer. The `last_holder` /// state write is authorised. -/// `Ok(false)` — outer instruction is NOT Token-2022 (CPI-initiated path, e.g. -/// marketplace/orderbook); validation gates pass, but the caller -/// MUST NOT write `last_holder` (the new_owner value is -/// attacker-controllable in this path — #152/#153 fix). +/// `Ok(false)` — outer instruction is NOT Token-2022: either a CPI-initiated +/// transfer (marketplace/orderbook) or a spoofed direct Execute. +/// Validation gates pass. This alone does not AUTHORISE the +/// `last_holder` write, but it no longer FORBIDS it either — +/// the caller additionally consults Token-2022's in-flight +/// `transferring` flag, which separates the two cases. /// `Err(_)` — outer instruction is Token-2022 but is invalid (wrong tag, /// wrong mint, empty data); reject the transfer entirely. /// -/// This is the canonical anti-spoofing signal for the SPL TransferHook interface. -/// Token-2022 sets source/dest `TransferHookAccount.transferring = true` only for -/// the duration of a real transfer, but reading that extension requires the -/// `spl-token-2022` crate which conflicts with our `solana-program = 2.2.1` pin -/// (see Cargo.toml). The top-level-instruction check is the next-best equivalent: -/// it correctly distinguishes genuine direct transfers (Token-2022 top-level) from -/// CPI-initiated transfers (marketplace top-level) AND from spoofed direct Execute -/// calls (both of the latter return `Ok(false)`, skipping the `last_holder` write). +/// This separates a genuine DIRECT transfer from everything else. It does NOT +/// separate a genuine marketplace/orderbook CPI transfer from a spoofed direct +/// Execute — both return `Ok(false)` — so on its own it cannot decide the +/// `last_holder` write. `account_is_transferring` supplies that second signal. +/// +/// An earlier note here claimed the canonical signal (Token-2022's +/// `TransferHookAccount.transferring`) was unreadable because it needs the +/// `spl-token-2022` crate, which conflicts with our `solana-program` pin. That is +/// not so: the flag is a single byte of TLV in the token account, and this file +/// already hand-parses those same accounts for mint/owner/state. No dependency +/// was added to read it. /// /// #145 composability: Requiring top-level = Token-2022 for the ENTIRE Execute -/// would block marketplace/orderbook CPI transfers. We only require it for the -/// `last_holder` WRITE — the validation gates run for both paths. +/// would block marketplace/orderbook CPI transfers, so it is not required at all +/// for the transfer to proceed — the validation gates run for both paths, and the +/// `last_holder` write is decided by the in-flight flag rather than by this check. /// /// How it works: /// - On Solana, when program A CPI-calls program B, program B's instruction @@ -147,23 +153,24 @@ fn verify_cpi_caller_is_token2022( // by another program via CPI — e.g. an NFT marketplace or the position // orderbook moving the NFT on a match. The validation gates (ATA checks, // bound-leg / market_id anchor, transfer-gate, registry) still run and must - // pass. The `last_holder` write is controlled by the return value: `false` - // tells the caller to skip it. This is safe because: - // (a) In a legitimate marketplace CPI, Token-2022 is invoked by the - // marketplace, which is invoked as the top-level instruction. The - // actual NFT token moves; the last genuine holder update happened at - // the previous genuine Token-2022 top-level transfer (or mint). Skipping - // the update here means `last_holder` stays at the PREVIOUS genuine - // holder — still correct for ReconcileBurnedNft. + // pass. A `false` return means "not a genuine DIRECT transfer"; it no longer + // decides the `last_holder` write on its own. This is safe because: + // (a) In a legitimate marketplace CPI the NFT really does move, so the + // holder MUST be recorded. Returning `false` here does not skip that + // write any more; the caller recovers the case from Token-2022's + // in-flight flag. Treating this case as "no write" is what pinned + // `last_holder` at the minter for every program-traded NFT and let + // ReconcileBurnedNft pay an already-paid seller. // (b) In a spoofed direct Execute call (attacker invokes Execute directly, - // top-level = attacker's program), we also return `false`, so - // `last_holder` is never updated. Theft closed. + // top-level = attacker's program) we also return `false`, and no + // transfer is in flight, so `last_holder` is never updated. Theft + // stays closed. // // Requiring the top-level instruction to be Token-2022 would block every // program-escrow marketplace/orderbook transfer — the core of the - // composability goal (#145). We only gate the last_holder WRITE on it. + // composability goal (#145). if current_ix.program_id != token2022::TOKEN_2022_PROGRAM_ID { - return Ok(false); // CPI-initiated or spoofed direct call: gates pass, no last_holder write + return Ok(false); // CPI-initiated or spoofed: not a genuine DIRECT transfer } // Top-level IS Token-2022 (a direct wallet→wallet transfer): keep the strict // instruction-type + mint-match validation below (incl. #103 plain-Transfer reject). @@ -240,6 +247,67 @@ fn verify_cpi_caller_is_token2022( // Execute — called by Token-2022 on every NFT transfer // ═══════════════════════════════════════════════════════════════ +/// `ExtensionType::TransferHookAccount` (`spl-token-2022`, `#[repr(u16)]`). +const EXT_TYPE_TRANSFER_HOOK_ACCOUNT: u16 = 15; +/// Length of the Token-2022 base token account, before `account_type` + TLV. +const TOKEN_ACCOUNT_BASE_LEN: usize = 165; +/// `AccountType::Account` — the discriminant at byte [165] of a token account. +const ACCOUNT_TYPE_ACCOUNT: u8 = 2; + +/// Read `TransferHookAccount.transferring` straight out of a token account's +/// TLV region. +/// +/// Token-2022 sets this flag on BOTH the source and the destination immediately +/// before invoking a transfer hook and clears it immediately afterwards (see +/// `process_transfer`). It is therefore true exactly while a genuine transfer is +/// in flight — for direct AND CPI-initiated transfers alike — and false during +/// a spoofed direct `Execute` call, where no transfer is running at all. That is +/// the discrimination the top-level-instruction heuristic stands in for, minus +/// the marketplace blind spot. +/// +/// Layout: base account is 165 bytes, `account_type` at [165], then TLV entries +/// of `[type: u16 LE][len: u16 LE][value; len]` from [166]. The value here is a +/// 1-byte `PodBool`. No new dependency is needed; this file already hand-parses +/// the same accounts for mint/owner/state. +/// +/// Returns `false` rather than an error when the extension is absent or the +/// account carries no TLV region, so callers degrade to prior behaviour. +/// +/// LOAD-BEARING INVARIANT. This signal is trustworthy only because the flag can +/// be observed set exclusively from inside Token-2022's own `invoke_execute`: +/// (i) `set_transferring` / `unset_transferring` bracket that call; +/// (ii) a failing inner instruction aborts the whole transaction, so a +/// set-without-unset state can never commit or be seen later; and +/// (iii) `process_execute` performs NO CPI, so no foreign code runs inside +/// the window. +/// If this program ever gains a CPI from `process_execute`, revisit this. +fn account_is_transferring(ata: &AccountInfo) -> Result { + let data = ata.try_borrow_data()?; + if data.len() <= TOKEN_ACCOUNT_BASE_LEN { + return Ok(false); + } + // [165] is `account_type`; TLV entries start at [166]. TransferHookAccount is + // account-scoped, so anything not tagged as a token account cannot carry it. + if data[TOKEN_ACCOUNT_BASE_LEN] != ACCOUNT_TYPE_ACCOUNT { + return Ok(false); + } + let mut off = TOKEN_ACCOUNT_BASE_LEN + 1; + while off + 4 <= data.len() { + let ext_type = u16::from_le_bytes([data[off], data[off + 1]]); + let len = usize::from(u16::from_le_bytes([data[off + 2], data[off + 3]])); + off += 4; + // `Uninitialized` marks the end of the initialised TLV region. + if ext_type == 0 || off + len > data.len() { + break; + } + if ext_type == EXT_TYPE_TRANSFER_HOOK_ACCOUNT { + return Ok(len >= 1 && data[off] != 0); + } + off += len; + } + Ok(false) +} + /// Process the TransferHook Execute instruction. /// /// Account layout (Token-2022 passes the 4 interface accounts then the 7 @@ -387,9 +455,10 @@ pub fn process_execute( // // Returns Ok(true) — genuine direct Token-2022 transfer (direct wallet). // Returns Ok(false) — CPI-initiated (marketplace/orderbook) OR spoofed - // direct Execute call. Both pass the validation gates; - // only genuine direct transfers authorise the - // last_holder write (#152/#153 anti-spoof fix). + // direct Execute call. Both pass the validation gates. + // These two are separated later by Token-2022's + // in-flight `transferring` flag, which authorises the + // last_holder write for the former but not the latter. // Returns Err(_) — Token-2022 top-level but invalid tag/mint; reject. // ──────────────────────────────────────────────────────────────────── let is_genuine_token2022_transfer = verify_cpi_caller_is_token2022(sysvar_ix, mint.key)?; @@ -515,31 +584,36 @@ pub fn process_execute( // no longer forwarded into one. let _ = nft_program_self; - // #138 + #152/#153 anti-spoof: record the new holder ONLY when the outer - // instruction is provably a genuine Token-2022 TransferChecked/WithFee - // (is_genuine_token2022_transfer == true). This is the canonical signal that - // a real NFT token movement occurred — not a spoofed direct Execute call or a - // marketplace CPI whose new_owner (dest_ata.data[32..64]) is attacker-controlled. + // #138 + #152/#153: record the new holder whenever a real NFT token movement + // provably occurred — either a genuine direct Token-2022 TransferChecked / + // WithFee, or a transfer Token-2022 is executing right now on behalf of + // another program. Both are real; a spoofed direct Execute is neither. + // + // A CPI-initiated transfer is just as genuine as a direct one, so the write + // is additionally authorised by Token-2022's own in-flight signal. The flag + // is set on BOTH the source and the destination immediately before the hook + // is invoked and cleared immediately after, so requiring both to be set + // admits every real transfer (direct or marketplace/orderbook CPI) while + // still excluding a spoofed direct `Execute`, where no transfer is running. + // + // Gating on the top-level instruction ALONE left `last_holder` pinned at the + // minter for any NFT traded only through a program. Since `last_holder` is + // the sole authorisation for ReconcileBurnedNft's escrow release, that paid + // the entire escrowed portfolio to a seller who had already been paid, and + // locked the actual owner out of their own recovery. // - // Why skipping the write for CPI-initiated marketplace transfers is safe: - // - `last_holder` is consumed ONLY by ReconcileBurnedNft — which fires when - // the NFT was burned out-of-band. In that scenario the most recent genuine - // transfer's last_holder is the correct recovery target. If the last genuine - // transfer was marketplace-CPI-initiated the last_holder from the previous - // direct transfer (or mint) remains recorded — still the rightful party. - // - The marketplace path successfully validates all gates (ATA, bound-leg, - // transfer-gate, registry) and the token moves; it just does not update - // last_holder. A recipient who receives an NFT only via marketplace CPI and - // never via direct TransferChecked can still use BurnPositionNft normally - // (which does not consult last_holder at all); only ReconcileBurnedNft falls - // back to last_holder, and in that case the previous genuine-transfer holder - // is a safe default (both parties are real holders; neither is an attacker). + // The disjunction is deliberate and monotonic: every case authorised before + // is still authorised, so if a token account somehow carries no + // TransferHookAccount extension the behaviour degrades to exactly the prior + // (fail-closed) one rather than regressing. // - // Attack closed: a spoofed direct Execute (top-level = attacker's program) or a - // thin CPI-wrapper attacking Execute receives is_genuine_token2022_transfer==false - // and cannot forge last_holder. ReconcileBurnedNft therefore cannot be used to - // divert the escrowed position to an attacker-controlled address. - if is_genuine_token2022_transfer { + // Attack still closed: a spoofed direct Execute (top-level = attacker's + // program) or a thin CPI-wrapper sees is_genuine_token2022_transfer == false + // AND both transferring flags clear, so it cannot forge last_holder. + let transfer_in_flight = + account_is_transferring(source_ata)? && account_is_transferring(dest_ata)?; + let holder_recorded = is_genuine_token2022_transfer || transfer_in_flight; + if holder_recorded { let mut pda_data = nft_pda.try_borrow_mut_data()?; let nft_state = bytemuck::from_bytes_mut::(&mut pda_data[..POSITION_NFT_V16_LEN]); @@ -550,7 +624,7 @@ pub fn process_execute( "Position NFT transferred (position remains escrowed): portfolio={}, asset_index={}, new_holder_recorded={}", portfolio.key, asset_index_u16, - is_genuine_token2022_transfer + holder_recorded ); Ok(()) @@ -568,12 +642,14 @@ pub fn process_execute( // tag = 12, correct mint → Ok(true) → last_holder write authorised. // // (B) Marketplace/orderbook CPI: top-level = marketplace program (not -// Token-2022) → Ok(false) → last_holder write skipped; transfer gate +// Token-2022) → Ok(false); the last_holder write is then decided by the +// in-flight `transferring` flag, not by this return value; transfer gate // still passes (composability preserved). // // (C) Spoofed direct Execute call: attacker invokes Execute directly with // top-level = attacker's program (also not Token-2022) → Ok(false) → -// last_holder write skipped. Theft closed. +// Ok(false) AND no transfer in flight → last_holder write skipped. +// Theft closed. // // (D) Spoofed fake Token-2022 top-level with wrong mint: top-level IS // Token-2022 but TransferChecked mint != expected → Err(Unauthorized). @@ -733,7 +809,7 @@ mod last_holder_antispoof_152_153 { assert_eq!( result, Ok(false), - "marketplace/orderbook CPI (non-Token-2022 top-level) must return Ok(false) — gates pass, no last_holder write (#145 composability preserved)" + "marketplace/orderbook CPI (non-Token-2022 top-level) must return Ok(false) — gates pass; the last_holder write is decided separately by the in-flight flag (#145 composability preserved)" ); } diff --git a/tests/poc_stale_last_holder.rs b/tests/poc_stale_last_holder.rs new file mode 100644 index 0000000..2da9e25 --- /dev/null +++ b/tests/poc_stale_last_holder.rs @@ -0,0 +1,530 @@ +//! PoC: a marketplace sale leaves `last_holder` at the SELLER, and +//! `ReconcileBurnedNft` then pays the entire escrowed portfolio to that seller. +//! +//! PR #159 gated the `last_holder` write on a genuine top-level Token-2022 +//! transfer to close the #152/#153 forgery. Its stated residual was: +//! +//! "an NFT acquired ONLY via marketplace-CPI leaves `last_holder` stale, +//! affecting only the rare out-of-band-burn ReconcileBurnedNft recovery +//! (which then routes to the prior genuine holder - no theft, no new drain)." +//! +//! These tests exercise the four links of the chain that claim depends on: +//! +//! 1. mint -> last_holder = minter (processor.rs:500) +//! 2. marketplace CPI -> hook returns Ok, NO write (transfer_hook.rs:166,542) +//! 3. out-of-band burn (supply -> 0), the #138 scenario +//! 4. ReconcileBurnedNft -> buyer REJECTED, seller PAID (processor.rs:1275,1326) + +use bytemuck::Zeroable; +use percolator_nft::{ + cpi_v16::{derive_nft_registry, PERCOLATOR_MAINNET}, + instruction::TAG_RECONCILE_BURNED_NFT, + processor, + slab_types_v16 as sl, + state_v16::{ + mint_authority_pda, position_nft_pda, PositionNftV16, POSITION_NFT_V16_LEN, + POSITION_NFT_V16_MAGIC, POSITION_NFT_V16_VERSION, + }, + token2022::TOKEN_2022_PROGRAM_ID, + transfer_hook::{extra_account_metas_pda, process_execute}, +}; +use solana_program::{ + account_info::AccountInfo, program_error::ProgramError, pubkey::Pubkey, + sysvar::instructions as sysvar_instructions, +}; + +// -- cast -------------------------------------------------------------------- +const PROG: Pubkey = Pubkey::new_from_array([9u8; 32]); // this NFT program +const ALICE: Pubkey = Pubkey::new_from_array([0xA1; 32]); // minter, then SELLER +const BOB: Pubkey = Pubkey::new_from_array([0xB0; 32]); // BUYER +const MARKETPLACE: Pubkey = Pubkey::new_from_array([0x33; 32]); // escrow marketplace +const PORTFOLIO: Pubkey = Pubkey::new_from_array([0x50; 32]); +const MARKET_GROUP: Pubkey = Pubkey::new_from_array([0x60; 32]); +const NFT_MINT: Pubkey = Pubkey::new_from_array([0x11; 32]); +const SRC_ATA: Pubkey = Pubkey::new_from_array([0x71; 32]); +const DST_ATA: Pubkey = Pubkey::new_from_array([0x72; 32]); + +const ASSET_INDEX: u32 = 7; +const MARKET_ID: u64 = 42; +const PDA_RENT: u64 = 2_000_000; + +fn leak(v: T) -> &'static mut T { + Box::leak(Box::new(v)) +} + +fn acct( + key: Pubkey, + owner: Pubkey, + data: Vec, + lamports: u64, + writable: bool, +) -> AccountInfo<'static> { + AccountInfo::new( + leak(key), + false, + writable, + leak(lamports), + Box::leak(data.into_boxed_slice()), + leak(owner), + false, + 0, + ) +} + +/// Token-2022 base token-account image (165 bytes). +fn token_account(mint: &Pubkey, owner: &Pubkey, amount: u64) -> Vec { + let mut d = vec![0u8; 165]; + d[0..32].copy_from_slice(mint.as_ref()); + d[32..64].copy_from_slice(owner.as_ref()); + d[64..72].copy_from_slice(&amount.to_le_bytes()); + d[108] = 1; // Initialized + d +} + +/// Same, plus a `TransferHookAccount` TLV entry (ExtensionType 15, 1-byte +/// PodBool). Token-2022 sets `transferring` on source AND destination for the +/// duration of a real transfer, so this is what the hook sees in flight. +fn token_account_ext(mint: &Pubkey, owner: &Pubkey, amount: u64, transferring: bool) -> Vec { + token_account_ext_typed(mint, owner, amount, transferring, 2) +} + +fn token_account_ext_typed( + mint: &Pubkey, + owner: &Pubkey, + amount: u64, + transferring: bool, + account_type: u8, +) -> Vec { + let mut d = token_account(mint, owner, amount); + d.resize(171, 0); + d[165] = account_type; + d[166..168].copy_from_slice(&15u16.to_le_bytes()); // TransferHookAccount + d[168..170].copy_from_slice(&1u16.to_le_bytes()); // value length + d[170] = u8::from(transferring); + d +} + +/// The layout a REAL ATA has: the ATA program installs `ImmutableOwner` +/// (type 7, length 0) first, then Token-2022 appends `TransferHookAccount` +/// (type 15, length 1) because the mint carries the TransferHook extension. +/// A parser that mishandles the leading zero-length entry would silently read +/// `false` here and restore the very bug this fix closes. +fn token_account_realistic_ata( + mint: &Pubkey, + owner: &Pubkey, + amount: u64, + transferring: bool, +) -> Vec { + let mut d = token_account(mint, owner, amount); + d.resize(175, 0); + d[165] = 2; // account_type = Account + d[166..168].copy_from_slice(&7u16.to_le_bytes()); // ImmutableOwner + d[168..170].copy_from_slice(&0u16.to_le_bytes()); // length 0, no value + d[170..172].copy_from_slice(&15u16.to_le_bytes()); // TransferHookAccount + d[172..174].copy_from_slice(&1u16.to_le_bytes()); + d[174] = u8::from(transferring); + d +} + +/// Token-2022 base mint image; `supply` is a u64 at offset 36. +fn mint_account(supply: u64) -> Vec { + let mut d = vec![0u8; 200]; + d[36..44].copy_from_slice(&supply.to_le_bytes()); + d[45] = 1; // is_initialized + d +} + +/// Wrapper-framed portfolio, escrowed to the mint-authority PDA (#105). +fn portfolio_buf(escrow_owner: [u8; 32]) -> Vec { + let mut a: sl::PortfolioAccountV16Account = Zeroable::zeroed(); + a.provenance_header.market_group_id = MARKET_GROUP.to_bytes(); + a.provenance_header.portfolio_account_id = PORTFOLIO.to_bytes(); + a.provenance_header.owner = escrow_owner; + a.provenance_header.version = sl::V16PodU16::new(sl::V16_ACCOUNT_VERSION); + a.provenance_header.layout_discriminator = sl::V16PodU16::new(sl::V16_LAYOUT_DISCRIMINATOR); + a.owner = escrow_owner; + a.legs[0].active = 1; + a.legs[0].asset_index = sl::V16PodU32::new(ASSET_INDEX); + a.legs[0].market_id = sl::V16PodU64::new(MARKET_ID); + a.legs[0].stale = 0; + + let mut buf = vec![0u8; sl::HEADER_LEN + sl::EXPECTED_PORTFOLIO_ACCOUNT_SIZE]; + buf[0..8].copy_from_slice(&sl::MAGIC.to_le_bytes()); + buf[8..10].copy_from_slice(&sl::VERSION.to_le_bytes()); + buf[10] = sl::KIND_PORTFOLIO; + buf[sl::HEADER_LEN..].copy_from_slice(bytemuck::bytes_of(&a)); + buf +} + +fn nft_pda_buf(bump: u8, last_holder: [u8; 32]) -> Vec { + let mut s: PositionNftV16 = Zeroable::zeroed(); + s.magic = sl::V16PodU64::new(POSITION_NFT_V16_MAGIC); + s.version = POSITION_NFT_V16_VERSION; + s.bump = bump; + s.portfolio_account = PORTFOLIO.to_bytes(); + s.nft_mint = NFT_MINT.to_bytes(); + s.asset_index = sl::V16PodU32::new(ASSET_INDEX); + s.market_id_at_mint = sl::V16PodU64::new(MARKET_ID); + s.last_holder = last_holder; + bytemuck::bytes_of(&s).to_vec() +} + +/// Instructions-sysvar image with exactly one top-level instruction. +fn build_sysvar(top_prog: &Pubkey, data: &[u8], accounts: &[Pubkey]) -> Vec { + let mut ib: Vec = Vec::new(); + ib.extend_from_slice(&(accounts.len() as u16).to_le_bytes()); + for a in accounts { + ib.push(0u8); + ib.extend_from_slice(a.as_ref()); + } + ib.extend_from_slice(top_prog.as_ref()); + ib.extend_from_slice(&(data.len() as u16).to_le_bytes()); + ib.extend_from_slice(data); + let total = 2 + 2 + ib.len() + 2; + let mut sv = vec![0u8; total]; + sv[0..2].copy_from_slice(&1u16.to_le_bytes()); // num instructions + sv[2..4].copy_from_slice(&4u16.to_le_bytes()); // offset of instruction 0 + sv[4..4 + ib.len()].copy_from_slice(&ib); + sv[total - 2..].copy_from_slice(&0u16.to_le_bytes()); // current index + sv +} + +fn read_last_holder(nft_pda: &AccountInfo) -> [u8; 32] { + let d = nft_pda.data.borrow(); + bytemuck::from_bytes::(&d[..POSITION_NFT_V16_LEN]).last_holder +} + +/// Drive `process_execute` for a transfer whose TOP-LEVEL instruction is +/// `top_prog`. Returns the result plus the nft_pda so callers can inspect it. +fn run_hook( + top_prog: Pubkey, + top_data: Vec, + top_accounts: Vec, + starting_last_holder: [u8; 32], + in_flight: Option<(bool, bool)>, +) -> (Result<(), ProgramError>, AccountInfo<'static>) { + run_hook_inner(top_prog, top_data, top_accounts, starting_last_holder, in_flight, false) +} + +fn run_hook_inner( + top_prog: Pubkey, + top_data: Vec, + top_accounts: Vec, + starting_last_holder: [u8; 32], + in_flight: Option<(bool, bool)>, + realistic: bool, +) -> (Result<(), ProgramError>, AccountInfo<'static>) { + let (mint_auth, _) = mint_authority_pda(&PROG); + let (nft_pda_key, bump) = position_nft_pda(&PORTFOLIO, MARKET_ID, &PROG); + let (metas, _) = extra_account_metas_pda(&NFT_MINT, &PROG); + let (registry, _) = derive_nft_registry(&PERCOLATOR_MAINNET, &MARKET_GROUP); + + let nft_pda = acct( + nft_pda_key, + PROG, + nft_pda_buf(bump, starting_last_holder), + PDA_RENT, + true, + ); + + let accounts = vec![ + acct( + SRC_ATA, + TOKEN_2022_PROGRAM_ID, + match in_flight { + None => token_account(&NFT_MINT, &ALICE, 1), + Some((src, _)) if realistic => { + token_account_realistic_ata(&NFT_MINT, &ALICE, 1, src) + } + Some((src, _)) => token_account_ext(&NFT_MINT, &ALICE, 1, src), + }, + 0, + false, + ), + acct(NFT_MINT, TOKEN_2022_PROGRAM_ID, mint_account(1), 0, false), + acct( + DST_ATA, + TOKEN_2022_PROGRAM_ID, + match in_flight { + None => token_account(&NFT_MINT, &BOB, 0), + Some((_, dst)) if realistic => { + token_account_realistic_ata(&NFT_MINT, &BOB, 0, dst) + } + Some((_, dst)) => token_account_ext(&NFT_MINT, &BOB, 0, dst), + }, + 0, + false, + ), + acct(ALICE, Pubkey::default(), vec![], 0, false), + acct(metas, PROG, vec![0u8; 261], 0, false), + nft_pda.clone(), + acct(PORTFOLIO, PERCOLATOR_MAINNET, portfolio_buf(mint_auth.to_bytes()), 0, false), + acct(PERCOLATOR_MAINNET, Pubkey::default(), vec![], 0, false), + acct(mint_auth, Pubkey::default(), vec![], 0, false), + acct( + sysvar_instructions::ID, + Pubkey::default(), + build_sysvar(&top_prog, &top_data, &top_accounts), + 0, + false, + ), + acct(PROG, Pubkey::default(), vec![], 0, false), + acct(registry, PERCOLATOR_MAINNET, vec![], 0, false), + ]; + + let r = process_execute(&PROG, &accounts, 1); + (r, nft_pda) +} + +/// Drive `ReconcileBurnedNft` (tag 7) with `recipient` supplied as account 6. +/// Returns the result plus the recipient's lamports afterwards. +fn run_reconcile( + last_holder_in_state: [u8; 32], + recipient: Pubkey, +) -> (Result<(), ProgramError>, u64) { + let (mint_auth, _) = mint_authority_pda(&PROG); + let (nft_pda_key, bump) = position_nft_pda(&PORTFOLIO, MARKET_ID, &PROG); + let (registry, _) = derive_nft_registry(&PERCOLATOR_MAINNET, &MARKET_GROUP); + + let recipient_ai = acct(recipient, Pubkey::default(), vec![], 0, true); + + let accounts = vec![ + acct(nft_pda_key, PROG, nft_pda_buf(bump, last_holder_in_state), PDA_RENT, true), + // supply == 0: the NFT really was burned out of band + acct(NFT_MINT, TOKEN_2022_PROGRAM_ID, mint_account(0), 0, false), + acct(PORTFOLIO, PERCOLATOR_MAINNET, portfolio_buf(mint_auth.to_bytes()), 0, true), + acct(mint_auth, Pubkey::default(), vec![], 0, false), + acct(registry, PERCOLATOR_MAINNET, vec![], 0, false), + acct(PERCOLATOR_MAINNET, Pubkey::default(), vec![], 0, false), + recipient_ai.clone(), + ]; + + let r = processor::process(&PROG, &accounts, &[TAG_RECONCILE_BURNED_NFT]); + let got = **recipient_ai.lamports.borrow(); + (r, got) +} + +// -- 1. the finding ---------------------------------------------------------- + +#[test] +fn marketplace_sale_without_the_extension_degrades_to_prior_behaviour() { + // Alice minted, so last_holder == Alice (processor.rs:500). + // She now sells to Bob through a marketplace that moves the token by CPI, + // so the TOP-LEVEL instruction is the marketplace, not Token-2022. + let (r, nft_pda) = run_hook(MARKETPLACE, vec![7u8], vec![], ALICE.to_bytes(), None); + + assert!(r.is_ok(), "the sale itself must succeed (#145 composability): {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + ALICE.to_bytes(), + "last_holder still points at the SELLER after the sale settled", + ); + assert_ne!(read_last_holder(&nft_pda), BOB.to_bytes(), "the buyer was never recorded"); +} + +// -- 2. control: the harness DOES observe a write ---------------------------- + +#[test] +fn control_a_direct_transfer_does_record_the_buyer() { + // Identical in every respect except the top-level program. This is the A/B + // control: it proves the assertion above is a real skipped write, not a + // harness that silently fails before reaching the write. + let mut data = vec![12u8]; // TransferChecked + data.extend_from_slice(&1u64.to_le_bytes()); + data.push(0); // decimals + let top_accounts = vec![SRC_ATA, NFT_MINT, DST_ATA, ALICE]; + + let (r, nft_pda) = run_hook(TOKEN_2022_PROGRAM_ID, data, top_accounts, ALICE.to_bytes(), None); + + assert!(r.is_ok(), "genuine direct transfer must succeed: {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + BOB.to_bytes(), + "a genuine top-level Token-2022 transfer DOES record the buyer", + ); +} + +// -- 3+4. the impact: buyer locked out, seller paid -------------------------- + +#[test] +fn buyer_cannot_reconcile_his_own_burned_nft() { + // Bob owns the NFT and burns it out of band (#138). He tries to recover. + let (r, _) = run_reconcile(ALICE.to_bytes(), BOB); + assert!( + matches!(r, Err(ProgramError::Custom(c)) if c == 7), // NftError::NotNftHolder + "the actual owner is rejected by the last_holder gate, got {r:?}", + ); +} + +#[test] +fn seller_is_paid_the_escrowed_portfolio_and_the_rent() { + // Anyone may crank it; the escrow + rent go to the RECORDED last_holder. + let (r, alice_lamports) = run_reconcile(ALICE.to_bytes(), ALICE); + assert!(r.is_ok(), "reconcile to the stale seller succeeds: {r:?}"); + assert_eq!( + alice_lamports, PDA_RENT, + "PDA rent swept to the seller (the unwrap CPI likewise names her as new owner)", + ); +} + +#[test] +fn control_a_correctly_recorded_buyer_can_reconcile() { + // Same accounts, same burned mint, same everything as the rejection test -- + // only the recorded last_holder differs. This isolates the gate at + // processor.rs:1275 as the sole reason Bob was refused above. + let (r, bob_lamports) = run_reconcile(BOB.to_bytes(), BOB); + assert!(r.is_ok(), "with the buyer correctly recorded, he recovers: {r:?}"); + assert_eq!(bob_lamports, PDA_RENT); +} + +// -- 5. the fix --------------------------------------------------------------- + +#[test] +fn marketplace_sale_in_flight_records_the_buyer() { + // The same marketplace-CPI sale as above, except the source and destination + // now carry Token-2022's in-flight `transferring` flag -- exactly what the + // token program sets around `invoke_execute`. The buyer is now recorded. + let (r, nft_pda) = run_hook(MARKETPLACE, vec![7u8], vec![], ALICE.to_bytes(), Some((true, true))); + + assert!(r.is_ok(), "the marketplace sale must still succeed: {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + BOB.to_bytes(), + "the buyer who actually holds the NFT is now the recorded last_holder", + ); +} + +#[test] +fn buyer_recorded_by_the_fix_can_reconcile_and_seller_cannot() { + // The end-to-end consequence: run the sale, take the resulting state, and + // feed it to ReconcileBurnedNft. The roles are now the right way round. + let (_, nft_pda) = run_hook(MARKETPLACE, vec![7u8], vec![], ALICE.to_bytes(), Some((true, true))); + let recorded = read_last_holder(&nft_pda); + + let (bob_r, bob_lamports) = run_reconcile(recorded, BOB); + assert!(bob_r.is_ok(), "the real owner recovers his own position: {bob_r:?}"); + assert_eq!(bob_lamports, PDA_RENT); + + let (alice_r, _) = run_reconcile(recorded, ALICE); + assert!( + matches!(alice_r, Err(ProgramError::Custom(c)) if c == 7), + "the paid-out seller is now refused, got {alice_r:?}", + ); +} + +// -- 6. #152/#153 must stay closed ------------------------------------------- + +#[test] +fn spoofed_direct_execute_still_cannot_forge_last_holder() { + // The #152/#153 attack: an attacker invokes Execute top-level with a + // dest_ata they own. No transfer is in flight, so both flags read false and + // the write must still be suppressed. + let (r, nft_pda) = run_hook(MARKETPLACE, vec![7u8], vec![], ALICE.to_bytes(), Some((false, false))); + + assert!(r.is_ok(), "gates pass, as before: {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + ALICE.to_bytes(), + "a spoofed Execute cannot forge last_holder", + ); +} + +#[test] +fn a_half_set_flag_pair_is_not_enough() { + // Both source and destination must be in flight. Token-2022 always sets the + // pair together, so a single flag is not a real transfer window. + for (src, dst) in [(true, false), (false, true)] { + let (r, nft_pda) = + run_hook(MARKETPLACE, vec![7u8], vec![], ALICE.to_bytes(), Some((src, dst))); + assert!(r.is_ok(), "gates still pass for ({src},{dst}): {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + ALICE.to_bytes(), + "half-set pair ({src},{dst}) must not authorise the write", + ); + } +} + +#[test] +fn a_non_account_type_tlv_is_not_honoured() { + // TransferHookAccount is account-scoped. A buffer whose account_type byte is + // not `Account` must not have its TLV honoured, even with the flag byte set. + let (mint_auth, _) = mint_authority_pda(&PROG); + let (nft_pda_key, bump) = position_nft_pda(&PORTFOLIO, MARKET_ID, &PROG); + let (metas, _) = extra_account_metas_pda(&NFT_MINT, &PROG); + let (registry, _) = derive_nft_registry(&PERCOLATOR_MAINNET, &MARKET_GROUP); + + let nft_pda = acct(nft_pda_key, PROG, nft_pda_buf(bump, ALICE.to_bytes()), PDA_RENT, true); + let accounts = vec![ + acct( + SRC_ATA, + TOKEN_2022_PROGRAM_ID, + token_account_ext_typed(&NFT_MINT, &ALICE, 1, true, 1), // 1 = Mint, not Account + 0, + false, + ), + acct(NFT_MINT, TOKEN_2022_PROGRAM_ID, mint_account(1), 0, false), + acct( + DST_ATA, + TOKEN_2022_PROGRAM_ID, + token_account_ext_typed(&NFT_MINT, &BOB, 0, true, 1), + 0, + false, + ), + acct(ALICE, Pubkey::default(), vec![], 0, false), + acct(metas, PROG, vec![0u8; 261], 0, false), + nft_pda.clone(), + acct(PORTFOLIO, PERCOLATOR_MAINNET, portfolio_buf(mint_auth.to_bytes()), 0, false), + acct(PERCOLATOR_MAINNET, Pubkey::default(), vec![], 0, false), + acct(mint_auth, Pubkey::default(), vec![], 0, false), + acct( + sysvar_instructions::ID, + Pubkey::default(), + build_sysvar(&MARKETPLACE, &[7u8], &[]), + 0, + false, + ), + acct(PROG, Pubkey::default(), vec![], 0, false), + acct(registry, PERCOLATOR_MAINNET, vec![], 0, false), + ]; + + let r = process_execute(&PROG, &accounts, 1); + assert!(r.is_ok(), "gates still pass: {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + ALICE.to_bytes(), + "a non-Account account_type must not authorise the write", + ); +} + +#[test] +fn the_real_on_chain_ata_layout_is_parsed_correctly() { + // ImmutableOwner (type 7, len 0) precedes TransferHookAccount in every real + // ATA. The zero-length entry must be walked over, not tripped on. + let (r, nft_pda) = run_hook_inner( + MARKETPLACE, + vec![7u8], + vec![], + ALICE.to_bytes(), + Some((true, true)), + true, + ); + assert!(r.is_ok(), "marketplace sale must succeed: {r:?}"); + assert_eq!( + read_last_holder(&nft_pda), + BOB.to_bytes(), + "the buyer must be recorded through a realistic multi-extension TLV", + ); + + // ...and the same layout with the flag clear must NOT authorise the write. + let (r2, pda2) = run_hook_inner( + MARKETPLACE, + vec![7u8], + vec![], + ALICE.to_bytes(), + Some((false, false)), + true, + ); + assert!(r2.is_ok()); + assert_eq!(read_last_holder(&pda2), ALICE.to_bytes()); +}