Skip to content

Commit

Permalink
Merge pull request #3452 from arik-so/stop-disbursement-underflow
Browse files Browse the repository at this point in the history 
Detect underflows in `build_closing_transaction`
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored Dec 11, 2024
2 parents 9728e51 + a652980 commit 98a46ac
Showing 1 changed file with 36 additions and 20 deletions.
56 changes: 36 additions & 20 deletions lightning/src/ln/channel.rs
View file
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);
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()});
(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

0 comments on commit 98a46ac

Please sign in to comment.