From 3acf109dfbae1f71ab63c9708be81d453721d49d Mon Sep 17 00:00:00 2001 From: nymius <155548262+nymius@users.noreply.github.com> Date: Wed, 5 Feb 2025 16:44:18 -0300 Subject: [PATCH] fix(tx_builder)!: check foreign utxos are not local before inclusion The new check is added to ensure all added foreign utxos are not owned by the wallet. --- crates/wallet/src/wallet/tx_builder.rs | 43 +++++---- crates/wallet/tests/wallet.rs | 127 +++++++++++++++++++++++-- 2 files changed, 142 insertions(+), 28 deletions(-) diff --git a/crates/wallet/src/wallet/tx_builder.rs b/crates/wallet/src/wallet/tx_builder.rs index 235987fc0..bf5097d47 100644 --- a/crates/wallet/src/wallet/tx_builder.rs +++ b/crates/wallet/src/wallet/tx_builder.rs @@ -351,8 +351,9 @@ impl<'a, Cs> TxBuilder<'a, Cs> { /// /// This method returns errors in the following circumstances: /// - /// 1. The `psbt_input` does not contain a `witness_utxo` or `non_witness_utxo`. - /// 2. The data in `non_witness_utxo` does not match what is in `outpoint`. + /// 1. The provided outpoint is associated to a [`LocalOutput`]. + /// 2. The `psbt_input` does not contain a `witness_utxo` or `non_witness_utxo`. + /// 3. The data in `non_witness_utxo` does not match what is in `outpoint`. /// /// Note unless you set [`only_witness_utxo`] any non-taproot `psbt_input` you pass to this /// method must have `non_witness_utxo` set otherwise you will get an error when [`finish`] @@ -383,24 +384,27 @@ impl<'a, Cs> TxBuilder<'a, Cs> { satisfaction_weight: Weight, sequence: Sequence, ) -> Result<&mut Self, AddForeignUtxoError> { - if psbt_input.witness_utxo.is_none() { - match psbt_input.non_witness_utxo.as_ref() { - Some(tx) => { - if tx.compute_txid() != outpoint.txid { - return Err(AddForeignUtxoError::InvalidTxid { - input_txid: tx.compute_txid(), - foreign_utxo: outpoint, - }); - } - if tx.output.len() <= outpoint.vout as usize { - return Err(AddForeignUtxoError::InvalidOutpoint(outpoint)); - } - } - None => { - return Err(AddForeignUtxoError::MissingUtxo); + let txout = match (&psbt_input.witness_utxo, &psbt_input.non_witness_utxo) { + (Some(ref txout), _) => Ok(txout.clone()), + (_, Some(tx)) => { + if tx.compute_txid() != outpoint.txid { + Err(AddForeignUtxoError::InvalidTxid { + input_txid: tx.compute_txid(), + foreign_utxo: outpoint, + }) + } else { + tx.tx_out(outpoint.vout as usize) + .map_err(|_| AddForeignUtxoError::InvalidOutpoint(outpoint)) + .cloned() } } - } + (_, _) => Err(AddForeignUtxoError::MissingUtxo), + }?; + + // Avoid the inclusion of local utxos as foreign utxos + if self.wallet.is_mine(txout.script_pubkey) { + return Err(AddForeignUtxoError::NotForeignUtxo); + }; if let Some(WeightedUtxo { utxo: Utxo::Local { .. }, @@ -731,6 +735,8 @@ pub enum AddForeignUtxoError { InvalidOutpoint(OutPoint), /// Foreign utxo missing witness_utxo or non_witness_utxo MissingUtxo, + /// UTxO is owned by wallet + NotForeignUtxo, } impl fmt::Display for AddForeignUtxoError { @@ -750,6 +756,7 @@ impl fmt::Display for AddForeignUtxoError { outpoint.txid, outpoint.vout, ), Self::MissingUtxo => write!(f, "Foreign utxo missing witness_utxo or non_witness_utxo"), + Self::NotForeignUtxo => write!(f, "UTxO is owned by wallet"), } } } diff --git a/crates/wallet/tests/wallet.rs b/crates/wallet/tests/wallet.rs index f42f0bcd5..e02c9bfd9 100644 --- a/crates/wallet/tests/wallet.rs +++ b/crates/wallet/tests/wallet.rs @@ -1644,18 +1644,121 @@ fn test_calculate_fee_with_missing_foreign_utxo() { } #[test] -fn test_add_foreign_utxo_invalid_psbt_input() { +fn test_add_foreign_utxo_fails_without_psbt_input_prev_tx_data() { let (mut wallet, _) = get_funded_wallet_wpkh(); - let outpoint = wallet.list_unspent().next().expect("must exist").outpoint; + let address = Address::from_str("bcrt1q3qtze4ys45tgdvguj66zrk4fu6hq3a3v9pfly5") + .expect("address") + .require_network(Network::Regtest) + .unwrap(); + let tx0 = Transaction { + output: vec![TxOut { + value: Amount::from_sat(76_000), + script_pubkey: address.script_pubkey(), + }], + ..new_tx(0) + }; + let external_outpoint = OutPoint { + txid: tx0.compute_txid(), + vout: 0, + }; let foreign_utxo_satisfaction = wallet .public_descriptor(KeychainKind::External) .max_weight_to_satisfy() .unwrap(); let mut builder = wallet.build_tx(); - let result = - builder.add_foreign_utxo(outpoint, psbt::Input::default(), foreign_utxo_satisfaction); - assert!(matches!(result, Err(AddForeignUtxoError::MissingUtxo))); + let result = builder.add_foreign_utxo( + external_outpoint, + psbt::Input::default(), + foreign_utxo_satisfaction, + ); + assert!( + matches!(result, Err(AddForeignUtxoError::MissingUtxo)), + "should fail when there is no data about the transaction creating the input" + ); +} + +#[test] +fn test_add_foreign_utxo_fails_when_utxo_is_owned_by_wallet_with_prev_txout() { + let (mut wallet1, _) = + get_funded_wallet_single("wpkh(cVbZ8ovhye9AoAHFsqobCf7LxbXDAECy9Kb8TZdfsDYMZGBUyCnm)"); + + let utxo = wallet1.list_unspent().next().expect("must take!"); + let foreign_utxo_satisfaction = wallet1 + .public_descriptor(KeychainKind::External) + .max_weight_to_satisfy() + .unwrap(); + + let psbt_input = psbt::Input { + // Add txout + witness_utxo: Some(utxo.txout.clone()), + ..Default::default() + }; + + let mut builder = wallet1.build_tx(); + let result = builder.add_foreign_utxo(utxo.outpoint, psbt_input, foreign_utxo_satisfaction); + assert!( + matches!(result, Err(AddForeignUtxoError::NotForeignUtxo)), + "should fail when the outpoint provided is not foreign to the wallet" + ); +} + +#[test] +fn test_add_foreign_utxo_fails_when_utxo_is_owned_by_wallet_with_full_prev_tx() { + let (mut wallet1, txid1) = + get_funded_wallet_single("wpkh(cVbZ8ovhye9AoAHFsqobCf7LxbXDAECy9Kb8TZdfsDYMZGBUyCnm)"); + + let utxo = wallet1.list_unspent().next().expect("must take!"); + let full_tx = wallet1.get_tx(txid1).unwrap().tx_node.tx.clone(); + let foreign_utxo_satisfaction = wallet1 + .public_descriptor(KeychainKind::External) + .max_weight_to_satisfy() + .unwrap(); + + let psbt_input = psbt::Input { + // Add full transaction + non_witness_utxo: Some(full_tx.as_ref().clone()), + ..Default::default() + }; + + let mut builder = wallet1.build_tx(); + let result = builder.add_foreign_utxo(utxo.outpoint, psbt_input, foreign_utxo_satisfaction); + assert!( + matches!(result, Err(AddForeignUtxoError::NotForeignUtxo)), + "should fail when the outpoint provided is not foreign to the wallet" + ); +} + +#[test] +fn test_add_foreign_utxo_fails_with_invalid_outpoint_vout() { + let (mut wallet1, txid1) = + get_funded_wallet_single("wpkh(cVbZ8ovhye9AoAHFsqobCf7LxbXDAECy9Kb8TZdfsDYMZGBUyCnm)"); + + let utxo = wallet1.list_unspent().next().expect("must take!"); + let full_tx = wallet1.get_tx(txid1).unwrap().tx_node.tx.clone(); + let foreign_utxo_satisfaction = wallet1 + .public_descriptor(KeychainKind::External) + .max_weight_to_satisfy() + .unwrap(); + + let psbt_input = psbt::Input { + non_witness_utxo: Some(full_tx.as_ref().clone()), + ..Default::default() + }; + + let mut builder = wallet1.build_tx(); + let result = builder.add_foreign_utxo( + OutPoint { + vout: full_tx.output.len() as u32 + 1, + txid: utxo.outpoint.txid, + }, + psbt_input, + foreign_utxo_satisfaction, + ); + assert!( + matches!(result, Err(AddForeignUtxoError::InvalidOutpoint { .. })), + "should fail when the outpoint provided index is not one of the possible transaction vouts" + ); } #[test] @@ -1674,19 +1777,23 @@ fn test_add_foreign_utxo_where_outpoint_doesnt_match_psbt_input() { .unwrap(); let mut builder = wallet1.build_tx(); + + // Check we are activating the code path that should rise the error assert!( - builder - .add_foreign_utxo( + matches!( + builder.add_foreign_utxo( utxo2.outpoint, psbt::Input { non_witness_utxo: Some(tx1.as_ref().clone()), ..Default::default() }, satisfaction_weight - ) - .is_err(), - "should fail when outpoint doesn't match psbt_input" + ), + Err(AddForeignUtxoError::InvalidTxid { .. }) + ), + "should fail when outpoint txid doesn't match psbt_input txout txid" ); + assert!( builder .add_foreign_utxo(