fix(payments): a seller's own wallet could be used to get them banned - #819
Merged
Conversation
GET /api/v1/pay/... and POST /api/v1/payments/public each mint a REAL Lightning invoice through the RECIPIENT's own LNURL/NWC relay, on every request. Both were limited per IP and nothing else, so an attacker rotating addresses could drive unbounded invoice creation against one seller — getting them rate-limited or banned by their own wallet provider, and littering their queue with orphan intents. The victim is the person who did nothing. Per-IP answers "is one caller hammering us". It cannot answer the question that protects a seller: "is one RECIPIENT's wallet being hammered", however many addresses it arrives from. rateLimitTipRecipient was built for exactly this — 20 invoices per 5 minutes against any one recipient — and its own doc says to apply it IN ADDITION to the per-IP limit. It was wired to tips and lnurlp only. rateLimitPaymentRecipient addresses an entity rather than a username and shares that budget, because it is the same victim's wallet either way. Ordering matters and differs per route: the L402 challenge has the recipient in its path, so the check sits beside the per-IP one; the public-support route only learns the recipient after the body parses, so it sits after parsing and BEFORE initiatePublicSupport, which is the call that actually mints the invoice. The verify-with-proof branch is deliberately left alone — it creates no intent (finding 7 covers its outbound cost separately). Residual, stated rather than hidden: a profile reachable both by username and by entity id has two buckets, so splitting across both paths doubles the budget. Collapsing them needs a username->entity lookup on the rate-limit path, a database round trip before deciding whether to serve at all. Twice a bounded number is still bounded; unbounded was the bug. #563 finding 1. Unblocked by #816 — until the key stopped being caller-controlled, no limiter here could be tripped at all. Mutation-proven: removing the guard fails exactly the two new tests and nothing else; restoring it passes all 12. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes finding 1 of #563.
GET /api/v1/pay/...andPOST /api/v1/payments/publiceach mint a real Lightning invoice through the recipient's own LNURL/NWC relay, on every request. Both were limited per IP and nothing else.So an attacker rotating addresses could drive unbounded invoice creation against one seller — getting them rate-limited or banned by their own wallet provider, and littering their queue with orphan intents. The victim is the person who did nothing.
Per-IP answers "is one caller hammering us". It cannot answer the question that actually protects a seller: "is one recipient's wallet being hammered", however many addresses it arrives from.
The tool already existed
rateLimitTipRecipient— 20 invoices per 5 minutes against any one recipient — and its own doc comment says to apply it in addition to the per-IP limit. It was wired to tips and lnurlp only.rateLimitPaymentRecipientaddresses an entity rather than a username and shares that budget, because it's the same victim's wallet either way.Ordering differs per route, deliberately
initiatePublicSupport, which is the call that actually mints the invoice.The verify-with-proof branch is left alone: it creates no intent. Finding 7 covers its outbound cost separately.
Residual, stated rather than hidden
A profile reachable both by username and by entity id has two buckets, so an attacker splitting across both paths gets twice the budget. Collapsing them needs a username→entity lookup on the rate-limit path — a database round trip before deciding whether to serve at all. Twice a bounded number is still bounded; unbounded was the bug.
Verification
type-checkgreen,check:client-ipgreen, 20 tests across both route suites.Mutation-proven: removing the guard fails exactly the two new tests and nothing else; restoring it passes all 12.
Unblocked by #816 — until the limiter key stopped being caller-controlled, no limiter on these routes could be tripped at all.