## Description There is a discrepancy between the `user_price` check in the `shared_accounts.rs::calculate_fees()` function and the actual transaction amount. To illustrate this, let's examine the `lib.rs::buy_nft()` function: ```rust // lib.rs::buy_nft() pub fn buy_nft<'info>( ctx: Context<'_, '_, '_, 'info, BuyNft<'info>>, @> max_amount: u64, authorization_data: Option, optional_royalty_pct: Option, ) -> Result<()> { instructions::legacy::buy_nft::process_buy_nft( ctx, max_amount, authorization_data, optional_royalty_pct, ) } // buy_nfs.rs::process_buy_nft() pub fn process_buy_nft<'info>( ctx: Context<'_, '_, '_, 'info, BuyNft<'info>>, @> max_amount: u64, authorization_data: Option, optional_royalty_pct: Option, ) -> Result<()> { // Pre-handler validation checks. let asset = ctx.accounts.pre_process_checks()?; let taker = ctx.accounts.trade.taker.to_account_info(); let owner = ctx.accounts.trade.owner.to_account_info(); let fees = ctx.accounts.trade.calculate_fees( asset.seller_fee_basis_points, @> max_amount, TakerSide::Buy, if asset.royalty_enforced { Some(100) } else { optional_royalty_pct }, )?; // SNIP... @> ctx.accounts .trade .pay_buyer_fees(asset, fees, ctx.remaining_accounts)?; // SNIP... } ``` The `max_amount` parameter represents the maximum transaction amount accepted when the user calls `lib.rs::buy_nft()`, which is then utilized in the `calculate_fees()` function for price validation. In the `TakerSide::Buy` branch, the price used for verification is calculated as follows: ```rust // shared_accounts.rs::calculate_fees() pub fn calculate_fees( &self, seller_fee_basis_points: u16, @> user_price: u64, taker_side: TakerSide, royalty_pct: Option, ) -> Result { // SNIP... match taker_side { TakerSide::Buy => { // Check that the price + royalties + mm_fee doesn't exceed the max amount the user specified to prevent sandwich attacks. @> let price = unwrap_checked!({ current_price.checked_add(mm_fee)?.checked_add(creators_fee) }); @> if price > user_price { throw_err!(ErrorCode::PriceMismatch); } } TakerSide::Sell => { // Check that the total price the seller receives isn't lower than the min price the user specified. @> let price = unwrap_checked!({ current_price.checked_sub(mm_fee)?.checked_sub(creators_fee) }); @> if price < user_price { throw_err!(ErrorCode::PriceMismatch); } } } Ok(Fees { taker_fee, tamm_fee, maker_broker_fee, taker_broker_fee, creators_fee, }) } ``` However, when executing `pay_buyer_fees()` to process the payment, as indicated by the `@>` marks, the total fee paid by the `taker` is represented by: ```makefile buy_price = current_price + taker_fee + mm_fee + creators_fee ``` This is further detailed as: ```makefile buy_price = current_price + tamm_fee + maker_broker_fee + taker_broker_fee + mm_fee + creators_fee ``` As a result, the actual payment amount made by the user exceeds max_amount upon transaction completion. ```rust // shared_accounts.rs::pay_buyer_fees() pub fn pay_buyer_fees( &self, amm_asset: AmmAsset, fees: Fees, creator_accounts: &[AccountInfo<'info>], ) -> Result<()> { let Fees { taker_fee: _, tamm_fee, maker_broker_fee, taker_broker_fee, creators_fee, } = fees; let pool = &self.pool; let current_price = pool.current_price(TakerSide::Buy)?; let mm_fee = pool.calc_mm_fee(current_price)?; /* **Transfer Fees** The buy price is the total price the buyer pays for buying the NFT from the pool. buy_price = current_price + taker_fee + mm_fee + creators_fee taker_fee = tamm_fee + broker_fee + maker_rebate Fees are paid by individual transfers as they go to various accounts depending on the pool type and configuration. */ // Determine the SOL destination: owner, pool, or shared escrow account. //(!) this block has to come before royalties transfer due to remaining_accounts let destination = match pool.config.pool_type { // Send money direct to seller/owner PoolType::NFT => self.owner.to_account_info(), // Send money to the pool PoolType::Trade => { if pool.shared_escrow != Pubkey::default() { let incoming_shared_escrow = unwrap_opt!(self.shared_escrow.as_ref(), ErrorCode::BadSharedEscrow) .to_account_info(); // NB: we don't explicitly assert for presence and decode the escrow account. // This is because a pool owner can close the escrow and DOS buying from their pool. // We already asserted the shared_escrow was a valid MarginAccount during creator. if incoming_shared_escrow.key != &pool.shared_escrow { throw_err!(ErrorCode::BadSharedEscrow); } incoming_shared_escrow } else { self.pool.to_account_info() } } PoolType::Token => unreachable!(), }; // Buyer is the taker and pays the taker fee: tamm_fee + maker_broker_fee + taker_broker_fee. @>1 transfer_lamports(&self.taker, &self.fee_vault, tamm_fee)?; @>2 transfer_lamports_checked( &self.taker, self.maker_broker .as_ref() .map(|acc| acc.to_account_info()) .as_ref() .unwrap_or(&self.fee_vault), maker_broker_fee, )?; @>3 transfer_lamports_checked( &self.taker, self.taker_broker .as_ref() .map(|acc| acc.to_account_info()) .as_ref() .unwrap_or(&self.fee_vault), taker_broker_fee, )?; // transfer royalties (on top of current price) // Buyer pays the royalty fee. @>4 transfer_creators_fee( &amm_asset .royalty_creators .clone() .unwrap_or_default() .into_iter() .map(Into::into) .collect(), &mut creator_accounts.iter(), creators_fee, &CreatorFeeMode::Sol { from: &FromAcc::External(&FromExternal { from: &self.taker, sys_prog: &self.native_program, }), }, )?; // Price always goes to the destination: NFT pool --> owner, Trade pool --> either the pool or the escrow account. @>5 transfer_lamports(&self.taker, &destination, current_price)?; // Trade pools need to check compounding fees @>6 if matches!(pool.config.pool_type, PoolType::Trade) { // If MM fees are compounded they go to the destination to remain // in the pool or escrow. if pool.config.mm_compound_fees { transfer_lamports(&self.taker, &destination, mm_fee)?; } else { // If MM fees are not compounded they go to the owner. transfer_lamports(&self.taker, &self.owner, mm_fee)?; } } Ok(()) } ``` Since the `calculate_fees()` function does not account for `tamm_fee`, `broker_fee`, and `maker_rebate` in the price verification, the actual payment amount in the `TakerSide::Buy` branch will exceed expectations, whereas in the `TakerSide::Sell` branch, it will fall short. ## Proof of Concept Please add the following test code to the `amm/clients/js/test/legacy/buy.test.ts` file and execute: ```js test('The buyer actual spending amount is greater than maxAmount', async (t) => { const { client, signers, nft, testConfig, pool } = await setupLegacyTest({ t, poolType: PoolType.NFT, action: TestAction.Buy, useMakerBroker: true, }); const { buyer, poolOwner, nftUpdateAuthority, makerBroker } = signers; // To facilitate testing, update `maxAmount=100000000n` to make it smaller than the actual payment amount of the buyer. testConfig.price = 100000000n; const { price: maxAmount } = testConfig; const { mint } = nft; // Record initial balance of the buyer const initialBuyerBalance = ( await client.rpc.getBalance(buyer.address).send() ).value; // Buy NFT from pool const buyNftIx = await getBuyNftInstructionAsync({ owner: poolOwner.address, taker: buyer, pool, mint, maxAmount, makerBroker: makerBroker.address, // Remaining accounts creators: [nftUpdateAuthority.address], }); const sig = await pipe( await createDefaultTransaction(client, buyer), (tx) => appendTransactionMessageInstruction(buyNftIx, tx), (tx) => signAndSendTransaction(client, tx) ); // Record final balance of the buyer const finalBuyerBalance = ( await client.rpc.getBalance(buyer.address).send() ).value; // Get the actual amount spent by the buyer let actualSpentAmount = initialBuyerBalance - finalBuyerBalance; // Assert that the buyer's actual spending amount is greater than maxAmount const assert = require('assert'); assert.ok(actualSpentAmount > maxAmount, 'Actual spent amount is greater than maxAmount'); // NFT is now owned by the buyer. await assertTokenNftOwnedBy({ t, client, mint, owner: buyer.address, }); assertTammNoop(t, client, sig); }); // ✔ legacy › buy › The buyer actual spending amount is greater than maxAmount ``` ## Recommendation To address this discrepancy, it is recommended to incorporate the `taker_fee` into the price calculation: ```diff pub fn calculate_fees( &self, seller_fee_basis_points: u16, user_price: u64, taker_side: TakerSide, royalty_pct: Option, ) -> Result { let pool = &self.pool; // SNIP... match taker_side { TakerSide::Buy => { // Check that the price + royalties + mm_fee doesn't exceed the max amount the user specified to prevent sandwich attacks. - let price = unwrap_checked!({ - current_price.checked_add(mm_fee)?.checked_add(creators_fee) + let price = unwrap_checked!({ + current_price.checked_add(mm_fee)?.checked_add(creators_fee)?.checked_add(taker_fee) }); if price > user_price { throw_err!(ErrorCode::PriceMismatch); } } TakerSide::Sell => { // Check that the total price the seller receives isn't lower than the min price the user specified. - let price = unwrap_checked!({ - current_price.checked_sub(mm_fee)?.checked_sub(creators_fee) + let price = unwrap_checked!({ + current_price.checked_sub(mm_fee)?.checked_sub(creators_fee)?.checked_sub(taker_fee) }); if price < user_price { throw_err!(ErrorCode::PriceMismatch); } } } Ok(Fees { taker_fee, tamm_fee, maker_broker_fee, taker_broker_fee, creators_fee, }) } ``` test,again ```zsh ✘ [fail]: legacy › buy › The buyer actual spending amount is greater than maxAmount Rejected promise returned by test ```

Contest	Date	Submissions	Payout
Infrared Finance	Jan 15, 2025	1	$122
LI.FI	Jan 13, 2025	3	$750
0xhoneyjar	Dec 4, 2024	1	$98
Sorella Labs	Nov 11, 2024	1	$0
Tensor	Oct 16, 2024	3	$2,058
InfinityPools	Sep 30, 2024	4	$5,640

joicygiore

Public reviews