Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/psbt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ impl PsbtUtils for Psbt {
let tx = &self.unsigned_tx;
let input = self.inputs.get(input_index)?;

// Validate that the input index is within bounds of the transaction's inputs.
// This prevents out-of-bounds panic when PSBT is malformed (inputs.len() > tx.input.len()).
if input_index >= tx.input.len() {
return None;
}

match (&input.witness_utxo, &input.non_witness_utxo) {
(Some(_), _) => input.witness_utxo.clone(),
(_, Some(_)) => input.non_witness_utxo.as_ref().and_then(|prev_tx| {
Expand Down Expand Up @@ -154,4 +160,23 @@ mod tests {
// Must return None — vout out of bounds, no panic
assert_eq!(psbt.get_utxo_for(0), None);
}

#[test]
fn get_utxo_doesnt_panic_on_input_count_mismatch() {
let prev_tx = build_tx(Amount::from_sat(100_000));

// Create a valid PSBT with 1 input
let mut psbt = build_psbt(&prev_tx, 0);

// Add more inputs to psbt.inputs without adding to unsigned_tx.input
let extra_input = Input {
non_witness_utxo: Some(prev_tx.clone()),
..Default::default()
};
psbt.inputs.push(extra_input);

assert_eq!(psbt.inputs.len(), 2);
assert_eq!(psbt.unsigned_tx.input.len(), 1);
assert_eq!(None, psbt.get_utxo_for(1));
}
}
62 changes: 60 additions & 2 deletions tests/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use bitcoin::script::PushBytesBuf;
use bitcoin::sighash::{EcdsaSighashType, TapSighashType};
use bitcoin::taproot::TapNodeHash;
use bitcoin::{
absolute, transaction, Address, Amount, BlockHash, FeeRate, Network, OutPoint, ScriptBuf,
Sequence, SignedAmount, Transaction, TxIn, TxOut, Txid,
absolute, transaction, Address, Amount, BlockHash, FeeRate, Network, OutPoint, Psbt, ScriptBuf,
Sequence, SignedAmount, Transaction, TxIn, TxOut, Txid, Witness,
};
use rand::rngs::StdRng;
use rand::SeedableRng;
Expand Down Expand Up @@ -3007,3 +3007,61 @@ fn test_tx_ordering_untouched_preserves_insertion_ordering_bnb_success() {
"UTXOs should be ordered with required first, then selected"
);
}

#[test]
fn test_wallet_sign_rejects_malformed_psbt_input_count_mismatch() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus I am sorry I wasn't able to understand what this test is trying to check. Could you please add some more comments here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test effectively calls the wallet::sign with a malformed Psbt and expects the wallet to return an error instead of panicking.

So what I did here is crafting a raw transaction that initially just has 1 input then transformed it into a Psbt type to which I added more inputs (3 vs 1 in original tx).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but how does the input count relate to the error raised?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get your question but that's the essence of the audit report.
When Wallet::sign will be called with the current setup (3 inputs in PSBT but 1 in raw tx), the get_utxo_for will return None and then the SignerError::MissingNonWitnessUtxo will be returned.
Not sure if it answers the question.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We get the error even without the other 2 inputs because the first input itself does not contain a non_witness_utxo!

let (wallet, _) = get_funded_wallet_wpkh();

// Create a transaction with 1 input
let prev_tx = Transaction {
version: transaction::Version::TWO,
lock_time: absolute::LockTime::ZERO,
input: vec![TxIn {
previous_output: OutPoint::null(),
script_sig: ScriptBuf::default(),
sequence: Sequence::MAX,
witness: Witness::default(),
}],
output: vec![TxOut {
value: Amount::from_sat(50_000),
script_pubkey: ScriptBuf::default(),
}],
};

let unsigned_tx = Transaction {
version: transaction::Version::TWO,
lock_time: absolute::LockTime::ZERO,
input: vec![TxIn {
previous_output: OutPoint {
txid: prev_tx.compute_txid(),
vout: 0,
},
script_sig: ScriptBuf::default(),
sequence: Sequence::MAX,
witness: Witness::default(),
}],
output: vec![TxOut {
value: Amount::from_sat(40_000),
script_pubkey: ScriptBuf::default(),
}],
};

let mut psbt = Psbt::from_unsigned_tx(unsigned_tx).unwrap();

// Add extra PSBT inputs without adding to transaction
use bitcoin::psbt::Input;
psbt.inputs.push(Input {
non_witness_utxo: Some(prev_tx.clone()),
..Default::default()
});
psbt.inputs.push(Input {
non_witness_utxo: Some(prev_tx),
..Default::default()
});

assert_eq!(psbt.inputs.len(), 3);
assert_eq!(psbt.unsigned_tx.input.len(), 1);

let result = wallet.sign(&mut psbt.clone(), SignOptions::default());
assert!(matches!(result, Err(SignerError::MissingNonWitnessUtxo)));
}