Skip to content

Commit 56cbb24

Browse files
icotaicota
authored
Merge pull request #120 from Foundation-Devices/darijo/env-2969-p2-01-do-not-spend-utxo-locks-can-be-bypassed-by-selected
fix(P2-01): enforce do_not_spend on explicit UTXO selections
2 parents 5fe47da + cdd19c3 commit 56cbb24

3 files changed

Lines changed: 218 additions & 29 deletions

File tree

‎src/rbf.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
use crate::account::NgAccount;
2+
#[cfg(feature = "envoy")]
3+
use crate::fee_rate::FeeRateSatPerKvb;
4+
use crate::fee_rate::FeeRateSatPerKwu;
25
use crate::ngwallet::NgWallet;
36
use crate::rbf::BumpFeeError::ComposeTxError;
47
use crate::send::DraftTransaction;
5-
use crate::fee_rate::FeeRateSatPerKwu;
6-
#[cfg(feature = "envoy")]
7-
use crate::fee_rate::FeeRateSatPerKvb;
88
#[cfg(feature = "envoy")]
99
use crate::send::TransactionFeeResult;
1010
use crate::transaction::{BitcoinTransaction, Input, KeyChain, Output};
@@ -44,6 +44,7 @@ pub enum BumpFeeError {
4444
FeeRateUnavailable,
4545
UnableToAccessWallet,
4646
UnableToAddForeignUtxo(AddForeignUtxoError),
47+
LockedUtxoSelected(Vec<String>),
4748
}
4849

4950
// TODO: chore: cleanup duplicate code
@@ -155,7 +156,8 @@ impl<P: WalletPersister> NgAccount<P> {
155156
max_fee = Some(required.to_sat());
156157
}
157158
CreateTxError::FeeRateTooLow { required } => {
158-
max_fee_rate = FeeRateSatPerKwu::from_bdk(required) + FeeRateSatPerKwu::from_sat_per_vb(1);
159+
max_fee_rate = FeeRateSatPerKwu::from_bdk(required)
160+
+ FeeRateSatPerKwu::from_sat_per_vb(1);
159161
max_fee = None;
160162
}
161163
CoinSelection(error) => {
@@ -200,7 +202,8 @@ impl<P: WalletPersister> NgAccount<P> {
200202
max_fee = Some(required.to_sat());
201203
}
202204
CreateTxError::FeeRateTooLow { required } => {
203-
max_fee_rate = FeeRateSatPerKwu::from_bdk(required) + FeeRateSatPerKwu::from_sat_per_vb(1);
205+
max_fee_rate = FeeRateSatPerKwu::from_bdk(required)
206+
+ FeeRateSatPerKwu::from_sat_per_vb(1);
204207
max_fee = None;
205208
}
206209
CoinSelection(error) => {
@@ -439,7 +442,8 @@ impl<P: WalletPersister> NgAccount<P> {
439442
mature_utxos.clone(),
440443
&mut do_not_spend_utxos,
441444
&mut spendables,
442-
);
445+
)
446+
.map_err(BumpFeeError::LockedUtxoSelected)?;
443447

444448
for do_not_spend_utxo in do_not_spend_utxos.clone() {
445449
tx_builder.add_unspendable(do_not_spend_utxo.get_outpoint());

‎src/send.rs‎

Lines changed: 52 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ pub enum TransactionComposeError {
6868
CreateTxError(CreateTxError),
6969
WalletError(String),
7070
Error(String),
71+
LockedUtxoSelected(Vec<String>),
7172
}
7273

7374
impl fmt::Display for TransactionComposeError {
@@ -76,6 +77,9 @@ impl fmt::Display for TransactionComposeError {
7677
TransactionComposeError::CreateTxError(e) => write!(f, "CreateTxError: {e}"),
7778
TransactionComposeError::WalletError(e) => write!(f, "WalletError: {e}"),
7879
TransactionComposeError::Error(e) => write!(f, "Error: {e}"),
80+
TransactionComposeError::LockedUtxoSelected(ids) => {
81+
write!(f, "LockedUtxoSelected: {}", ids.join(", "))
82+
}
7983
}
8084
}
8185
}
@@ -115,7 +119,8 @@ impl<P: WalletPersister> NgAccount<P> {
115119
utxos.clone(),
116120
&mut do_not_spend_utxos,
117121
&mut spendables,
118-
);
122+
)
123+
.map_err(TransactionComposeError::LockedUtxoSelected)?;
119124

