diff --git a/payjoin/src/core/receive/common/mod.rs b/payjoin/src/core/receive/common/mod.rs index 5d2cc2d80..514c59f5e 100644 --- a/payjoin/src/core/receive/common/mod.rs +++ b/payjoin/src/core/receive/common/mod.rs @@ -32,13 +32,27 @@ pub struct OriginalContext { impl OriginalContext { /// Live and replay both route through here so the param sanitization can't diverge. pub(super) fn new(original_psbt: Psbt, mut params: Params, owned_vouts: &[usize]) -> Self { - if let Some((_, additional_fee_output_index)) = params.additional_fee_contribution { + if let Some((max_additional_fee_contribution, additional_fee_output_index)) = + params.additional_fee_contribution + { // Per BIP78, ignore a fee-contribution index that is out of bounds or // pointing at a receiver output. // https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#optional-parameters - if additional_fee_output_index >= original_psbt.unsigned_tx.output.len() - || owned_vouts.contains(&additional_fee_output_index) - { + // Also ignore a contribution outside the valid range: subtracting + // more than the fee output's value minus its dust value would leave + // a dust output. + let fee_output = original_psbt.unsigned_tx.output.get(additional_fee_output_index); + let in_valid_range = match fee_output { + Some(fee_output) if !owned_vouts.contains(&additional_fee_output_index) => { + let max_contribution = fee_output + .value + .checked_sub(fee_output.script_pubkey.minimal_non_dust()) + .unwrap_or(Amount::ZERO); + max_additional_fee_contribution <= max_contribution + } + _ => false, + }; + if !in_valid_range { params.additional_fee_contribution = None; } } @@ -483,7 +497,10 @@ impl WantsFeeRange { .iter() .position(|txo| txo.script_pubkey == sender_fee_output.script_pubkey) .expect("Sender output is missing from payjoin PSBT"); - // Determine the additional amount that the sender will pay in fees + // Determine the additional amount that the sender will pay in fees. + // Sanitization bounds the contribution to the fee output's value + // minus its dust value, and the sender output is copied unchanged + // into the payjoin PSBT, so the subtraction cannot underflow. let sender_additional_fee = min(max_additional_fee_contribution, additional_fee); tracing::trace!("sender_additional_fee: {sender_additional_fee}"); // Remove additional miner fee from the sender's specified output @@ -507,9 +524,18 @@ impl WantsFeeRange { return Err(InternalPayloadError::FeeTooHigh(proposed_fee_rate, max_fee_rate)); } if receiver_additional_fee >= Amount::ONE_SAT { - // Remove additional miner fee from the receiver's specified output - payjoin_psbt.unsigned_tx.output[self.proposal.change_vout].value -= - receiver_additional_fee; + // Remove additional miner fee from the receiver's specified output. + // Reject rather than underflow when a small payment plus a high + // sender minfeerate makes the fee exceed the change output's value. + let change_output = &mut payjoin_psbt.unsigned_tx.output[self.proposal.change_vout]; + change_output.value = + change_output.value.checked_sub(receiver_additional_fee).ok_or_else(|| { + InternalPayloadError::FeeTooHigh( + receiver_additional_fee + / (input_contribution_weight + output_contribution_weight), + max_fee_rate, + ) + })?; } Ok(payjoin_psbt) } @@ -535,7 +561,10 @@ impl WantsFeeRange { .output .iter() .fold(Weight::ZERO, |acc, txo| acc + txo.weight()); - let output_contribution_weight = payjoin_outputs_weight - original_outputs_weight; + // If the receiver's substitution shrank the total output size, the + // contribution is negative; treat it as zero rather than underflowing. + let output_contribution_weight = + payjoin_outputs_weight.checked_sub(original_outputs_weight).unwrap_or(Weight::ZERO); tracing::trace!("output_contribution_weight : {output_contribution_weight}"); output_contribution_weight } @@ -1213,4 +1242,154 @@ mod tests { ) .expect("fee calculation should succeed without the sender contribution"); } + + // A `maxadditionalfeecontribution` outside the valid range — greater than + // the fee output's value minus its dust value — must be ignored at + // sanitization so the receiver, not the sender output, pays the additional + // fee, rather than clamped or underflowing the output's Amount subtraction. + #[test] + fn excessive_fee_contribution_is_ignored() { + let mut original = original_from_test_vector(); + let sender_script = original.psbt.unsigned_tx.output[0].script_pubkey.clone(); + // Fee index 0 is a sender output when the receiver owns vout 1, so the + // contribution survives the index checks; the 100_000_000 sat + // contribution exceeds the output's 95983068 sat value and is out of + // range. + original.params.additional_fee_contribution = Some((Amount::from_sat(100_000_000), 0)); + + let wants_inputs = WantsOutputs::new(original, vec![1]).commit_outputs(); + assert_eq!( + wants_inputs.original.params.additional_fee_contribution, None, + "out-of-range fee contribution must be dropped at sanitization" + ); + + let proposal_psbt = Psbt::from_str(RECEIVER_INPUT_CONTRIBUTION).unwrap(); + let input = InputPair::new( + proposal_psbt.unsigned_tx.input[1].clone(), + proposal_psbt.inputs[1].clone(), + None, + ) + .unwrap(); + let wants_fee_range = wants_inputs + .contribute_inputs([input]) + .expect("contribution should succeed") + .commit_inputs(); + + let psbt = wants_fee_range + .calculate_psbt_with_fee_range( + Some(FeeRate::from_sat_per_vb_u32(1000)), + Some(FeeRate::from_sat_per_vb_u32(1000)), + ) + .expect("receiver must cover the fee without the sender contribution"); + + let sender_out = psbt + .unsigned_tx + .output + .iter() + .find(|txo| txo.script_pubkey == sender_script) + .expect("sender output must be present"); + assert_eq!(sender_out.value, Amount::from_sat(95_983_068)); + } + + // A contribution of exactly the fee output's value minus its dust value is + // the top of the valid range and must survive sanitization; one sat more + // must be dropped. + #[test] + fn fee_contribution_dust_boundary() { + let mut original = original_from_test_vector(); + let fee_output = original.psbt.unsigned_tx.output[0].clone(); + let max_contribution = fee_output.value - fee_output.script_pubkey.minimal_non_dust(); + + original.params.additional_fee_contribution = Some((max_contribution, 0)); + let wants_outputs = WantsOutputs::new(original.clone(), vec![1]); + assert_eq!( + wants_outputs.original.params.additional_fee_contribution, + Some((max_contribution, 0)), + "a contribution leaving exactly the dust value is in range" + ); + + original.params.additional_fee_contribution = Some((max_contribution + Amount::ONE_SAT, 0)); + let wants_outputs = WantsOutputs::new(original, vec![1]); + assert_eq!( + wants_outputs.original.params.additional_fee_contribution, None, + "a contribution past the dust value is out of range" + ); + } + + // A receiver fee exceeding the receiver change output's value must return + // FeeTooHigh instead of panicking on the Amount subtraction. + #[test] + fn receiver_fee_exceeding_change_outputs_fee_too_high() { + use crate::receive::InternalPayloadError; + + let original = original_from_test_vector(); + let mut wants_fee_range = + WantsOutputs::new(original, vec![0]).commit_outputs().commit_inputs(); + let payjoin_psbt = &mut wants_fee_range.proposal.payjoin_psbt; + // A large additional output makes the receiver owe substantial weight + // fees while the change output stays too small to cover them. + payjoin_psbt.unsigned_tx.output.push(TxOut { + value: Amount::ZERO, + script_pubkey: ScriptBuf::from_bytes(vec![0x51; 10_000]), + }); + payjoin_psbt.outputs.push(Default::default()); + payjoin_psbt.unsigned_tx.output[0].value = Amount::from_sat(100); + + // Equal min and max rates so the max_fee check passes and only the + // change-value check fires. + let fee_rate = FeeRate::from_sat_per_vb_u32(25_000); + let result = wants_fee_range.calculate_psbt_with_fee_range(Some(fee_rate), Some(fee_rate)); + match result { + Err(InternalPayloadError::FeeTooHigh(proposed, max)) => { + assert_eq!(max, fee_rate); + assert!(proposed > FeeRate::BROADCAST_MIN); + } + _ => panic!("expected FeeTooHigh when receiver fee exceeds change output"), + } + } + + // A receiver fee exactly equal to the change output's value must succeed + // and drain the change output to zero rather than error, pinning the + // strict inequality of the change-value check. + #[test] + fn receiver_fee_equal_to_change_drains_change_to_zero() { + let original = original_from_test_vector(); + let mut wants_fee_range = + WantsOutputs::new(original, vec![0]).commit_outputs().commit_inputs(); + let payjoin_psbt = &mut wants_fee_range.proposal.payjoin_psbt; + // One extra 1-byte-script output contributes 8 + 1 + 1 = 10 bytes + // = 40 weight units, so at 1 sat/vb (250 sat/kwu) the receiver fee is + // exactly ceil(40 * 250 / 1000) = 10 sats. + payjoin_psbt + .unsigned_tx + .output + .push(TxOut { value: Amount::ZERO, script_pubkey: ScriptBuf::from_bytes(vec![0x51]) }); + payjoin_psbt.outputs.push(Default::default()); + payjoin_psbt.unsigned_tx.output[0].value = Amount::from_sat(10); + + // Equal min and max rates so the max_fee check passes and only the + // change-value boundary is exercised. + let fee_rate = FeeRate::from_sat_per_vb_u32(1); + let psbt = wants_fee_range + .calculate_psbt_with_fee_range(Some(fee_rate), Some(fee_rate)) + .expect("a fee exactly equal to the change value must not error"); + assert_eq!(psbt.unsigned_tx.output[0].value, Amount::ZERO); + } + + // Substituting a receiver output script smaller than the original shrinks + // the total output weight; the Weight subtraction must saturate at zero + // instead of underflowing. + #[test] + fn shrinking_output_substitution_does_not_underflow() { + let original = original_from_test_vector(); + let mut wants_fee_range = + WantsOutputs::new(original, vec![0]).commit_outputs().commit_inputs(); + wants_fee_range.proposal.payjoin_psbt.unsigned_tx.output[0].script_pubkey = + ScriptBuf::new(); + + let psbt = wants_fee_range + .calculate_psbt_with_fee_range(None, None) + .expect("shrinking substitution must not underflow output weight"); + assert!(psbt.unsigned_tx.output[0].script_pubkey.is_empty()); + } } diff --git a/payjoin/src/core/receive/optional_parameters.rs b/payjoin/src/core/receive/optional_parameters.rs index e6716038e..793b140d9 100644 --- a/payjoin/src/core/receive/optional_parameters.rs +++ b/payjoin/src/core/receive/optional_parameters.rs @@ -98,8 +98,20 @@ impl Params { Ok(fee_rate_sat_per_vb) => { // TODO Parse with serde when rust-bitcoin supports it let fee_rate_sat_per_kwu = fee_rate_sat_per_vb * 250.0_f32; + if !(fee_rate_sat_per_kwu.is_finite() && fee_rate_sat_per_kwu >= 0.0) { + return Err(Error::FeeRate); + } // since it's a minimum, we want to round up - FeeRate::from_sat_per_kwu(fee_rate_sat_per_kwu.ceil() as u64) + let fee_rate_sat_per_kwu = fee_rate_sat_per_kwu.ceil() as u64; + // Reject absurd rates before they reach fee arithmetic: + // a saturated u64::MAX sat/kwu would overflow the + // Weight * FeeRate fee computation. + if FeeRate::from_sat_per_kwu(fee_rate_sat_per_kwu) + > bitcoin::Psbt::DEFAULT_MAX_FEE_RATE + { + return Err(Error::FeeRate); + } + FeeRate::from_sat_per_kwu(fee_rate_sat_per_kwu) } Err(_) => return Err(Error::FeeRate), }, @@ -183,4 +195,33 @@ pub(crate) mod test { assert!(params.is_err()); assert_eq!(params.err().unwrap(), Error::UnknownVersion { supported_versions }); } + + #[test] + fn min_fee_rate_rejected_when_negative() { + // A finite negative rate must be rejected outright, not clamped to + // zero by the saturating `as u64` cast. + assert_eq!( + Params::from_query_str("minfeerate=-1", &[Version::One]).unwrap_err(), + Error::FeeRate + ); + } + + #[test] + fn min_fee_rate_rejected_above_sanity_ceiling() { + // A rate whose sat/kwu saturates near u64::MAX must be rejected rather + // than reaching fee arithmetic where Weight * FeeRate would overflow. + assert_eq!( + Params::from_query_str("minfeerate=100000000000000000000", &[Version::One]) + .unwrap_err(), + Error::FeeRate + ); + } + + #[test] + fn min_fee_rate_at_ceiling_is_accepted() { + // `DEFAULT_MAX_FEE_RATE` (25000 sat/vB) is the boundary and must pass. + let params = + Params::from_query_str("minfeerate=25000", &[Version::One]).expect("valid feerate"); + assert_eq!(params.min_fee_rate, bitcoin::Psbt::DEFAULT_MAX_FEE_RATE); + } } diff --git a/payjoin/src/core/send/error.rs b/payjoin/src/core/send/error.rs index 7143cc9cb..78cca2046 100644 --- a/payjoin/src/core/send/error.rs +++ b/payjoin/src/core/send/error.rs @@ -54,7 +54,7 @@ impl fmt::Display for BuildSenderError { NoOutputs => write!(f, "the original transaction has no outputs"), MultiplePayeeOutputs => write!(f, "the original transaction has more than one output belonging to the payee"), MissingPayeeOutput => write!(f, "the output belonging to payee is missing from the original transaction"), - FeeOutputValueLowerThanFeeContribution => write!(f, "the value of fee output is lower than maximum allowed contribution"), + FeeOutputValueLowerThanFeeContribution => write!(f, "the value of fee output is lower than maximum allowed contribution, or the contribution would leave the output at or below its dust value"), AmbiguousChangeOutput => write!(f, "can not determine which output is change because there's more than two outputs"), ChangeIndexOutOfBounds => write!(f, "fee output index is points out of bounds"), ChangeIndexPointsAtPayee => write!(f, "fee output index is points at output belonging to the payee"), diff --git a/payjoin/src/core/send/mod.rs b/payjoin/src/core/send/mod.rs index 4bd503b19..3e210b984 100644 --- a/payjoin/src/core/send/mod.rs +++ b/payjoin/src/core/send/mod.rs @@ -603,15 +603,21 @@ fn clear_unneeded_fields(psbt: &mut Psbt) { } } -/// Ensure that an additional fee output is sufficient to pay for the specified additional fee +/// Ensure that an additional fee output can pay for the specified additional fee +/// without dropping to its dust value, so an honest sender never offers a +/// contribution the receiver will ignore. fn check_fee_output_amount( output: &TxOut, fee: bitcoin::Amount, clamp_fee_contribution: bool, ) -> Result { - if output.value < fee { + let max_contribution = output + .value + .checked_sub(output.script_pubkey.minimal_non_dust()) + .unwrap_or(bitcoin::Amount::ZERO); + if fee > max_contribution { if clamp_fee_contribution { - Ok(output.value) + Ok(max_contribution) } else { Err(InternalBuildSenderError::FeeOutputValueLowerThanFeeContribution) } @@ -679,12 +685,15 @@ fn determine_fee_contribution( fee_contribution: Option<(bitcoin::Amount, Option)>, clamp_fee_contribution: bool, ) -> Result, InternalBuildSenderError> { - Ok(match fee_contribution { + let contribution = match fee_contribution { Some((fee, None)) => find_change_index(psbt, payee, fee, clamp_fee_contribution)?, Some((fee, Some(index))) => Some(check_change_index(psbt, payee, fee, index, clamp_fee_contribution)?), None => None, - }) + }; + // A clamped zero contribution offers the receiver nothing and would only + // advertise a fee output the receiver must leave untouched. + Ok(contribution.filter(|contribution| contribution.max_amount > bitcoin::Amount::ZERO)) } fn serialize_url( @@ -852,13 +861,63 @@ mod test { Script::from_bytes(& as FromHex>::from_hex( "0014b60943f60c3ee848828bdace7474a92e81f3fcdd", )?), - Some((Amount::from_sat(95983068), None)), + // The change output (vout 0) holds 95983068 sats and is P2SH with + // a 540 sat dust value, so the contribution must leave 540 sats. + Some((Amount::from_sat(95983068 - 540), None)), false, ); assert!(fee_contribution.is_ok()); Ok(()) } + #[test] + fn test_fee_contribution_above_dust_margin() -> Result<(), BoxError> { + let payee_script = ScriptBuf::from_hex("0014b60943f60c3ee848828bdace7474a92e81f3fcdd")?; + let mut psbt = PARSED_ORIGINAL_PSBT.clone(); + psbt.unsigned_tx.output[0].value = Amount::from_sat(1000); + + // A contribution of 1000 sats from a 1000 sat P2SH output would leave + // it dust, so it must be rejected rather than offered to the receiver. + let fee_contribution = determine_fee_contribution( + &psbt, + &payee_script, + Some((Amount::from_sat(1000), None)), + false, + ); + assert_eq!( + fee_contribution, + Err(InternalBuildSenderError::FeeOutputValueLowerThanFeeContribution) + ); + + // With clamping, the contribution is decreased to the output's value + // minus its 540 sat dust value. + let fee_contribution = determine_fee_contribution( + &psbt, + &payee_script, + Some((Amount::from_sat(1000), None)), + true, + ); + assert_eq!( + fee_contribution, + Ok(Some(AdditionalFeeContribution { + max_amount: Amount::from_sat(1000 - 540), + vout: 0, + })) + ); + + // An output that cannot afford any contribution above dust contributes + // nothing at all. + psbt.unsigned_tx.output[0].value = Amount::from_sat(540); + let fee_contribution = determine_fee_contribution( + &psbt, + &payee_script, + Some((Amount::from_sat(1000), None)), + true, + ); + assert_eq!(fee_contribution, Ok(None)); + Ok(()) + } + #[test] fn test_self_pay_change_index() -> Result<(), BoxError> { let script_bytes = diff --git a/payjoin/src/core/send/v1.rs b/payjoin/src/core/send/v1.rs index b9fa86e7f..a7c601765 100644 --- a/payjoin/src/core/send/v1.rs +++ b/payjoin/src/core/send/v1.rs @@ -336,10 +336,9 @@ mod test { ) .build_recommended(FeeRate::MIN); assert!(sender.is_ok(), "{:#?}", sender.err()); - assert_eq!( - sender.unwrap().psbt_ctx.fee_contribution.unwrap().max_amount, - Amount::from_sat(0) - ); + // A zero fee contribution offers the receiver nothing and is dropped + // instead of advertised in the request. + assert!(sender.unwrap().psbt_ctx.fee_contribution.is_none()); Ok(()) } @@ -364,10 +363,9 @@ mod test { ) .build_recommended(FeeRate::MIN); assert!(sender.is_ok(), "{:#?}", sender.err()); - assert_eq!( - sender.unwrap().psbt_ctx.fee_contribution.unwrap().max_amount, - Amount::from_sat(0) - ); + // A zero fee contribution offers the receiver nothing and is dropped + // instead of advertised in the request. + assert!(sender.unwrap().psbt_ctx.fee_contribution.is_none()); let mut psbt = Psbt::from_str(MULTIPARTY_ORIGINAL_PSBT_ONE).unwrap(); psbt.unsigned_tx.input.pop(); @@ -382,9 +380,11 @@ mod test { ) .build_recommended(FeeRate::from_sat_per_vb(170000000).expect("Could not determine feerate")); assert!(sender.is_ok(), "{:#?}", sender.err()); + // The contribution is clamped to the change output's value minus its + // 294 sat P2WPKH dust value. assert_eq!( sender.unwrap().psbt_ctx.fee_contribution.unwrap().max_amount, - Amount::from_sat(9999999822) + Amount::from_sat(9999999822 - 294) ); Ok(()) @@ -402,7 +402,9 @@ mod test { assert_eq!(&sender.psbt_ctx.payee, &pj_uri(PJ_URI).address().script_pubkey()); let fee_contribution = sender.psbt_ctx.fee_contribution.expect("sender should contribute fees"); - assert_eq!(fee_contribution.max_amount, psbt.unsigned_tx.output[0].value); + // The contribution is clamped to the change output's value minus its + // 540 sat P2SH dust value. + assert_eq!(fee_contribution.max_amount, Amount::from_sat(95983068 - 540)); assert_eq!(fee_contribution.vout, 0); assert_eq!(sender.psbt_ctx.min_fee_rate, FeeRate::from_sat_per_kwu(500000000)); }