From 13835c04cc134cf595f501a01eef65f6c53ed1ae Mon Sep 17 00:00:00 2001 From: Arik Sosman Date: Mon, 9 Dec 2024 11:11:17 -0800 Subject: [PATCH 1/2] Detect underflows in `build_closing_transaction` In `build_closing_transaction`, we check that `value_to_holder` and `value_to_counterparty`, which are signed, are not lower than the dust limit. However, in doing this check, we convert them to signed integers, which could result in an underflow and a failed detection. This scenario should not be reachable, but here we add debug_asserts to positive ensure that scenario isn't hit. --- lightning/src/ln/channel.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 32e81cabda0..0c0def417a5 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1501,7 +1501,7 @@ pub(super) struct ChannelContext where SP::Target: SignerProvider { // The `next_funding_txid` field allows peers to finalize the signing steps of an interactive // transaction construction, or safely abort that transaction if it was not signed by one of the // peers, who has thus already removed it from its state. - // + // // If we've sent `commtiment_signed` for an interactively constructed transaction // during a signing session, but have not received `tx_signatures` we MUST set `next_funding_txid` // to the txid of that interactive transaction, else we MUST NOT set it. @@ -4368,10 +4368,12 @@ impl Channel where total_fee_satoshis += (-value_to_counterparty) as u64; } + debug_assert!(value_to_counterparty >= 0); if skip_remote_output || value_to_counterparty as u64 <= self.context.holder_dust_limit_satoshis { value_to_counterparty = 0; } + debug_assert!(value_to_holder >= 0); if value_to_holder as u64 <= self.context.holder_dust_limit_satoshis { value_to_holder = 0; } From a652980045b1f4ff0f80cb83bc4aef464f819e00 Mon Sep 17 00:00:00 2001 From: Arik Sosman Date: Mon, 9 Dec 2024 13:13:58 -0800 Subject: [PATCH 2/2] Force-close channels on underflow Following up on the previous commit, where we added debug_asserts within `build_closing_transaction` to ensure neither `value_to_holder` nor `value_to_counterparty` underflow, we now also force-close the channel in the (presumably impossible) event that it did happen. --- lightning/src/ln/channel.rs | 52 +++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0c0def417a5..62df7f39f66 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4351,7 +4351,7 @@ impl Channel where } #[inline] - fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> (ClosingTransaction, u64) { + fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> Result<(ClosingTransaction, u64), ChannelError> { assert!(self.context.pending_inbound_htlcs.is_empty()); assert!(self.context.pending_outbound_htlcs.is_empty()); assert!(self.context.pending_update_fee.is_none()); @@ -4369,11 +4369,17 @@ impl Channel where } debug_assert!(value_to_counterparty >= 0); + if value_to_counterparty < 0 { + return Err(ChannelError::close(format!("Value to counterparty below 0: {}", value_to_counterparty))) + } if skip_remote_output || value_to_counterparty as u64 <= self.context.holder_dust_limit_satoshis { value_to_counterparty = 0; } debug_assert!(value_to_holder >= 0); + if value_to_holder < 0 { + return Err(ChannelError::close(format!("Value to holder below 0: {}", value_to_holder))) + } if value_to_holder as u64 <= self.context.holder_dust_limit_satoshis { value_to_holder = 0; } @@ -4384,7 +4390,7 @@ impl Channel where let funding_outpoint = self.funding_outpoint().into_bitcoin_outpoint(); let closing_transaction = ClosingTransaction::new(value_to_holder as u64, value_to_counterparty as u64, holder_shutdown_script, counterparty_shutdown_script, funding_outpoint); - (closing_transaction, total_fee_satoshis) + Ok((closing_transaction, total_fee_satoshis)) } fn funding_outpoint(&self) -> OutPoint { @@ -6138,19 +6144,27 @@ impl Channel where if let Some((fee, skip_remote_output, fee_range, holder_sig)) = self.context.last_sent_closing_fee.clone() { debug_assert!(holder_sig.is_none()); log_trace!(logger, "Attempting to generate pending closing_signed..."); - let (closing_tx, fee) = self.build_closing_transaction(fee, skip_remote_output); - let closing_signed = self.get_closing_signed_msg(&closing_tx, skip_remote_output, - fee, fee_range.min_fee_satoshis, fee_range.max_fee_satoshis, logger); - let signed_tx = if let (Some(ClosingSigned { signature, .. }), Some(counterparty_sig)) = - (closing_signed.as_ref(), self.context.last_received_closing_sig) { - let funding_redeemscript = self.context.get_funding_redeemscript(); - let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.context.channel_value_satoshis); - debug_assert!(self.context.secp_ctx.verify_ecdsa(&sighash, &counterparty_sig, - &self.context.get_counterparty_pubkeys().funding_pubkey).is_ok()); - Some(self.build_signed_closing_transaction(&closing_tx, &counterparty_sig, signature)) - } else { None }; - let shutdown_result = signed_tx.as_ref().map(|_| self.shutdown_result_coop_close()); - (closing_signed, signed_tx, shutdown_result) + let closing_transaction_result = self.build_closing_transaction(fee, skip_remote_output); + match closing_transaction_result { + Ok((closing_tx, fee)) => { + let closing_signed = self.get_closing_signed_msg(&closing_tx, skip_remote_output, + fee, fee_range.min_fee_satoshis, fee_range.max_fee_satoshis, logger); + let signed_tx = if let (Some(ClosingSigned { signature, .. }), Some(counterparty_sig)) = + (closing_signed.as_ref(), self.context.last_received_closing_sig) { + let funding_redeemscript = self.context.get_funding_redeemscript(); + let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.context.channel_value_satoshis); + debug_assert!(self.context.secp_ctx.verify_ecdsa(&sighash, &counterparty_sig, + &self.context.get_counterparty_pubkeys().funding_pubkey).is_ok()); + Some(self.build_signed_closing_transaction(&closing_tx, &counterparty_sig, signature)) + } else { None }; + let shutdown_result = signed_tx.as_ref().map(|_| self.shutdown_result_coop_close()); + (closing_signed, signed_tx, shutdown_result) + } + Err(err) => { + let shutdown = self.context.force_shutdown(true, ClosureReason::ProcessingError {err: err.to_string()}); + (None, None, Some(shutdown)) + } + } } else { (None, None, None) } } else { (None, None, None) }; @@ -6618,7 +6632,7 @@ impl Channel where let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator); assert!(self.context.shutdown_scriptpubkey.is_some()); - let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(our_min_fee, false); + let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(our_min_fee, false)?; log_trace!(logger, "Proposing initial closing_signed for our counterparty with a fee range of {}-{} sat (with initial proposal {} sats)", our_min_fee, our_max_fee, total_fee_satoshis); @@ -6852,7 +6866,7 @@ impl Channel where let funding_redeemscript = self.context.get_funding_redeemscript(); let mut skip_remote_output = false; - let (mut closing_tx, used_total_fee) = self.build_closing_transaction(msg.fee_satoshis, skip_remote_output); + let (mut closing_tx, used_total_fee) = self.build_closing_transaction(msg.fee_satoshis, skip_remote_output)?; if used_total_fee != msg.fee_satoshis { return Err(ChannelError::close(format!("Remote sent us a closing_signed with a fee other than the value they can claim. Fee in message: {}. Actual closing tx fee: {}", msg.fee_satoshis, used_total_fee))); } @@ -6864,7 +6878,7 @@ impl Channel where // The remote end may have decided to revoke their output due to inconsistent dust // limits, so check for that case by re-checking the signature here. skip_remote_output = true; - closing_tx = self.build_closing_transaction(msg.fee_satoshis, skip_remote_output).0; + closing_tx = self.build_closing_transaction(msg.fee_satoshis, skip_remote_output)?.0; let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.context.channel_value_satoshis); secp_check!(self.context.secp_ctx.verify_ecdsa(&sighash, &msg.signature, self.context.counterparty_funding_pubkey()), "Invalid closing tx signature from peer".to_owned()); }, @@ -6895,7 +6909,7 @@ impl Channel where (closing_tx, $new_fee) } else { skip_remote_output = false; - self.build_closing_transaction($new_fee, skip_remote_output) + self.build_closing_transaction($new_fee, skip_remote_output)? }; let closing_signed = self.get_closing_signed_msg(&closing_tx, skip_remote_output, used_fee, our_min_fee, our_max_fee, logger);