Skip to content

Validate amount_msats against invreq amount #3535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jan 14, 2025

Add a check to ensure that the amount_msats in an invoice matches the amount_msats specified in the invoice_request or offer (or refund). Reject the invoice as invalid if there is a mismatch between these amounts. Otherwise, an invoice may be paid with an amount greater than the requested amount.

@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Jan 14, 2025
@@ -1539,6 +1544,13 @@ impl TryFrom<PartialInvoiceTlvStream> for InvoiceContents {
experimental_offer_tlv_stream, experimental_invoice_request_tlv_stream,
)
)?;

if let Some(requested_amount_msats) = invoice_request.amount_msats() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check this against the offer's set amount as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fixed this and added tests as discussed offline. Now InvoiceRequest::amount_msats will infer the amount from the Offer::amount and InvoiceRequest::quantity, is possible. Added a new InvoiceRequest::has_amount_msats method as otherwise we can't determine whether the amount was explicitly set.

@TheBlueMatt TheBlueMatt mentioned this pull request Jan 15, 2025
@jkczyz jkczyz force-pushed the 2025-01-invoice-amount branch from bdf5dcb to 2ad61a0 Compare January 15, 2025 16:00
@jkczyz jkczyz changed the title Validate amount_msats against invoice and refund amounts Validate amount_msats against invreq amount Jan 15, 2025
@TheBlueMatt
Copy link
Collaborator

LGTM, feel free to squash.

@valentinewallace
Copy link
Contributor

CI is sad, I think it looks related to the PR

@jkczyz
Copy link
Contributor Author

jkczyz commented Jan 15, 2025

CI is sad, I think it looks related to the PR

Ah, right. I guess I need to rebase on main and fix it.

slanesuke and others added 2 commits January 15, 2025 11:23
Add a check to ensure that the amount_msats in an invoice matches the
amount_msats specified in the invoice_request or offer (or refund).
Reject the invoice as invalid if there is a mismatch between these
amounts. Otherwise, an invoice may be paid with an amount greater than
the requested amount.

Co-authored-by: Ian Slane <[email protected]>
Co-authored-by: Jeffrey Czyz <[email protected]>
When InvoiceRequest::amount_msats returns Some, it may have been
inferred from the Offer::amount and InvoiceRequest::quantity. Add a
method to InvoiceRequest for determining if the amount was explicitly
set.
@jkczyz jkczyz force-pushed the 2025-01-invoice-amount branch from 2ad61a0 to c2360be Compare January 15, 2025 17:23
@TheBlueMatt TheBlueMatt merged commit 6d604c5 into lightningdevkit:main Jan 15, 2025
16 of 25 checks passed
@TheBlueMatt
Copy link
Collaborator

Backported in #3536.

@@ -87,6 +87,14 @@ macro_rules! invoice_builder_methods_test { (
$self: ident, $self_type: ty, $invoice_fields: expr, $return_type: ty, $return_value: expr
$(, $self_mut: tt)?
) => {
#[cfg_attr(c_bindings, allow(dead_code))]
pub(crate) fn amount_msats_unchecked(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, just realized this breaks CI because it doesn't work for --cfg=async_payments unfortunately. Happy to look into a fix if that helps

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grr, yea, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants