Check UpdateAddHTLC::skimmed_fee_msat on receive

Make sure the penultimate hop took the amount of fee that they claimed to take.
Without checking this TLV, we're heavily relying on the receiving wallet code
to correctly implement logic to calculate that that the fee is as expected.
This commit is contained in:
Valentine Wallace 2023-05-24 19:21:21 -04:00
parent 4cee62233c
commit 0d94d9f9b7
No known key found for this signature in database
GPG key ID: FD3E106A2CE099B4

View file

@ -2528,7 +2528,8 @@ where
fn construct_recv_pending_htlc_info(
&self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], payment_hash: PaymentHash,
amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool
amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool,
counterparty_skimmed_fee_msat: Option<u64>,
) -> Result<PendingHTLCInfo, ReceiveError> {
// final_incorrect_cltv_expiry
if hop_data.outgoing_cltv_value > cltv_expiry {
@ -2555,7 +2556,10 @@ where
msg: "The final CLTV expiry is too soon to handle",
});
}
if !allow_underpay && hop_data.amt_to_forward > amt_msat {
if (!allow_underpay && hop_data.amt_to_forward > amt_msat) ||
(allow_underpay && hop_data.amt_to_forward >
amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0)))
{
return Err(ReceiveError {
err_code: 19,
err_data: amt_msat.to_be_bytes().to_vec(),
@ -2622,7 +2626,7 @@ where
incoming_amt_msat: Some(amt_msat),
outgoing_amt_msat: hop_data.amt_to_forward,
outgoing_cltv_value: hop_data.outgoing_cltv_value,
skimmed_fee_msat: None,
skimmed_fee_msat: counterparty_skimmed_fee_msat,
})
}
@ -2861,7 +2865,7 @@ where
onion_utils::Hop::Receive(next_hop_data) => {
// OUR PAYMENT!
match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash,
msg.amount_msat, msg.cltv_expiry, None, allow_underpay)
msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat)
{
Ok(info) => {
// Note that we could obviously respond immediately with an update_fulfill_htlc
@ -3680,7 +3684,7 @@ where
onion_utils::Hop::Receive(hop_data) => {
match self.construct_recv_pending_htlc_info(hop_data,
incoming_shared_secret, payment_hash, outgoing_amt_msat,
outgoing_cltv_value, Some(phantom_shared_secret), false)
outgoing_cltv_value, Some(phantom_shared_secret), false, None)
{
Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, prev_user_channel_id, vec![(info, prev_htlc_id)])),
Err(ReceiveError { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret))
@ -3931,6 +3935,8 @@ where
htlcs.iter_mut().for_each(|htlc| htlc.total_value_received = Some(amount_msat));
let counterparty_skimmed_fee_msat = htlcs.iter()
.map(|htlc| htlc.counterparty_skimmed_fee_msat.unwrap_or(0)).sum();
debug_assert!(total_value.saturating_sub(amount_msat) <=
counterparty_skimmed_fee_msat);
new_events.push_back((events::Event::PaymentClaimable {
receiver_node_id: Some(receiver_node_id),
payment_hash,
@ -9743,6 +9749,50 @@ mod tests {
get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, last_random_pk);
}
#[test]
fn reject_excessively_underpaying_htlcs() {
let chanmon_cfg = create_chanmon_cfgs(1);
let node_cfg = create_node_cfgs(1, &chanmon_cfg);
let node_chanmgr = create_node_chanmgrs(1, &node_cfg, &[None]);
let node = create_network(1, &node_cfg, &node_chanmgr);
let sender_intended_amt_msat = 100;
let extra_fee_msat = 10;
let hop_data = msgs::OnionHopData {
amt_to_forward: 100,
outgoing_cltv_value: 42,
format: msgs::OnionHopDataFormat::FinalNode {
keysend_preimage: None,
payment_metadata: None,
payment_data: Some(msgs::FinalOnionHopData {
payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
}),
}
};
// Check that if the amount we received + the penultimate hop extra fee is less than the sender
// intended amount, we fail the payment.
if let Err(crate::ln::channelmanager::ReceiveError { err_code, .. }) =
node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat))
{
assert_eq!(err_code, 19);
} else { panic!(); }
// If amt_received + extra_fee is equal to the sender intended amount, we're fine.
let hop_data = msgs::OnionHopData { // This is the same hop_data as above, OnionHopData doesn't implement Clone
amt_to_forward: 100,
outgoing_cltv_value: 42,
format: msgs::OnionHopDataFormat::FinalNode {
keysend_preimage: None,
payment_metadata: None,
payment_data: Some(msgs::FinalOnionHopData {
payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat,
}),
}
};
assert!(node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat)).is_ok());
}
#[cfg(anchors)]
#[test]
fn test_anchors_zero_fee_htlc_tx_fallback() {