Tie each cached preimage to its HTLC to avoid collision

[ariard]

ChannelMonitor::payment_preimages cache preimage by hash indifferently of its origin HTLC. It may be exploited in a really really loose way to trigger unintentional channel closure, so for peace of mind, tie each preimage to its HTLC.

[TheBlueMatt]

To describe the issue more concretely (guessing by your statements here, good catch!): We use self.payment_preimages.contains_key() in ChannelMonitor::would_broadcast_at_height to determine if an inbound HTLC is near expiry is worth failing-to-chain for. This is potentially exploitable which allows any node to potentially cause us to force-close a channel by:

1. routing an HTLC through us, and fulfilling it so that we learn the preimage,
2. sending a second HTLC with the same payment_hash through us and allowing it to time out.

As long as the second HTLC isn't claimed before it times out, we'll see that its getting close to timeout and see that we have the preimage and close the channel to chain to try to claim it.

[ariard]

Yes description is right. It's a bit more subtle, I don't think this is easy to trigger because the second pending incoming HTLC routed through us should have a corresponding outbound. This one should be time-out either offchain or onchain on outgoing link. Thanks to CLTV_DELTA we should have plenty of time to expire the incoming one off-chain, and remove HTLC from ChannelMonitor local commitment transaction view.

That said, other case is if HTLC is a final one, and application logic doesn't cancel properly a non-expected HTLC, it may trigger a force-close. So yes we should fix it.

[ariard]

While reviewing #597, it occurs to me we may claim an incoming HTLC for which we know the preimage without checking preimage is addressed for this HTLC (hash collision), therefore settling a payment path without downstream resolution. A preimage committed to cache is already assumed to have been revealed to outside world, but that's not great...

That's a different case than triggering a channel closure as mentioned above.

[TheBlueMatt]

Worth doing, but doesn't need to be in 0.1 I think.