From b7000efc050308661b898a35c7dbc93bf3f9d85e Mon Sep 17 00:00:00 2001 From: Rigidity Date: Sun, 12 Jul 2026 02:14:59 +0200 Subject: [PATCH] Fix bug in offer implementation --- crates/sage-wallet/src/wallet.rs | 14 +- crates/sage-wallet/src/wallet/offer.rs | 225 ++++++++++++------ .../src/wallet/offer/take_offer.rs | 27 ++- crates/sage/src/endpoints/offers.rs | 13 +- 4 files changed, 194 insertions(+), 85 deletions(-) diff --git a/crates/sage-wallet/src/wallet.rs b/crates/sage-wallet/src/wallet.rs index 884ce3f67..d385e436e 100644 --- a/crates/sage-wallet/src/wallet.rs +++ b/crates/sage-wallet/src/wallet.rs @@ -190,6 +190,17 @@ impl Wallet { ctx: &mut SpendContext, spends: &mut Spends, actions: &[Action], + ) -> Result<(), WalletError> { + self.select_spends_excluding(ctx, spends, actions, &[]) + .await + } + + pub async fn select_spends_excluding( + &self, + ctx: &mut SpendContext, + spends: &mut Spends, + actions: &[Action], + excluded_coin_ids: &[Bytes32], ) -> Result<(), WalletError> { let mut deltas = Deltas::from_actions(actions); @@ -199,8 +210,9 @@ impl Wallet { deltas.update(id).input += cat.selected_amount(); } - let selected_coin_ids: HashSet = + let mut selected_coin_ids: HashSet = spends.non_settlement_coin_ids().into_iter().collect(); + selected_coin_ids.extend(excluded_coin_ids); for &id in deltas.ids() { let delta = deltas.get(&id).copied().unwrap_or_default(); diff --git a/crates/sage-wallet/src/wallet/offer.rs b/crates/sage-wallet/src/wallet/offer.rs index 79117cde0..f76319e43 100644 --- a/crates/sage-wallet/src/wallet/offer.rs +++ b/crates/sage-wallet/src/wallet/offer.rs @@ -6,6 +6,7 @@ mod take_offer; pub use aggregate_offer::*; pub use make_offer::*; +pub use take_offer::*; #[cfg(test)] mod tests { @@ -14,10 +15,33 @@ mod tests { use sage_database::NftOfferInfo; use test_log::test; - use crate::{Offered, Requested, RequestedCat, TestWallet, WalletNftMint}; + use crate::{ + Offered, Requested, RequestedCat, TakenOffer, TestWallet, WalletError, WalletNftMint, + }; use super::aggregate_offers; + async fn sign_taken_offer( + wallet: &TestWallet, + taken: TakenOffer, + ) -> anyhow::Result { + let TakenOffer { + offer, + spend_bundle, + } = taken; + let spend_bundle = wallet + .wallet + .sign_transaction( + spend_bundle, + &wallet.agg_sig, + wallet.master_sk.clone(), + false, + ) + .await?; + + Ok(offer.take(spend_bundle)) + } + #[test(tokio::test)] async fn test_offer_xch_for_cat() -> anyhow::Result<()> { let mut alice = TestWallet::new(1000).await?; @@ -51,10 +75,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -128,10 +149,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -194,10 +212,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -272,10 +287,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -362,10 +374,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -447,10 +456,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -552,10 +558,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -633,10 +636,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; @@ -713,10 +713,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; @@ -785,10 +782,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; @@ -825,10 +819,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -840,6 +831,110 @@ mod tests { Ok(()) } + #[test(tokio::test)] + async fn test_take_offer_excludes_inputs() -> anyhow::Result<()> { + let alice = TestWallet::new(1000).await?; + + // Simulate taking our own deleted offer. + let offer = alice + .wallet + .make_offer( + Offered { + xch: 100, + ..Default::default() + }, + Requested { + xch: 1000, + ..Default::default() + }, + None, + ) + .await?; + let offer = alice + .wallet + .sign_transaction(offer, &alice.agg_sig, alice.master_sk.clone(), true) + .await?; + + let result = alice.wallet.take_offer(offer, 0).await; + assert!( + matches!(result, Err(WalletError::CoinSelection(_))), + "taking the offer should not reuse the offer input coin" + ); + + Ok(()) + } + + #[test(tokio::test)] + async fn test_take_offer_skips_injected_spend() -> anyhow::Result<()> { + let alice = TestWallet::new(1000).await?; + let mut bob = alice.next(1001).await?; + + // Keep the injected spend distinct from the offer. + let coins = bob.wallet.db.selectable_xch_coins().await?; + let coin_spends = bob + .wallet + .split(coins.iter().map(Coin::coin_id).collect(), 2, 0) + .await?; + bob.transact(coin_spends).await?; + bob.wait_for_coins().await; + + let mut coins = bob.wallet.db.selectable_xch_coins().await?; + coins.sort_by_key(|coin| coin.amount); + let malicious_coin = coins[1]; + + // Attacker-supplied unsigned spend hidden in the offer. + let mut ctx = SpendContext::new(); + bob.wallet + .spend( + &mut ctx, + vec![malicious_coin.coin_id()], + &[Action::send( + Id::Xch, + alice.puzzle_hash, + malicious_coin.amount, + Memos::None, + )], + ) + .await?; + let malicious_spends = ctx.take(); + assert_eq!(malicious_spends.len(), 1); + + // Bob needs no XCH input to take this offer. + let offer = alice + .wallet + .make_offer( + Offered { + xch: 100, + ..Default::default() + }, + Requested::default(), + None, + ) + .await?; + let mut offer = alice + .wallet + .sign_transaction(offer, &alice.agg_sig, alice.master_sk.clone(), true) + .await?; + + offer.coin_spends.extend(malicious_spends); + + let taken = bob.wallet.take_offer(offer, 0).await?; + assert!( + !taken + .spend_bundle + .coin_spends + .iter() + .any(|coin_spend| coin_spend.coin.coin_id() == malicious_coin.coin_id()), + "the injected spend must not be signed" + ); + + let spend_bundle = sign_taken_offer(&bob, taken).await?; + let result = bob.sim.lock().await.new_transaction(spend_bundle); + assert!(result.is_err()); + + Ok(()) + } + #[test(tokio::test)] async fn test_offer_cat_single_sided() -> anyhow::Result<()> { let mut alice = TestWallet::new(1000).await?; @@ -868,10 +963,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; // We need to wait for both wallets to sync in this case @@ -929,10 +1021,7 @@ mod tests { // Take offer let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; assert_eq!(spend_bundle.coin_spends.len(), 3); bob.push_bundle(spend_bundle).await?; @@ -1076,10 +1165,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; @@ -1185,10 +1271,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; @@ -1284,10 +1367,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; @@ -1386,10 +1466,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; @@ -1513,10 +1590,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; @@ -1629,10 +1703,7 @@ mod tests { ) .await?; let offer = bob.wallet.take_offer(offer, 0).await?; - let spend_bundle = bob - .wallet - .sign_transaction(offer, &bob.agg_sig, bob.master_sk.clone(), true) - .await?; + let spend_bundle = sign_taken_offer(&bob, offer).await?; bob.push_bundle(spend_bundle).await?; bob.wait_for_coins().await; alice.wait_for_puzzles().await; diff --git a/crates/sage-wallet/src/wallet/offer/take_offer.rs b/crates/sage-wallet/src/wallet/offer/take_offer.rs index 1fc8e7944..05808907c 100644 --- a/crates/sage-wallet/src/wallet/offer/take_offer.rs +++ b/crates/sage-wallet/src/wallet/offer/take_offer.rs @@ -11,12 +11,18 @@ use sage_database::NftOfferInfo; use crate::{Wallet, WalletError}; +#[derive(Debug)] +pub struct TakenOffer { + pub offer: Offer, + pub spend_bundle: SpendBundle, +} + impl Wallet { pub async fn take_offer( &self, spend_bundle: SpendBundle, fee: u64, - ) -> Result { + ) -> Result { let mut ctx = SpendContext::new(); let offer = Offer::from_spend_bundle(&mut ctx, &spend_bundle)?; @@ -83,7 +89,19 @@ impl Wallet { actions.extend(offer.requested_payments().actions()); // Add requested payments - self.select_spends(&mut ctx, &mut spends, &actions).await?; + let offer_input_coin_ids = offer + .spend_bundle() + .coin_spends + .iter() + .map(|coin_spend| coin_spend.coin.coin_id()) + .collect_vec(); + self.select_spends_excluding( + &mut ctx, + &mut spends, + &actions, + &offer_input_coin_ids, + ) + .await?; // Reset DIDs and reveal trade prices let mut royalty_nft_count = 0; @@ -134,6 +152,9 @@ impl Wallet { self.complete_spends(&mut ctx, &deltas, spends).await?; - Ok(offer.take(SpendBundle::new(ctx.take(), Signature::default()))) + Ok(TakenOffer { + offer, + spend_bundle: SpendBundle::new(ctx.take(), Signature::default()), + }) } } diff --git a/crates/sage/src/endpoints/offers.rs b/crates/sage/src/endpoints/offers.rs index 974898e5c..20fc81284 100644 --- a/crates/sage/src/endpoints/offers.rs +++ b/crates/sage/src/endpoints/offers.rs @@ -15,7 +15,7 @@ use sage_api::{ use sage_assets::fetch_uris_with_hash; use sage_database::{AssetKind, OfferRow, OfferStatus, OfferedAsset}; use sage_wallet::{ - Offered, Requested, RequestedCat, SyncCommand, Transaction, Wallet, WalletError, + Offered, Requested, RequestedCat, SyncCommand, TakenOffer, Transaction, Wallet, WalletError, aggregate_offers, insert_transaction, sort_offer, }; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -202,7 +202,7 @@ impl Sage { let offer = decode_offer(&req.offer)?; let fee = parse_amount(req.fee)?; - let unsigned = wallet.take_offer(offer, fee).await?; + let taken = wallet.take_offer(offer, fee).await?; let (_mnemonic, Some(master_sk)) = self.keychain.extract_secrets(wallet.fingerprint, b"")? @@ -210,14 +210,19 @@ impl Sage { return Err(Error::NoSigningKey); }; + let TakenOffer { + offer, + spend_bundle, + } = taken; let spend_bundle = wallet .sign_transaction( - unsigned, + spend_bundle, &AggSigConstants::new(self.network().agg_sig_me()), master_sk, - true, + false, ) .await?; + let spend_bundle = offer.take(spend_bundle); debug!( "{}",