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.

Author: nymius

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"),
         }
     }
 }

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(