From 9f00a0bf0c69b8fb07e3892219708ecf47992af2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 9 Jan 2023 18:42:59 +0000 Subject: [PATCH] Consider counterparty inbound fees when forwarding While this code could without question be substantially more concise, being overly verbose and checking lots of overflows seems much better when we're checking HTLC forwarding amounts, given any bug trivially loses us money. --- lightning/src/ln/channel.rs | 99 +++++++++++++++++++++++++++++++++---- 1 file changed, 89 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 182fc986a..5ab900c1e 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -44,6 +44,7 @@ use crate::util::config::{UserConfig, ChannelConfig, LegacyChannelConfig, Channe use crate::util::scid_utils::scid_from_parts; use crate::io; +use core::convert::TryInto; use crate::prelude::*; use core::{cmp,mem,fmt}; use core::ops::Deref; @@ -4723,24 +4724,92 @@ impl Channel { } fn internal_htlc_satisfies_config( - &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, config: &ChannelConfig, + &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, + config: &ChannelConfig, inbound_chan_config: &ChannelConfig, ) -> Result { - let fee = amt_to_forward.checked_mul(config.forwarding_fee_proportional_millionths as u64) + if amt_to_forward > (core::i64::MAX as u64) || htlc.amount_msat > (core::i64::MAX as u64) { + // Return a fee_insufficient for lack of a better error + return Err(("HTLC requested we forward more than all available BTC", 0x1000 | 12)); + } + + let counterparty_fee_base_msat = self.counterparty_forwarding_info() + .map(|info| info.inbound_fee_base_msat).unwrap_or(0) as i64; + let fee_base_msat = (config.forwarding_fee_base_msat as i64) + counterparty_fee_base_msat; + let counterparty_fee_prop_millionths = self.counterparty_forwarding_info() + .map(|info| info.inbound_fee_proportional_millionths).unwrap_or(0) as i64; + let fee_prop_millionths = (config.forwarding_fee_proportional_millionths as i64) + + counterparty_fee_prop_millionths; + if fee_base_msat > core::u32::MAX.into() || fee_prop_millionths > core::u32::MAX.into() { + // Return a fee_insufficient for lack of a better error + return Err(("Total fee between us and our counterparty overflowed", 0x1000 | 12)); + } + + let total_outbound_fee = (amt_to_forward as i64).checked_mul(fee_prop_millionths) + .and_then(|prop_fee| (prop_fee / 1000000).checked_add(fee_base_msat)); + let their_fee = (amt_to_forward as i64).checked_mul(counterparty_fee_prop_millionths) + .and_then(|prop_fee| (prop_fee / 1000000).checked_add(counterparty_fee_base_msat)); + let our_outbound_fee = amt_to_forward.checked_mul(config.forwarding_fee_proportional_millionths as u64) .and_then(|prop_fee| (prop_fee / 1000000).checked_add(config.forwarding_fee_base_msat as u64)); - if fee.is_none() || htlc.amount_msat < fee.unwrap() || - (htlc.amount_msat - fee.unwrap()) < amt_to_forward { + + if total_outbound_fee.is_none() || their_fee.is_none() || our_outbound_fee.is_none() { + return Err(( + "Fee calculation overflowed, some node is attempting to take substantial fees", + 0x1000 | 12, // fee_insufficient + )); + } + debug_assert_eq!(our_outbound_fee.unwrap() as i128 + their_fee.unwrap() as i128, + total_outbound_fee.unwrap() as i128); + + let sender_expected_inbound_amt = (amt_to_forward as i64).checked_sub(total_outbound_fee.unwrap()); + if sender_expected_inbound_amt.is_none() || sender_expected_inbound_amt.unwrap() < 0 { + return Err(( + "Fee calculation overflowed, some node is attempting to take substantial fees", + 0x1000 | 12, // fee_insufficient + )); + } + + let our_inbound_fee = sender_expected_inbound_amt.unwrap() + .checked_mul(inbound_chan_config.inbound_forwarding_fee_proportional_millionths.into()) + .and_then(|prop_fee_millions| (prop_fee_millions / 1_000_000) + .checked_add(inbound_chan_config.inbound_forwarding_fee_base_msat.into())); + + let our_total_fee = if let Some(inbound_fee) = our_inbound_fee { + if let Some(Ok(outbound_fee)) = our_outbound_fee.map(|fee| fee.try_into()) { + inbound_fee.checked_add(outbound_fee) + } else { None } + } else { None }; + + if our_total_fee.is_none() { + return Err(( + "Fee calculation overflowed, some node is attempting to take substantial fees", + 0x1000 | 12, // fee_insufficient + )); + } + + let real_amt_to_forward = match (amt_to_forward as i64).checked_add(their_fee.unwrap()) { + None => return Err(( + "Forwarding amount calculation overflowed, some node is attempting to take substantial fees", + 0x1000 | 12, // fee_insufficient + )), + Some(amt) => cmp::max(amt, 0i64), + }; + + if (htlc.amount_msat as i64) < our_total_fee.unwrap() || + (htlc.amount_msat as i64 - our_total_fee.unwrap()) < real_amt_to_forward + { return Err(( "Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, // fee_insufficient )); } + if (htlc.cltv_expiry as u64) < outgoing_cltv_value as u64 + config.cltv_expiry_delta as u64 { return Err(( "Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, // incorrect_cltv_expiry )); } - Ok(amt_to_forward) + Ok(real_amt_to_forward as u64) } /// Determines whether the parameters of an incoming HTLC to be forwarded satisfy the channel's @@ -4752,13 +4821,23 @@ impl Channel { &self, htlc: &msgs::UpdateAddHTLC, amt_to_forward: u64, outgoing_cltv_value: u32, inbound_channel: &Self, ) -> Result { - self.internal_htlc_satisfies_config(&htlc, amt_to_forward, outgoing_cltv_value, &self.config()) + self.internal_htlc_satisfies_config(&htlc, amt_to_forward, outgoing_cltv_value, &self.config(), &inbound_channel.config()) .or_else(|err| { if let Some(prev_config) = self.prev_config() { - self.internal_htlc_satisfies_config(htlc, amt_to_forward, outgoing_cltv_value, &prev_config) - } else { - Err(err) - } + self.internal_htlc_satisfies_config(htlc, amt_to_forward, outgoing_cltv_value, &prev_config, &inbound_channel.config()) + } else { Err(err) } + }) + .or_else(|err| { + if let Some(prev_config) = inbound_channel.prev_config() { + self.internal_htlc_satisfies_config(htlc, amt_to_forward, outgoing_cltv_value, &self.config(), &prev_config) + } else { Err(err) } + }) + .or_else(|err| { + if let Some(prev_inbound_config) = inbound_channel.prev_config() { + if let Some(prev_config) = self.prev_config() { + self.internal_htlc_satisfies_config(htlc, amt_to_forward, outgoing_cltv_value, &prev_config, &prev_inbound_config) + } else { Err(err) } + } else { Err(err) } }) } -- 2.39.5