From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Date: Thu, 28 Apr 2022 21:56:49 +0000 (+0000)
Subject: Merge pull request #1454 from TheBlueMatt/2022-04-fuzz-underflow
X-Git-Tag: v0.0.107~49
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=dc8479a6202ea84489e51cba020a600f0a2b9695;hp=-c;p=rust-lightning

Merge pull request #1454 from TheBlueMatt/2022-04-fuzz-underflow

Reject channels if the total reserves are larger than the funding
---

dc8479a6202ea84489e51cba020a600f0a2b9695
diff --combined lightning/src/ln/channel.rs
index cf70749a,60ff1ded..1cb7a689
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@@ -62,17 -62,6 +62,17 @@@ pub struct ChannelValueStat 
  	pub counterparty_dust_limit_msat: u64,
  }
  
 +pub struct AvailableBalances {
 +	/// The amount that would go to us if we close the channel, ignoring any on-chain fees.
 +	pub balance_msat: u64,
 +	/// Total amount available for our counterparty to send to us.
 +	pub inbound_capacity_msat: u64,
 +	/// Total amount available for us to send to our counterparty.
 +	pub outbound_capacity_msat: u64,
 +	/// The maximum value we can assign to the next outbound HTLC
 +	pub next_outbound_htlc_limit_msat: u64,
 +}
 +
  #[derive(Debug, Clone, Copy, PartialEq)]
  enum FeeUpdateState {
  	// Inbound states mirroring InboundHTLCState
@@@ -745,13 -734,9 +745,13 @@@ pub const COMMITMENT_TX_WEIGHT_PER_HTLC
  
  pub const ANCHOR_OUTPUT_VALUE_SATOSHI: u64 = 330;
  
 -/// Maximum `funding_satoshis` value, according to the BOLT #2 specification
 -/// it's 2^24.
 -pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24;
 +/// Maximum `funding_satoshis` value according to the BOLT #2 specification, if
 +/// `option_support_large_channel` (aka wumbo channels) is not supported.
 +/// It's 2^24 - 1.
 +pub const MAX_FUNDING_SATOSHIS_NO_WUMBO: u64 = (1 << 24) - 1;
 +
 +/// Total bitcoin supply in satoshis.
 +pub const TOTAL_BITCOIN_SUPPLY_SATOSHIS: u64 = 21_000_000 * 1_0000_0000;
  
  /// The maximum network dust limit for standard script formats. This currently represents the
  /// minimum output value for a P2SH output before Bitcoin Core 22 considers the entire
@@@ -865,11 -850,8 +865,11 @@@ impl<Signer: Sign> Channel<Signer> 
  		let holder_signer = keys_provider.get_channel_signer(false, channel_value_satoshis);
  		let pubkeys = holder_signer.pubkeys().clone();
  
 -		if channel_value_satoshis >= MAX_FUNDING_SATOSHIS {
 -			return Err(APIError::APIMisuseError{err: format!("funding_value must be smaller than {}, it was {}", MAX_FUNDING_SATOSHIS, channel_value_satoshis)});
 +		if !their_features.supports_wumbo() && channel_value_satoshis > MAX_FUNDING_SATOSHIS_NO_WUMBO {
 +			return Err(APIError::APIMisuseError{err: format!("funding_value must not exceed {}, it was {}", MAX_FUNDING_SATOSHIS_NO_WUMBO, channel_value_satoshis)});
 +		}
 +		if channel_value_satoshis >= TOTAL_BITCOIN_SUPPLY_SATOSHIS {
 +			return Err(APIError::APIMisuseError{err: format!("funding_value must be smaller than the total bitcoin supply, it was {}", channel_value_satoshis)});
  		}
  		let channel_value_msat = channel_value_satoshis * 1000;
  		if push_msat > channel_value_msat {
@@@ -1094,23 -1076,19 +1094,22 @@@
  		}
  
  		// Check sanity of message fields:
 -		if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS {
 -			return Err(ChannelError::Close(format!("Funding must be smaller than {}. It was {}", MAX_FUNDING_SATOSHIS, msg.funding_satoshis)));
 +		if msg.funding_satoshis > config.peer_channel_config_limits.max_funding_satoshis {
 +			return Err(ChannelError::Close(format!("Per our config, funding must be at most {}. It was {}", config.peer_channel_config_limits.max_funding_satoshis, msg.funding_satoshis)));
 +		}
 +		if msg.funding_satoshis >= TOTAL_BITCOIN_SUPPLY_SATOSHIS {
 +			return Err(ChannelError::Close(format!("Funding must be smaller than the total bitcoin supply. It was {}", msg.funding_satoshis)));
  		}
  		if msg.channel_reserve_satoshis > msg.funding_satoshis {
  			return Err(ChannelError::Close(format!("Bogus channel_reserve_satoshis ({}). Must be not greater than funding_satoshis: {}", msg.channel_reserve_satoshis, msg.funding_satoshis)));
  		}
- 		let funding_value = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
- 		if msg.push_msat > funding_value {
- 			return Err(ChannelError::Close(format!("push_msat {} was larger than funding value {}", msg.push_msat, funding_value)));
+ 		let full_channel_value_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
+ 		if msg.push_msat > full_channel_value_msat {
+ 			return Err(ChannelError::Close(format!("push_msat {} was larger than channel amount minus reserve ({})", msg.push_msat, full_channel_value_msat)));
  		}
  		if msg.dust_limit_satoshis > msg.funding_satoshis {
  			return Err(ChannelError::Close(format!("dust_limit_satoshis {} was larger than funding_satoshis {}. Peer never wants payout outputs?", msg.dust_limit_satoshis, msg.funding_satoshis)));
  		}
- 		let full_channel_value_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000;
  		if msg.htlc_minimum_msat >= full_channel_value_msat {
  			return Err(ChannelError::Close(format!("Minimum htlc value ({}) was larger than full channel value ({})", msg.htlc_minimum_msat, full_channel_value_msat)));
  		}
@@@ -1164,6 -1142,9 +1163,9 @@@
  		if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
  			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
  		}
+ 		if holder_selected_channel_reserve_satoshis * 1000 >= full_channel_value_msat {
+ 			return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). Channel value is ({} - {}).", holder_selected_channel_reserve_satoshis, full_channel_value_msat, msg.push_msat)));
+ 		}
  		if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
  			log_debug!(logger, "channel_reserve_satoshis ({}) is smaller than our dust limit ({}). We can broadcast stale states without any risk, implying this channel is very insecure for our counterparty.",
  				msg.channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS);
@@@ -2351,39 -2332,40 +2353,39 @@@
  		stats
  	}
  
 -	/// Get the available (ie not including pending HTLCs) inbound and outbound balance in msat.
 +	/// Get the available balances, see [`AvailableBalances`]'s fields for more info.
  	/// Doesn't bother handling the
  	/// if-we-removed-it-already-but-haven't-fully-resolved-they-can-still-send-an-inbound-HTLC
  	/// corner case properly.
 -	/// The channel reserve is subtracted from each balance.
 -	/// See also [`Channel::get_balance_msat`]
 -	pub fn get_inbound_outbound_available_balance_msat(&self) -> (u64, u64) {
 +	pub fn get_available_balances(&self) -> AvailableBalances {
  		// Note that we have to handle overflow due to the above case.
 -		(
 -			cmp::max(self.channel_value_satoshis as i64 * 1000
 -				- self.value_to_self_msat as i64
 -				- self.get_inbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64
 -				- self.holder_selected_channel_reserve_satoshis as i64 * 1000,
 -			0) as u64,
 -			cmp::max(self.value_to_self_msat as i64
 -				- self.get_outbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64
 -				- self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,
 -			0) as u64
 -		)
 -	}
 +		let outbound_stats = self.get_outbound_pending_htlc_stats(None);
  
 -	/// Get our total balance in msat.
 -	/// This is the amount that would go to us if we close the channel, ignoring any on-chain fees.
 -	/// See also [`Channel::get_inbound_outbound_available_balance_msat`]
 -	pub fn get_balance_msat(&self) -> u64 {
 -		// Include our local balance, plus any inbound HTLCs we know the preimage for, minus any
 -		// HTLCs sent or which will be sent after commitment signed's are exchanged.
  		let mut balance_msat = self.value_to_self_msat;
  		for ref htlc in self.pending_inbound_htlcs.iter() {
  			if let InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(_)) = htlc.state {
  				balance_msat += htlc.amount_msat;
  			}
  		}
 -		balance_msat - self.get_outbound_pending_htlc_stats(None).pending_htlcs_value_msat
 +		balance_msat -= outbound_stats.pending_htlcs_value_msat;
 +
 +		let outbound_capacity_msat = cmp::max(self.value_to_self_msat as i64
 +				- outbound_stats.pending_htlcs_value_msat as i64
 +				- self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0) as i64 * 1000,
 +			0) as u64;
 +		AvailableBalances {
 +			inbound_capacity_msat: cmp::max(self.channel_value_satoshis as i64 * 1000
 +					- self.value_to_self_msat as i64
 +					- self.get_inbound_pending_htlc_stats(None).pending_htlcs_value_msat as i64
 +					- self.holder_selected_channel_reserve_satoshis as i64 * 1000,
 +				0) as u64,
 +			outbound_capacity_msat,
 +			next_outbound_htlc_limit_msat: cmp::max(cmp::min(outbound_capacity_msat as i64,
 +					self.counterparty_max_htlc_value_in_flight_msat as i64
 +						- outbound_stats.pending_htlcs_value_msat as i64),
 +				0) as u64,
 +			balance_msat,
 +		}
  	}
  
  	pub fn get_holder_counterparty_selected_channel_reserve_satoshis(&self) -> (u64, Option<u64>) {
@@@ -4130,7 -4112,7 +4132,7 @@@
  		if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() {
  			return Err(ChannelError::Close("Remote end sent us a closing_signed while there were still pending HTLCs".to_owned()));
  		}
 -		if msg.fee_satoshis > 21_000_000 * 1_0000_0000 { //this is required to stop potential overflow in build_closing_transaction
 +		if msg.fee_satoshis > TOTAL_BITCOIN_SUPPLY_SATOSHIS { // this is required to stop potential overflow in build_closing_transaction
  			return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned()));
  		}
  
