lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.64k stars 2.07k forks source link

[bug]:BOLT4(11) - Not Spec conform when sending more than twice of the amount specified in the invoice #7634

Open ziggie1984 opened 1 year ago

ziggie1984 commented 1 year ago

The Bolt4 specification states:

if the amount paid is more than twice the amount expected:
SHOULD fail the HTLC.
SHOULD return an incorrect_or_unknown_payment_details error.
Note: this allows the origin node to reduce information leakage by altering the amount while not allowing for accidental gross overpayment.

https://github.com/lightning/bolts/blob/master/04-onion-routing.md#requirements-5

I checked with current master branch of lnd, I was able to successfully pay an amount greater than twice the amount specified in the invoice.

Steps to reproduce

Expected behaviour

LND should follow BOLT4 spec which states that only an amount twice as big should be accepted.

ziggie1984 commented 1 year ago

Checked with CLN, and they limit it according to the spec was trying to pay an 100msat invoice and acutally paying 150 sats, the payment failed:

2023-04-25T10:58:55.881Z DEBUG   lightningd: Attept to pay 329662dda5135c5b888df01d3d984d53a6e87957f4a23c0d10ebcccd232f9fec with amount 150000msat > 200msat
2023-04-25T10:58:55.881Z DEBUG   lightningd: WIRE_INCORRECT_OR_UNKNOWN_PAYMENT_DETAILS: lightningd/htlc_set.c:115
yyforyongyu commented 1 year ago

Nice test! It's worth noting that this is a SHOULD not a MUST tho, so won't say it's a bug.

morehouse commented 1 year ago

Regardless of what the spec says, I think this is a UI problem. We should at least warn the user if they try to send significantly more than the invoice amount. It is easy to accidentally type an extra digit and send 10x more than expected.

saubyk commented 1 year ago

Regardless of what the spec says, I think this is a UI problem. We should at least warn the user if they try to send significantly more than the invoice amount. It is easy to accidentally type an extra digit and send 10x more than expected.

Warning should be for any amount over the one specified in the invoice, as it's hard to qualify what significant is

ziggie1984 commented 1 year ago

Not sure if its an UI problem though, we are quite safe not allowing a user to overpay when specifying an invoice and an amount, it's basically not possible when we pay an invoice (with an amount specified). https://github.com/lightningnetwork/lnd/blob/master/lnrpc/routerrpc/router_backend.go#L676

I was really eyeing to be compliant with the spec tbh. I agree it's not a bug though.

As mentioned in the spec, I tried to add noise to the amt by just using the payment_secret and the payment_hash to build my own route and then using the gprc call SendToRouteV2.

The problem being is, that currently if we do not supply an invoice request lnd does not know how big the amount was in the first place but only the node which created the invoice knows it, and only the receiving node can essentially abort the payment preventing gross overpayments. So I find the spec quite reasonable.

carlaKC commented 1 year ago

I was really eyeing to be compliant with the spec tbh.

You're only not-compliant if you don't obey the MUST / MUST NOT keywords.

The problem being is, that currently if we do not supply an invoice request lnd does not know how big the amount was in the first place

Would say that it makes sense to add an "are you sure" message for lncli when overpaying a pay_req with a non-zero amount, otherwise don't worry about it (sender will likely reject it).