Merge pull request #1454 from TheBlueMatt/2022-04-fuzz-underflow
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Thu, 28 Apr 2022 21:56:49 +0000 (21:56 +0000)
committerGitHub <noreply@github.com>
Thu, 28 Apr 2022 21:56:49 +0000 (21:56 +0000)
Reject channels if the total reserves are larger than the funding

1  2 
lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs

index cf70749ad96b7bbc98fdca8917db77c0d03d601e,60ff1ded1ad99220686729be383ea8967869ec68..1cb7a689a21a1b710413e93afbba8e1881d48e90
@@@ -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 {
                }
  
                // 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)));
                }
                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);
                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>) {
                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;
        }
  
        #[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]
index d9756035f7813794eaf515c0537249b343cd3209,aa2551f3707c8447166f6add24b5571559167e28..60fa9c805052925194866182b327591fc5a0e2b0
@@@ -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
                } 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 });
  
        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