From 2df07277bb06d81236875a35b03662d89192ecbc Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Mon, 16 Mar 2020 16:51:29 +0100 Subject: [PATCH] Harden requirements on htlc-minimum-msat (#1339) We were allowing users to set htlc-minimum-msat to 0, which directly contradicts the fact that we must never send an HTLC for 0 msat. We now explicitly disallow that behavior: the minimum is 1 msat. In case the remote side of a channel had set its htlc-minimum-msat to 0, we would forward HTLC with a value of 0 msat if a sender crafted such a payment. The spec disallows that, so we now explicitly check for that lower bound. --- .../src/main/scala/fr/acinq/eclair/NodeParams.scala | 5 ++++- .../scala/fr/acinq/eclair/channel/Commitments.scala | 8 ++++---- .../test/scala/fr/acinq/eclair/StartupSpec.scala | 5 +++++ .../eclair/channel/states/e/NormalStateSpec.scala | 13 +++++++++++++ 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala b/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala index ebb1dedf9..06104f457 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala @@ -161,6 +161,9 @@ object NodeParams { require(dustLimitSatoshis >= Channel.MIN_DUSTLIMIT, s"dust limit must be greater than ${Channel.MIN_DUSTLIMIT}") } + val htlcMinimum = MilliSatoshi(config.getInt("htlc-minimum-msat")) + require(htlcMinimum > 0.msat, "htlc-minimum-msat must be strictly greater than 0") + val maxAcceptedHtlcs = config.getInt("max-accepted-htlcs") require(maxAcceptedHtlcs <= Channel.MAX_ACCEPTED_HTLCS, s"max-accepted-htlcs must be lower than ${Channel.MAX_ACCEPTED_HTLCS}") @@ -242,7 +245,7 @@ object NodeParams { maxAcceptedHtlcs = maxAcceptedHtlcs, expiryDeltaBlocks = expiryDeltaBlocks, fulfillSafetyBeforeTimeoutBlocks = fulfillSafetyBeforeTimeoutBlocks, - htlcMinimum = MilliSatoshi(config.getInt("htlc-minimum-msat")), + htlcMinimum = htlcMinimum, toRemoteDelayBlocks = CltvExpiryDelta(config.getInt("to-remote-delay-blocks")), maxToLocalDelayBlocks = CltvExpiryDelta(config.getInt("max-to-local-delay-blocks")), minDepthBlocks = config.getInt("mindepth-blocks"), diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala index d13eb93e7..9d48927e5 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala @@ -175,8 +175,8 @@ object Commitments { return Failure(ExpiryTooBig(commitments.channelId, maximum = maxExpiry, actual = cmd.cltvExpiry, blockCount = blockHeight)) } - if (cmd.amount < commitments.remoteParams.htlcMinimum) { - return Failure(HtlcValueTooSmall(commitments.channelId, minimum = commitments.remoteParams.htlcMinimum, actual = cmd.amount)) + if (cmd.amount < commitments.remoteParams.htlcMinimum.max(1 msat)) { + return Failure(HtlcValueTooSmall(commitments.channelId, minimum = commitments.remoteParams.htlcMinimum.max(1 msat), actual = cmd.amount)) } // let's compute the current commitment *as seen by them* with this change taken into account @@ -225,8 +225,8 @@ object Commitments { throw UnexpectedHtlcId(commitments.channelId, expected = commitments.remoteNextHtlcId, actual = add.id) } - if (add.amountMsat < commitments.localParams.htlcMinimum) { - throw HtlcValueTooSmall(commitments.channelId, minimum = commitments.localParams.htlcMinimum, actual = add.amountMsat) + if (add.amountMsat < commitments.localParams.htlcMinimum.max(1 msat)) { + throw HtlcValueTooSmall(commitments.channelId, minimum = commitments.localParams.htlcMinimum.max(1 msat), actual = add.amountMsat) } // let's compute the current commitment *as seen by us* including this change diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala index 62136305b..4bd4f4a9c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/StartupSpec.scala @@ -82,4 +82,9 @@ class StartupSpec extends FunSuite { assert(Try(makeNodeParamsWithDefaults(illegalFeaturesConf.withFallback(defaultConf))).isFailure) } + test("NodeParams should fail if htlc-minimum-msat is set to 0") { + val noHtlcMinimumConf = ConfigFactory.parseString("htlc-minimum-msat = 0") + assert(Try(makeNodeParamsWithDefaults(noHtlcMinimumConf.withFallback(defaultConf))).isFailure) + } + } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index 86456652b..f08e0684f 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala @@ -174,6 +174,19 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { alice2bob.expectNoMsg(200 millis) } + test("recv CMD_ADD_HTLC (0 msat)") { f => + import f._ + val sender = TestProbe() + // Alice has a minimum set to 0 msat (which should be invalid, but may mislead Bob into relaying 0-value HTLCs which is forbidden by the spec). + assert(alice.stateData.asInstanceOf[DATA_NORMAL].commitments.localParams.htlcMinimum === 0.msat) + val initialState = bob.stateData.asInstanceOf[DATA_NORMAL] + val add = CMD_ADD_HTLC(0 msat, randomBytes32, CltvExpiryDelta(144).toCltvExpiry(currentBlockHeight), TestConstants.emptyOnionPacket, Upstream.Local(UUID.randomUUID())) + sender.send(bob, add) + val error = HtlcValueTooSmall(channelId(bob), 1 msat, 0 msat) + sender.expectMsg(Failure(AddHtlcFailed(channelId(bob), add.paymentHash, error, Origin.Local(add.upstream.asInstanceOf[Upstream.Local].id, Some(sender.ref)), Some(initialState.channelUpdate), Some(add)))) + bob2alice.expectNoMsg(200 millis) + } + test("recv CMD_ADD_HTLC (increasing balance but still below reserve)", Tag("no_push_msat")) { f => import f._ val sender = TestProbe()