From 43bad4af9fa50d879678c00ab7dc939bd4ef7063 Mon Sep 17 00:00:00 2001
From: Joost Jager <joost.jager@gmail.com>
Date: Thu, 8 Aug 2019 16:25:25 +0200
Subject: [PATCH] invoices: always check htlc amt with invoice amount

Previously a check was made for accepted and settled invoices against
the paid amount. This opens up a probe vector where an attacker can pay
to an invoice with an amt that is higher than the invoice amount and
find out if the invoice is already paid or not.
---
 invoices/invoiceregistry.go      | 13 ++++---------
 invoices/invoiceregistry_test.go | 14 +++++++-------
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/invoices/invoiceregistry.go b/invoices/invoiceregistry.go
index a5570a3c8..2cbba30eb 100644
--- a/invoices/invoiceregistry.go
+++ b/invoices/invoiceregistry.go
@@ -437,10 +437,10 @@ func (i *InvoiceRegistry) checkHtlcParameters(invoice *channeldb.Invoice,
 		return channeldb.ErrInvoiceAlreadyCanceled
 	}
 
-	// If a payment has already been made, we only accept more payments if
-	// the amount is the exact same. This prevents probing with small
-	// amounts on settled invoices to find out the receiver node.
-	if invoice.AmtPaid != 0 && amtPaid != invoice.AmtPaid {
+	// If an invoice amount is specified, check that enough is paid. Also
+	// check this for duplicate payments if the invoice is already settled
+	// or accepted.
+	if invoice.Terms.Value > 0 && amtPaid < invoice.Terms.Value {
 		return ErrInvoiceAmountTooLow
 	}
 
@@ -468,11 +468,6 @@ func (i *InvoiceRegistry) checkHtlcParameters(invoice *channeldb.Invoice,
 		return ErrInvoiceExpiryTooSoon
 	}
 
-	// If an invoice amount is specified, check that enough is paid.
-	if invoice.Terms.Value > 0 && amtPaid < invoice.Terms.Value {
-		return ErrInvoiceAmountTooLow
-	}
-
 	return nil
 }
 
diff --git a/invoices/invoiceregistry_test.go b/invoices/invoiceregistry_test.go
index 990bcdc46..d8b8a5fe3 100644
--- a/invoices/invoiceregistry_test.go
+++ b/invoices/invoiceregistry_test.go
@@ -165,9 +165,9 @@ func TestSettleInvoice(t *testing.T) {
 		t.Fatal("expected settle event")
 	}
 
-	// Try to settle again with a higher amount. This should result in a
-	// cancel event because after a restart the amount should still be the
-	// same. New HTLCs with a different amount should be rejected.
+	// Try to settle again with a higher amount. This payment should also be
+	// accepted, to prevent any change in behaviour for a paid invoice that
+	// may open up a probe vector.
 	event, err = registry.NotifyExitHopHtlc(
 		hash, amtPaid+600, testHtlcExpiry, testCurrentHeight,
 		hodlChan, nil,
@@ -175,12 +175,12 @@ func TestSettleInvoice(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected NotifyExitHopHtlc error: %v", err)
 	}
-	if event.Preimage != nil {
-		t.Fatal("expected cancel event")
+	if event.Preimage == nil {
+		t.Fatal("expected settle event")
 	}
 
-	// Try to settle again with a lower amount. This should show the same
-	// behaviour as settling with a higher amount.
+	// Try to settle again with a lower amount. This should fail just as it
+	// would have failed if it were the first payment.
 	event, err = registry.NotifyExitHopHtlc(
 		hash, amtPaid-600, testHtlcExpiry, testCurrentHeight,
 		hodlChan, nil,