120125
if spendables.is_empty() {
121126
return Err(TransactionComposeError::Error(
@@ -201,7 +206,8 @@ impl<P: WalletPersister> NgAccount<P> {
201206
match psbt.clone().extract_tx_fee_rate_limit() {
202207
Ok(..) => {
203208
max_fee_rate = FeeRateSatPerKwu::from_bdk(
204-
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
209+
psbt.fee_rate()
210+
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
205211
);
206212
if max_fee_rate < FeeRateSatPerKwu::from_sat_per_vb(1) {
207213
max_fee_rate = FeeRateSatPerKwu::from_sat_per_vb(1);
@@ -215,13 +221,15 @@ impl<P: WalletPersister> NgAccount<P> {
215221
}
216222
ExtractTxError::MissingInputValue { .. } => {
217223
max_fee_rate = FeeRateSatPerKwu::from_bdk(
218-
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
224+
psbt.fee_rate()
225+
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
219226
);
220227
break;
221228
}
222229
ExtractTxError::SendingTooMuch { psbt } => {
223230
max_fee_rate = FeeRateSatPerKwu::from_bdk(
224-
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
231+
psbt.fee_rate()
232+
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
225233
);
226234
break;
227235
}
@@ -335,7 +343,8 @@ impl<P: WalletPersister> NgAccount<P> {
335343
utxos.clone(),
336344
&mut do_not_spend_utxos,
337345
&mut spendables,
338-
);
346+
)
347+
.map_err(TransactionComposeError::LockedUtxoSelected)?;
339348

340349
let mut do_not_spend_amount = 0;
341350

@@ -429,24 +438,44 @@ impl<P: WalletPersister> NgAccount<P> {
429438
utxos: Vec<Output>,
430439
do_not_spend_utxos: &mut Vec<Output>,
431440
spendables: &mut Vec<Output>,
432-
) {
433-
for output in utxos {
434-
//choose all output that are not selected by the user,
435-
//this will create a pool of available utxo for tx builder.
436-
for selected_outputs in selected_outputs.clone() {
437-
if output.get_id() == selected_outputs.get_id() {
438-
spendables.push(output.clone());
439-
break;
440-
}
441-
}
442-
if selected_outputs.is_empty() && !output.do_not_spend {
443-
spendables.push(output.clone());
441+
) -> Result<(), Vec<String>> {
442+
let locked_utxo_ids: HashSet<String> = utxos
443+
.iter()
444+
.filter(|o| o.do_not_spend)
445+
.map(|o| o.get_id())
446+
.collect();
447+
448+
let mut locked_selected: Vec<String> = Vec::new();
449+
for selected in &selected_outputs {
450+
let id = selected.get_id();
451+
if (selected.do_not_spend || locked_utxo_ids.contains(&id))
452+
&& !locked_selected.contains(&id)
453+
{
454+
locked_selected.push(id);
444455
}
456+
}
457+
if !locked_selected.is_empty() {
458+
return Err(locked_selected);
459+
}
460+
461+
let selected_ids: HashSet<String> = selected_outputs.iter().map(|o| o.get_id()).collect();
462+
let has_explicit_selection = !selected_ids.is_empty();
445463

446-
if !spendables.contains(&output) {
447-
do_not_spend_utxos.push(output.clone());
464+
for output in utxos {
465+
let is_selected = selected_ids.contains(&output.get_id());
466+
if has_explicit_selection {
467+
if is_selected {
468+
spendables.push(output);
469+
} else {
470+
do_not_spend_utxos.push(output);
471+
}
472+
} else if output.do_not_spend {
473+
do_not_spend_utxos.push(output);
474+
} else {
475+
spendables.push(output);
448476
}
449477
}
478+
Ok(())
450479
}
451480

452481
pub(crate) fn transform_psbt_to_bitcointx(
@@ -478,7 +507,8 @@ impl<P: WalletPersister> NgAccount<P> {
478507
is_confirmed: false,
479508
fee: psbt.fee().unwrap_or(Amount::from_sat(0)).to_sat(),
480509
fee_rate: FeeRateSatPerKvb::from_bdk(
481-
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
510+
psbt.fee_rate()
511+
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
482512
),
483513
amount,
484514
inputs,
@@ -806,7 +836,8 @@ impl<P: WalletPersister> NgAccount<P> {
806836
is_confirmed: false,
807837
fee: psbt.fee().unwrap_or(Amount::from_sat(0)).to_sat(),
808838
fee_rate: FeeRateSatPerKvb::from_bdk(
809-
psbt.fee_rate().unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
839+
psbt.fee_rate()
840+
.unwrap_or(FeeRate::from_sat_per_vb_unchecked(1)),
810841
),
811842
amount: amount as i64,
812843
inputs: vec![],

‎tests/spend_test.rs‎

Lines changed: 156 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@ mod spend_tests {
66
use crate::utils::tests_util;
77
use bdk_wallet::rusqlite::Connection;
88
use ngwallet::account::NgAccount;
9-
use ngwallet::send::{DraftTransaction, FeeRateSatPerKvb, TransactionParams};
9+
use ngwallet::rbf::BumpFeeError;
10+
use ngwallet::send::{
11+
DraftTransaction, FeeRateSatPerKvb, TransactionComposeError, TransactionParams,
12+
};
1013

1114
use crate::utils::tests_util::get_ng_hot_wallet;
1215

@@ -25,7 +28,7 @@ mod spend_tests {
2528
};
2629
let draft = account.get_max_fee(params.clone()).unwrap();
2730
assert_eq!(draft.max_fee_rate, FeeRateSatPerKvb(553_828)); // 138_457 sat/kwu * 4 = sat/kvB
28-
assert_eq!(draft.min_fee_rate, FeeRateSatPerKvb(1000)); // 1 sat/vB in sat/kvB
31+
assert_eq!(draft.min_fee_rate, FeeRateSatPerKvb(1000)); // 1 sat/vB in sat/kvB
2932
check_draft_tx_match_params(draft.draft_transaction.clone(), params.clone());
3033
}
3134

@@ -147,6 +150,157 @@ mod spend_tests {
147150
assert_eq!(transaction.note, params.note);
148151
assert_eq!(transaction.get_change_tag(), params.tag);
149152
}
153+
154+
// Audit P2-01: a UTXO marked do_not_spend must never be spent, even when
155+
// the caller passes it explicitly in `selected_outputs`. Both the send
156+
// and RBF paths share this policy and must surface a dedicated error.
157+
158+
#[test]
159+
fn test_compose_psbt_rejects_locked_selected_utxo() {
160+
let mut account = get_ng_hot_wallet();
161+
tests_util::add_funds_to_wallet(&mut account);
162+
163+
let locked = account.utxos().unwrap()[0].clone();
164+
account
165+
.set_do_not_spend(locked.get_id().as_str(), true)
166+
.unwrap();
167+
let locked_live = account
168+
.utxos()
169+
.unwrap()
170+
.into_iter()
171+
.find(|o| o.get_id() == locked.get_id())
172+
.expect("locked utxo should still exist");
173+
assert!(locked_live.do_not_spend);
174+
175+
let params = TransactionParams {
176+
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
177+
amount: 4000,
178+
fee_rate: FeeRateSatPerKvb(2000),
179+
selected_outputs: vec![locked_live.clone()],
180+
note: None,
181+
tag: None,
182+
do_not_spend_change: false,
183+
};
184+
match account.compose_psbt(params) {
185+
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
186+
assert_eq!(ids, vec![locked_live.get_id()]);
187+
}
188+
other => panic!("expected LockedUtxoSelected, got {other:?}"),
189+
}
190+
}
191+
192+
// A caller cannot bypass the lock by zeroing `do_not_spend` on the
193+
// Output values it passes in `selected_outputs`. The wallet's live UTXO
194+
// set is the source of truth.
195+
#[test]
196+
fn test_compose_psbt_rejects_stale_unlocked_selected_utxo() {
197+
let mut account = get_ng_hot_wallet();
198+
tests_util::add_funds_to_wallet(&mut account);
199+
200+
let locked_id = {
201+
let utxo = account.utxos().unwrap()[0].clone();
202+
account
203+
.set_do_not_spend(utxo.get_id().as_str(), true)
204+
.unwrap();
205+
utxo.get_id()
206+
};
207+
208+
let mut stale = account
209+
.utxos()
210+
.unwrap()
211+
.into_iter()
212+
.find(|o| o.get_id() == locked_id)
213+
.unwrap();
214+
// Simulate a compromised caller forging do_not_spend=false.
215+
stale.do_not_spend = false;
216+
217+
let params = TransactionParams {
218+
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
219+
amount: 4000,
220+
fee_rate: FeeRateSatPerKvb(2000),
221+
selected_outputs: vec![stale],
222+
note: None,
223+
tag: None,
224+
do_not_spend_change: false,
225+
};
226+
match account.compose_psbt(params) {
227+
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
228+
assert_eq!(ids, vec![locked_id]);
229+
}
230+
other => panic!("expected LockedUtxoSelected, got {other:?}"),
231+
}
232+
}
233+
234+
#[test]
235+
fn test_get_max_fee_rejects_locked_selected_utxo() {
236+
let mut account = get_ng_hot_wallet();
237+
tests_util::add_funds_to_wallet(&mut account);
238+
239+
let locked = account.utxos().unwrap()[0].clone();
240+
account
241+
.set_do_not_spend(locked.get_id().as_str(), true)
242+
.unwrap();
243+
let locked_live = account
244+
.utxos()
245+
.unwrap()
246+
.into_iter()
247+
.find(|o| o.get_id() == locked.get_id())
248+
.unwrap();
249+
250+
let params = TransactionParams {
251+
address: "tb1pspfcrvz538vvj9f9gfkd85nu5ty98zw9y5e302kha6zurv6vg07s8z7a8w".to_string(),
252+
amount: 2003,
253+
fee_rate: FeeRateSatPerKvb(1000),
254+
selected_outputs: vec![locked_live.clone()],
255+
note: None,
256+
tag: None,
257+
do_not_spend_change: false,
258+
};
259+
match account.get_max_fee(params) {
260+
Err(TransactionComposeError::LockedUtxoSelected(ids)) => {
261+
assert_eq!(ids, vec![locked_live.get_id()]);
262+
}
263+
other => panic!("expected LockedUtxoSelected, got {other:?}"),
264+
}
265+
}
266+
267+
#[test]
268+
fn test_rbf_rejects_locked_selected_utxo() {
269+
let mut account = get_ng_hot_wallet();
270+
tests_util::add_funds_wallet_with_unconfirmed(&mut account);
271+
272+
// Lock a confirmed mature UTXO (one of the funding outputs) so the
273+
// RBF builder will see it as a candidate input.
274+
let mature = account
275+
.utxos()
276+
.unwrap()
277+
.into_iter()
278+
.find(|o| o.is_confirmed)
279+
.expect("at least one confirmed utxo");
280+
account
281+
.set_do_not_spend(mature.get_id().as_str(), true)
282+
.unwrap();
283+
let locked_live = account
284+
.utxos()
285+
.unwrap()
286+
.into_iter()
287+
.find(|o| o.get_id() == mature.get_id())
288+
.unwrap();
289+
290+
let unconfirmed_tx = account
291+
.transactions()
292+
.unwrap()
293+
.into_iter()
294+
.find(|tx| tx.confirmations == 0)
295+
.expect("expected an unconfirmed tx for RBF");
296+
297+
match account.get_max_bump_fee(vec![locked_live.clone()], unconfirmed_tx) {
298+
Err(BumpFeeError::LockedUtxoSelected(ids)) => {
299+
assert_eq!(ids, vec![locked_live.get_id()]);
300+
}
301+
other => panic!("expected LockedUtxoSelected, got {other:?}"),
302+
}
303+
}
150304
}
151305

152306
// fn pretty_print<T: serde::Serialize>(value: &T) -> String {

0 commit comments

Comments
 (0)