From 3f4dc0decd192ecdf5878fb554fdeebe3f9660b3 Mon Sep 17 00:00:00 2001
From: Conner Fromknecht <conner@lightning.engineering>
Date: Thu, 2 Apr 2020 09:31:51 -0700
Subject: [PATCH] lnwallet/channel: increase htlc validation strictness

This commit adds an additional santity check that rejects zero-value
HTLCs, preventing them from being added to the channel state even if the
channel config's minhtlc value is zero.
---
 lnwallet/channel.go      |  9 +++++++++
 lnwallet/channel_test.go | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/lnwallet/channel.go b/lnwallet/channel.go
index c08a90b1..96f53e3e 100644
--- a/lnwallet/channel.go
+++ b/lnwallet/channel.go
@@ -67,6 +67,10 @@ var (
 	ErrBelowMinHTLC = fmt.Errorf("proposed HTLC value is below minimum " +
 		"allowed HTLC value")
 
+	// ErrInvalidHTLCAmt signals that a proposed HTLC has a value that is
+	// not positive.
+	ErrInvalidHTLCAmt = fmt.Errorf("proposed HTLC value must be positive")
+
 	// ErrCannotSyncCommitChains is returned if, upon receiving a ChanSync
 	// message, the state machine deems that is unable to properly
 	// synchronize states with the remote peer. In this case we should fail
@@ -3234,6 +3238,11 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 				amtInFlight += entry.Amount
 				numInFlight++
 
+				// Check that the HTLC amount is positive.
+				if entry.Amount == 0 {
+					return ErrInvalidHTLCAmt
+				}
+
 				// Check that the value of the HTLC they added
 				// is above our minimum.
 				if entry.Amount < constraints.MinHTLC {
diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go
index f5c6fb35..c266db1c 100644
--- a/lnwallet/channel_test.go
+++ b/lnwallet/channel_test.go
@@ -6636,6 +6636,41 @@ func TestMinHTLC(t *testing.T) {
 	}
 }
 
+// TestInvalidHTLCAmt tests that ErrInvalidHTLCAmt is returned when trying to
+// add HTLCs that don't carry a positive value.
+func TestInvalidHTLCAmt(t *testing.T) {
+	t.Parallel()
+
+	// We'll kick off the test by creating our channels which both are
+	// loaded with 5 BTC each.
+	aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
+		channeldb.SingleFunderTweaklessBit,
+	)
+	if err != nil {
+		t.Fatalf("unable to create test channels: %v", err)
+	}
+	defer cleanUp()
+
+	// We'll set the min HTLC values for each party to zero, which
+	// technically would permit zero-value HTLCs.
+	aliceChannel.channelState.LocalChanCfg.MinHTLC = 0
+	bobChannel.channelState.RemoteChanCfg.MinHTLC = 0
+
+	// Create a zero-value HTLC.
+	htlcAmt := lnwire.MilliSatoshi(0)
+	htlc, _ := createHTLC(0, htlcAmt)
+
+	// Sending or receiving the HTLC should fail with ErrInvalidHTLCAmt.
+	_, err = aliceChannel.AddHTLC(htlc, nil)
+	if err != ErrInvalidHTLCAmt {
+		t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err)
+	}
+	_, err = bobChannel.ReceiveHTLC(htlc)
+	if err != ErrInvalidHTLCAmt {
+		t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err)
+	}
+}
+
 // TestNewBreachRetributionSkipsDustHtlcs ensures that in the case of a
 // contract breach, all dust HTLCs are ignored and not reflected in the
 // produced BreachRetribution struct. We ignore these HTLCs as they aren't