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.

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -2311,7 +2311,7 @@ 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]>,
-		chan_retrieval_info: ChanMapRetrieval
+		counterparty_skimmed_fee_msat: Option<u64>, chan_retrieval_info: ChanMapRetrieval
 	) -> Result<PendingHTLCInfo, ReceiveError> {
 		// final_incorrect_cltv_expiry
 		if hop_data.outgoing_cltv_value > cltv_expiry {
@@ -2340,6 +2340,7 @@ where
 		}
 		if hop_data.amt_to_forward > amt_msat {
 			let allow_underpay = loop {
+				if counterparty_skimmed_fee_msat.is_none() { break false }
 				let (counterparty_node_id, channel_id) = match chan_retrieval_info {
 					ChanMapRetrieval::Scid(scid) => {
 						match self.short_to_chan_info.read().unwrap().get(&scid).cloned() {
@@ -2362,7 +2363,12 @@ where
 				};
 				break chan.config().accept_underpaying_htlcs
 			};
-			if !allow_underpay {
+			if !allow_underpay ||
+				// This check needs to match the extra fee calculation in
+				// [`Self::forward_intercepted_htlc`].
+				amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0)) !=
+					hop_data.amt_to_forward
+			{
 				return Err(ReceiveError {
 					err_code: 19,
 					err_data: amt_msat.to_be_bytes().to_vec(),
@@ -2494,7 +2500,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,
+					msg.amount_msat, msg.cltv_expiry, None, msg.skimmed_fee_msat,
 					ChanMapRetrieval::IdPubkey((*counterparty_node_id, msg.channel_id)))
 				{
 					Ok(info) => {
@@ -3404,7 +3410,9 @@ where
 												}
 											}
 										}
-										if let PendingHTLCRouting::Forward { onion_packet, .. } = routing {
+										if let PendingHTLCRouting::Forward {
+											onion_packet, skimmed_fee_msat, ..
+										} = routing {
 											let phantom_pubkey_res = self.node_signer.get_node_id(Recipient::PhantomNode);
 											if phantom_pubkey_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id, &self.genesis_hash) {
 												let phantom_shared_secret = self.node_signer.ecdh(Recipient::PhantomNode, &onion_packet.public_key.unwrap(), None).unwrap().secret_bytes();
@@ -3427,7 +3435,7 @@ where
 														match self.construct_recv_pending_htlc_info(hop_data,
 															incoming_shared_secret, payment_hash, outgoing_amt_msat,
 															outgoing_cltv_value, Some(phantom_shared_secret),
-															ChanMapRetrieval::Scid(prev_short_channel_id))
+															skimmed_fee_msat, ChanMapRetrieval::Scid(prev_short_channel_id))
 														{
 															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))
@@ -9292,6 +9300,55 @@ mod tests {
 		get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, last_random_pk);
 	}
 
+	#[test]
+	fn reject_excessively_underpaying_htlcs() {
+		let chanmon_cfgs = create_chanmon_cfgs(2);
+		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+		let mut underpay_config = test_default_channel_config();
+		underpay_config.channel_config.accept_underpaying_htlcs = true;
+		let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, Some(underpay_config)]);
+		let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+		let scid = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000, 0).0.contents.short_channel_id;
+		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, .. }) =
+			nodes[1].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
+				sender_intended_amt_msat - extra_fee_msat - 1, 42, None, Some(extra_fee_msat),
+				crate::ln::channelmanager::ChanMapRetrieval::Scid(scid))
+		{
+			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!(nodes[1].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]),
+			sender_intended_amt_msat - extra_fee_msat, 42, None, Some(extra_fee_msat),
+			crate::ln::channelmanager::ChanMapRetrieval::Scid(scid)).is_ok());
+	}
+
 	#[cfg(anchors)]
 	#[test]
 	fn test_anchors_zero_fee_htlc_tx_fallback() {