@@@ -6337,7 -6319,7 +6339,7 @@@ mod tests 
  	use ln::PaymentHash;
  	use ln::channelmanager::{HTLCSource, PaymentId};
  	use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
 -	use ln::channel::MAX_FUNDING_SATOSHIS;
 +	use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
  	use ln::features::InitFeatures;
  	use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
  	use ln::script::ShutdownScript;
@@@ -6373,10 -6355,9 +6375,10 @@@
  	}
  
  	#[test]
 -	fn test_max_funding_satoshis() {
 -		assert!(MAX_FUNDING_SATOSHIS <= 21_000_000 * 100_000_000,
 -		        "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
 +	fn test_max_funding_satoshis_no_wumbo() {
 +		assert_eq!(TOTAL_BITCOIN_SUPPLY_SATOSHIS, 21_000_000 * 100_000_000);
 +		assert!(MAX_FUNDING_SATOSHIS_NO_WUMBO <= TOTAL_BITCOIN_SUPPLY_SATOSHIS,
 +		        "MAX_FUNDING_SATOSHIS_NO_WUMBO is greater than all satoshis in existence");
  	}
  
  	#[test]
diff --combined lightning/src/ln/functional_tests.rs
index d9756035,aa2551f3..60fa9c80
--- a/lightning/src/ln/functional_tests.rs
+++ b/lightning/src/ln/functional_tests.rs
@@@ -58,12 -58,9 +58,12 @@@ use ln::chan_utils::CommitmentTransacti
  #[test]
  fn test_insane_channel_opens() {
  	// Stand up a network of 2 nodes
 +	use ln::channel::TOTAL_BITCOIN_SUPPLY_SATOSHIS;
 +	let mut cfg = UserConfig::default();
 +	cfg.peer_channel_config_limits.max_funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS + 1;
  	let chanmon_cfgs = create_chanmon_cfgs(2);
  	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 -	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 +	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(cfg)]);
  	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
  	// Instantiate channel parameters where we push the maximum msats given our
@@@ -95,15 -92,15 +95,15 @@@
  		} else { assert!(false); }
  	};
  
 -	use ln::channel::MAX_FUNDING_SATOSHIS;
  	use ln::channelmanager::MAX_LOCAL_BREAKDOWN_TIMEOUT;
  
  	// Test all mutations that would make the channel open message insane
 -	insane_open_helper(format!("Funding must be smaller than {}. It was {}", MAX_FUNDING_SATOSHIS, MAX_FUNDING_SATOSHIS).as_str(), |mut msg| { msg.funding_satoshis = MAX_FUNDING_SATOSHIS; msg });
 +	insane_open_helper(format!("Per our config, funding must be at most {}. It was {}", TOTAL_BITCOIN_SUPPLY_SATOSHIS + 1, TOTAL_BITCOIN_SUPPLY_SATOSHIS + 2).as_str(), |mut msg| { msg.funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS + 2; msg });
 +	insane_open_helper(format!("Funding must be smaller than the total bitcoin supply. It was {}", TOTAL_BITCOIN_SUPPLY_SATOSHIS).as_str(), |mut msg| { msg.funding_satoshis = TOTAL_BITCOIN_SUPPLY_SATOSHIS; msg });
  
  	insane_open_helper("Bogus channel_reserve_satoshis", |mut msg| { msg.channel_reserve_satoshis = msg.funding_satoshis + 1; msg });
  
