Don't send channel updates for private chans on error 2022-02-multi-node-review-mark
authorValentine Wallace <vwallace@protonmail.com>
Fri, 4 Feb 2022 20:21:01 +0000 (15:21 -0500)
committerValentine Wallace <vwallace@protonmail.com>
Mon, 14 Feb 2022 19:25:55 +0000 (14:25 -0500)
This commit also adds additional checks for the second-to-last (phantom) hop for phantom payments.

lightning/src/ln/channelmanager.rs

index caa5fba0336f1d3ed8da6cfe435b55b95918f6ec..0a0407eacffc3ce1d5b307e8f974ad57f1618369 100644 (file)
@@ -2293,53 +2293,59 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
                                let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
                                if let Some((err, code, chan_update)) = loop {
-                                       let forwarding_id = match id_option {
+                                       let forwarding_id_opt = match id_option {
                                                None => { // unknown_next_peer
                                                        // Note that this is likely a timing oracle for detecting whether an scid is a
                                                        // phantom.
                                                        if fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, *short_channel_id) {
-                                                               break None
+                                                               None
+                                                       } else {
+                                                               break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
                                                        }
-                                                       break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
                                                },
-                                               Some(id) => id.clone(),
+                                               Some(id) => Some(id.clone()),
                                        };
+                                       let (chan_update_opt, forwardee_cltv_expiry_delta) = if let Some(forwarding_id) = forwarding_id_opt {
+                                               let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();
+                                               // Leave channel updates as None for private channels.
+                                               let chan_update_opt = if chan.should_announce() {
+                                                       Some(self.get_channel_update_for_unicast(chan).unwrap()) } else { None };
+                                               if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
+                                                       // Note that the behavior here should be identical to the above block - we
+                                                       // should NOT reveal the existence or non-existence of a private channel if
+                                                       // we don't allow forwards outbound over them.
+                                                       break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
+                                               }
 
-                                       let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();
-
-                                       if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
-                                               // Note that the behavior here should be identical to the above block - we
-                                               // should NOT reveal the existence or non-existence of a private channel if
-                                               // we don't allow forwards outbound over them.
-                                               break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
-                                       }
+                                               // Note that we could technically not return an error yet here and just hope
+                                               // that the connection is reestablished or monitor updated by the time we get
+                                               // around to doing the actual forward, but better to fail early if we can and
+                                               // hopefully an attacker trying to path-trace payments cannot make this occur
+                                               // on a small/per-node/per-channel scale.
+                                               if !chan.is_live() { // channel_disabled
+                                                       break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, chan_update_opt));
+                                               }
+                                               if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
+                                                       break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, chan_update_opt));
+                                               }
+                                               let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64)
+                                                       .and_then(|prop_fee| { (prop_fee / 1000000)
+                                                       .checked_add(chan.get_outbound_forwarding_fee_base_msat() as u64) });
+                                               if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
+                                                       break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, chan_update_opt));
+                                               }
+                                               (chan_update_opt, chan.get_cltv_expiry_delta())
+                                       } else { (None, MIN_CLTV_EXPIRY_DELTA) };
 
-                                       // Note that we could technically not return an error yet here and just hope
-                                       // that the connection is reestablished or monitor updated by the time we get
-                                       // around to doing the actual forward, but better to fail early if we can and
-                                       // hopefully an attacker trying to path-trace payments cannot make this occur
-                                       // on a small/per-node/per-channel scale.
-                                       if !chan.is_live() { // channel_disabled
-                                               break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update_for_unicast(chan).unwrap())));
-                                       }
-                                       if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
-                                               break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update_for_unicast(chan).unwrap())));
-                                       }
-                                       let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64)
-                                               .and_then(|prop_fee| { (prop_fee / 1000000)
-                                               .checked_add(chan.get_outbound_forwarding_fee_base_msat() as u64) });
-                                       if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
-                                               break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update_for_unicast(chan).unwrap())));
-                                       }
-                                       if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + chan.get_cltv_expiry_delta() as u64 { // incorrect_cltv_expiry
-                                               break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update_for_unicast(chan).unwrap())));
+                                       if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + forwardee_cltv_expiry_delta as u64 { // incorrect_cltv_expiry
+                                               break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, chan_update_opt));
                                        }
                                        let cur_height = self.best_block.read().unwrap().height() + 1;
                                        // Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
                                        // but we want to be robust wrt to counterparty packet sanitization (see
                                        // HTLC_FAIL_BACK_BUFFER rationale).
                                        if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
-                                               break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
+                                               break Some(("CLTV expiry is too close", 0x1000 | 14, chan_update_opt));
                                        }
                                        if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
                                                break Some(("CLTV expiry is too far in the future", 21, None));
@@ -2353,7 +2359,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        // but there is no need to do that, and since we're a bit conservative with our
                                        // risk threshold it just results in failing to forward payments.
                                        if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
-                                               break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
+                                               break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, chan_update_opt));
                                        }
 
                                        break None;