Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect underflows in build_closing_transaction #3452

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Detect underflows in build_closing_transaction #3452

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13835c0
Detect underflows in `build_closing_transaction`
arik-so Dec 9, 2024
a652980
Force-close channels on underflow
arik-so Dec 9, 2024
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
56 changes: 36 additions & 20 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,7 @@ pub(super) struct ChannelContext<SP: Deref> 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.
Expand Down Expand Up @@ -4351,7 +4351,7 @@ impl<SP: Deref> Channel<SP> 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());
Expand All @@ -4368,10 +4368,18 @@ impl<SP: Deref> Channel<SP> where
total_fee_satoshis += (-value_to_counterparty) as u64;
}

debug_assert!(value_to_counterparty >= 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean panicing in debug is great, but we think its unreachable, we should have some code to handle it (FC the channel) in release builds as well.

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;
}
Expand All @@ -4382,7 +4390,7 @@ impl<SP: Deref> Channel<SP> 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 {
Expand Down Expand Up @@ -6136,19 +6144,27 @@ impl<SP: Deref> Channel<SP> 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()});
Copy link
Contributor

Choose a reason for hiding this comment

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

See: https://github.com/lightningdevkit/rust-lightning/blob/main/lightning/src/ln/channelmanager.rs#L3040-L3044

locked_close_channel mentions 2 things that need to happen in addition to calling force_shutdown, and they are being handled in convert_chan_phase_err.

Do we need to do something similar here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets handled by the call site:

rust-lightning/lightning/src/ln/channelmanager.rs

Line 9598 in e0d81ca

locked_close_channel!(self, peer_state, context, shutdown_result);

(None, None, Some(shutdown))
}
}
} else { (None, None, None) }
} else { (None, None, None) };

Expand Down Expand Up @@ -6616,7 +6632,7 @@ impl<SP: Deref> Channel<SP> 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);

Expand Down Expand Up @@ -6850,7 +6866,7 @@ impl<SP: Deref> Channel<SP> 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)));
}
Expand All @@ -6862,7 +6878,7 @@ impl<SP: Deref> Channel<SP> 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());
},
Expand Down Expand Up @@ -6893,7 +6909,7 @@ impl<SP: Deref> Channel<SP> 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);
Expand Down
Loading