- 	insane_open_helper(r"push_msat \d+ was larger than funding value \d+", |mut msg| { msg.push_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 + 1; msg });
+ 	insane_open_helper(r"push_msat \d+ was larger than channel amount minus reserve \(\d+\)", |mut msg| { msg.push_msat = (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 + 1; msg });
  
  	insane_open_helper("Peer never wants payout outputs?", |mut msg| { msg.dust_limit_satoshis = msg.funding_satoshis + 1 ; msg });
  
@@@ -116,25 -113,6 +116,25 @@@
  	insane_open_helper("max_accepted_htlcs was 484. It must not be larger than 483", |mut msg| { msg.max_accepted_htlcs = 484; msg });
  }
  
 +#[test]
 +fn test_funding_exceeds_no_wumbo_limit() {
 +	// Test that if a peer does not support wumbo channels, we'll refuse to open a wumbo channel to
 +	// them.
 +	use ln::channel::MAX_FUNDING_SATOSHIS_NO_WUMBO;
 +	let chanmon_cfgs = create_chanmon_cfgs(2);
 +	let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 +	node_cfgs[1].features = InitFeatures::known().clear_wumbo();
 +	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 +	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 +
 +	match nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), MAX_FUNDING_SATOSHIS_NO_WUMBO + 1, 0, 42, None) {
 +		Err(APIError::APIMisuseError { err }) => {
 +			assert_eq!(format!("funding_value must not exceed {}, it was {}", MAX_FUNDING_SATOSHIS_NO_WUMBO, MAX_FUNDING_SATOSHIS_NO_WUMBO + 1), err);
 +		},
 +		_ => panic!()
 +	}
 +}
 +
  fn do_test_counterparty_no_reserve(send_from_initiator: bool) {
  	// A peer providing a channel_reserve_satoshis of 0 (or less than our dust limit) is insecure,
  	// but only for them. Because some LSPs do it with some level of trust of the clients (for a