Allow nodes to overshoot final htlc amount and expiry

[t-bast]

When nodes receive HTLCs, they verify that the contents of those HTLCs match the intructions that the sender provided in the onion. It is important to ensure that intermediate nodes and final nodes have similar requirements, otherwise a malicious intermediate node could easily probe whether the next node is the final recipient or not.

Unfortunately, the requirements for intermediate nodes were more lenient than the requirements for final nodes. Intermediate nodes allowed overpaying and increasing the CLTV expiry, whereas final nodes required a perfect equality between the HTLC values and the onion values.

This provided a trivial way of probing: when relaying an HTLC, nodes could relay 1 msat more than what the onion instructed (or increase the outgoing expiry by 1). If the next node was an intermediate node, they would accept this HTLC, but if the next node was the recipient, they would reject it.

We update those requirements to fix this probing attack vector.

See lightning/bolts#1032

[pm47]

I'm struggling to understand how this plays with this previous provision from BOLT 4:

if the invoice specifies an amount:
MUST set total_msat to at least that amount, and less than or equal to twice amount.

[t-bast]

This is completely unrelated. The requirement you quote defines how you should set total_amount_msat depending on what the invoice contains.

What this PR does is about the relationship between the HTLC amount (from update_add_htlc) and the onion amount (not the total_amount_msat).

[pm47]

What this PR does is about the relationship between the HTLC amount (from update_add_htlc) and the onion amount (not the total_amount_msat).

Okay, this change applies to htlcs, not payments, it's a different layer. The original sender can overpay up to 2x, but intermediate nodes may overpay as much as they want. Receiver won't even see it, as it should only look at the final payload. But in practice it is a bit weird in our current code though, since in the following validation we consider the onion amount but we print the htlc amount :

eclair/eclair-core/src/main/scala/fr/acinq/eclair/payment/receive/MultiPartHandler.scala

Lines 382 to 388 in a8389a0

	if (payload.totalAmount < expectedAmount) {
	log.warning("received payment with amount too small for amount={} totalAmount={}", add.amountMsat, payload.totalAmount)
	false
	} else if (payload.totalAmount > expectedAmount * 2) {
	log.warning("received payment with amount too large for amount={} totalAmount={}", add.amountMsat, payload.totalAmount)
	false
	} else {

[codecov-commenter]

Codecov Report

Merging #2468 (a8389a0) into master (3b12475) will increase coverage by 0.04%.
The diff coverage is 92.10%.

❗ Current head a8389a0 differs from pull request most recent head 975cd7b. Consider uploading reports for the commit 975cd7b to get more accurate results

@@            Coverage Diff             @@
##           master    #2468      +/-   ##
==========================================
+ Coverage   84.92%   84.97%   +0.04%     
==========================================
  Files         198      198              
  Lines       15783    15812      +29     
  Branches      637      711      +74     
==========================================
+ Hits        13404    13436      +32     
+ Misses       2379     2376       -3     

Impacted Files	Coverage Δ
...in/scala/fr/acinq/eclair/channel/ChannelData.scala	98.11% <ø> (ø)
.../main/scala/fr/acinq/eclair/db/DualDatabases.scala	11.27% <0.00%> (+0.32%)	⬆️
.../src/main/scala/fr/acinq/eclair/db/NetworkDb.scala	100.00% <ø> (ø)
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	85.76% <50.00%> (-0.64%)	⬇️
...q/eclair/channel/fsm/ChannelOpenSingleFunded.scala	94.19% <80.00%> (ø)
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala	86.27% <81.81%> (+0.11%)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	95.05% <83.33%> (ø)
...fr/acinq/eclair/channel/InteractiveTxBuilder.scala	85.89% <86.36%> (+0.29%)	⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala	94.40% <87.23%> (-1.02%)	⬇️
.../scala/fr/acinq/eclair/message/OnionMessages.scala	81.81% <90.00%> (+5.45%)	⬆️
... and 29 more

[t-bast]

Okay, this change applies to htlcs, not payments, it's a different layer. The original sender can overpay up to 2x, but intermediate nodes may overpay as much as they want.

Yes, that is a good way of looking at it, this change is for the HTLC layer.

But in practice it is a bit weird in our current code though, since in the following validation we consider the onion amount but we print the htlc amount

This code snippet only verifies the spec quote from before: the onion total_amount_msat must be between the invoice amount and 2x that amount. It's unrelated to the current change. There is no reason to change anything here?

The MultiPartHandler then always uses the HTLC amount in the accounting we do, so since we validate that this amount is always greater than the amt_to_fwd provided in the onion, I believe we're doing everything correctly and storing exactly how much we actually received, don't you think?

[pm47]

The MultiPartHandler then always uses the HTLC amount in the accounting we do, so since we validate that this amount is always greater than the amt_to_fwd provided in the onion, I believe we're doing everything correctly and storing exactly how much we actually received, don't you think?

I guess before this PR we could (should for consistency?) have used payload.amount instead of add.amountMsat, but now we have to use add.amountMsat, because the last hop may have sent a higher amount. Wait, what if they sent a amount so high that it makes the payment valid in terms of total payment amount even if payload.totalAmount was too low. Fat fetch example, but doesn't it show the ambiguity here?

[t-bast]

I guess before this PR we could (should for consistency?) have used payload.amount instead of add.amountMsat, but now we have to use add.amountMsat, because the last hop may have sent a higher amount.

The way I think about this is that if you count using add.amountMsat, you are counting what you actually receive. If you count using payload.amount, you are counting what the sender wanted you to receive. The first option will always yield a greater value than the second option. From an accounting's perspective, I think the first option makes more sense, we don't really care that the sender intended to send less, do we? Even if we did, we could trivially track it in a separate value.

Wait, what if they sent a amount so high that it makes the payment valid in terms of total payment amount even if payload.totalAmount was too low. Fat fetch example, but doesn't it show the ambiguity here?

We verify payload.totalAmount against the invoice amount, so that would be rejected. We would be rejecting free money, but if payload.totalAmount is below the invoice amount, the sender is really buggy so it probably makes sense to reject the payment?

This is also a very far-fetched example: it would only happen if the previous node decides to just throw money at you (this is a net loss for them). We already let them do it if we're not the final node, so we definitely should let them do it if we're the final node!

[pm47]

Fair enough, I get it now.