Skip to content

Commit

Permalink
better check_incoming_htlc_cltv
Browse files Browse the repository at this point in the history
  • Loading branch information
@Evanfeenstra
Evanfeenstra committed Nov 4, 2023
1 parent 27d41dc commit 5d38217
Showing 1 changed file with 53 additions and 49 deletions.
102 changes: 53 additions & 49 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2983,7 +2983,7 @@ where
},
Some((cp_id, id)) => Some((cp_id.clone(), id.clone())),
};
let chan_update_opt = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
let (chan_update_opt, check_min_cltv_expiry_delta) = if let Some((counterparty_node_id, forwarding_id)) = forwarding_chan_info_opt {
let per_peer_state = self.per_peer_state.read().unwrap();
let peer_state_mutex_opt = per_peer_state.get(&counterparty_node_id);
if peer_state_mutex_opt.is_none() {
Expand Down Expand Up @@ -3036,40 +3036,19 @@ where
if let Err((err, code)) = chan.htlc_satisfies_config(&msg, outgoing_amt_msat, outgoing_cltv_value) {
break Some((err, code, chan_update_opt));
}
chan_update_opt
// min_cltv_expiry_delta is already checked above in htlc_satisfies_config
(chan_update_opt, false)
} else {
if (msg.cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
// We really should set `incorrect_cltv_expiry` here but as we're not
// forwarding over a real channel we can't generate a channel_update
// for it. Instead we just return a generic temporary_node_failure.
break Some((
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
0x2000 | 2, None,
));
}
None
(None, true)
};

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, 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));
}
// If the HTLC expires ~now, don't bother trying to forward it to our
// counterparty. They should fail it anyway, but we don't want to bother with
// the round-trips or risk them deciding they definitely want the HTLC and
// force-closing to ensure they get it if we're offline.
// We previously had a much more aggressive check here which tried to ensure
// our counterparty receives an HTLC which has *our* risk threshold met on it,
// 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, chan_update_opt));

if let Err((err_msg, code, include_chan_update_opt)) = check_incoming_htlc_cltv(
cur_height, outgoing_cltv_value, msg.cltv_expiry, check_min_cltv_expiry_delta
) {
let chan_update_opt = if include_chan_update_opt { chan_update_opt } else { None };
break Some((err_msg, code, chan_update_opt));
}

break None;
Expand Down Expand Up @@ -7921,26 +7900,13 @@ where
});
};
let (next_packet_pubkey, _, _, outgoing_cltv_value) = next_packet_details_opt.unwrap();
// CLTV checks here. See these same checks in decode_update_add_htlc_onion for more context
let check_cltv = || -> Result<(), &str> {
if (msg.cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_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");
}
if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
return Err("CLTV expiry is too close");
}
if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
return Err("CLTV expiry is too far in the future");
}
if (outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
return Err("Outgoing CLTV value is too soon");
}
Ok(())
};
if let Err(err_msg) = check_cltv() {

if let Err((err_msg, code, _)) = check_incoming_htlc_cltv(
cur_height, outgoing_cltv_value, msg.cltv_expiry, true
) {
return Err(InboundOnionErr {
msg: err_msg,
err_code: 0x4000 | 22,
err_code: code,
err_data: Vec::new(),
});
}
Expand Down Expand Up @@ -8052,6 +8018,44 @@ where
Ok((next_hop, shared_secret, Some(next_packet_details)))
}

fn check_incoming_htlc_cltv(
cur_height: u32, outgoing_cltv_value: u32, cltv_expiry: u32, check_min_cltv_expiry_delta: bool
) -> Result<(), (&'static str, u16, bool)> {
if check_min_cltv_expiry_delta {
if (cltv_expiry as u64) < (outgoing_cltv_value) as u64 + MIN_CLTV_EXPIRY_DELTA as u64 {
// We really should set `incorrect_cltv_expiry` here but as we're not
// forwarding over a real channel we can't generate a channel_update
// for it. Instead we just return a generic temporary_node_failure.
return Err((
"Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta",
0x2000 | 2, false,
));
}
};
// 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 cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
return Err(("CLTV expiry is too close", 0x1000 | 14, true));
}
if cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
return Err(("CLTV expiry is too far in the future", 21, false));
}
// If the HTLC expires ~now, don't bother trying to forward it to our
// counterparty. They should fail it anyway, but we don't want to bother with
// the round-trips or risk them deciding they definitely want the HTLC and
// force-closing to ensure they get it if we're offline.
// We previously had a much more aggressive check here which tried to ensure
// our counterparty receives an HTLC which has *our* risk threshold met on it,
// 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 {
return Err(("Outgoing CLTV value is too soon", 0x1000 | 14, true));
}

Ok(())
}

impl<M: Deref, T: Deref, ES: Deref, NS: Deref, SP: Deref, F: Deref, R: Deref, L: Deref> MessageSendEventsProvider for ChannelManager<M, T, ES, NS, SP, F, R, L>
where
M::Target: chain::Watch<<SP::Target as SignerProvider>::Signer>,
Expand Down

0 comments on commit 5d38217

Please sign in to comment.