Skip to content
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

Enforce a bounded max dust_limit_satoshis on remote transactions #575

Closed

Conversation

ariard
Copy link

@ariard ariard commented Apr 8, 2020

Close #287

/// Maximum allowed threshold above which outputs will not be generated in their commitment
/// transactions.
/// HTLCs below this amount plus HTLC transaction fees are not enforceable on-chain.
/// Maximum allowed dust_limit_satoshis for their commitment+HTLC transactions, above which
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change this? The new text seems even less readable - making reference to the particular name of the variable in the spec doesn't describe what it means, and is rather bad documentation.

Copy link
Author

Choose a reason for hiding this comment

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

Because comment is wrong, as far as I understand. Outputs will be generated if they are above the dust_limit_satoshis, but we won't accept channel if proposed value is above this "maximum allowed threshold".

On dust_limit_satoshis, its meaning is described in variable above min_dust_limit_satoshis, to which name proximity is enough to rely on a reader going through its comment too ?

Copy link
Author

Choose a reason for hiding this comment

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

Updated to make it clearer tho

///
/// Default value: u64::max_value.
/// Default value: 10*546
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. Why 10? Can you write up what the fee costs are of claiming an HTLC on-chain and then provide some guidance for what this value should be at different fee levels? eg if fees go up 100x, 10*546 would likely be pretty low, no?

Copy link
Author

Choose a reason for hiding this comment

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

Okay here full-rational.

Let's say Mallory announces a channel to Alice with dust-limit-satoshi set to 20% of channel value. Alice should accept incoming channels as long as its under her max_dust_limit_satoshis. Now Mallory can route 4 dust-HTLCs to Bob through Alice which will claim ~80% of channel value (dust_limit_satoshis MUST be under channel_reserve).

Bob, in collusion with Mallory, claim the whole set of HTLCs and at same moment Mallory broadcast her latest commitment transaction on which there is HTLCs because all of them are dust. Alice can't claim them onchain but has paid Bob forward.

As we talked before, this attack is at first-look not economically rational because dust-HTLCs are committed as fees. But if you assume Mallory can collude with some mining pool, economics change completely because it's a zero_cost to add Mallory commitment transaction in a block, hashrate won't be wasted. Fees are going back to miner, and Alice is still robbed. Mallory commitment transaction may stay in miner pool as long block isn't found and HTLCs timelocks don't expire. And attack may still stealth if block isn't signed.

It's that kind low-probability-and-hard-to-exploit-vulnerability but still you prefer not to have to think about your big LSP hub being targeted by a rogue mining pool employee.

To avoid this, max dust_limit_satoshis must be pretty low, max value open to exploitation is of course max_htlc_value_in_flight_msat but you can still offer dust-htlc in parallel to try to reach it. And as far as I remember we don't limit number of offered htlcs to us, as some implementation are doing.

Back to the pure fact of fee spikes, I think that's a spec default, dust_limit_satoshis should be a floating channel parameter to mirror dynamic conditions. Maybe we should take opportunity of dynamic channel upgrade introduced for anchor_ouputs to also integrate this. I'm not setup on 10*546 and we may have instead a max_trusted_dust_value from which to reason on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice writeup, can you put it in a doc here, though :). I agree with you - this should mostly be dynamic, which also implies we should tell the users how to set it (probably X*BackgroundFeerate).

re: max htlcs: we should expose it. its currently set by the OUR_MAX_HTLCs constant in channel.rs.

re: floating dust value: true, though it only kinda requries a spec update. Its somewhat anti-social, but in theory we could try to get this line removed from the spec (and only indicate that it must be irrevocably committed in the senders commitment tx) and then fail HTLCs below a floating dust limit. I'm not sure this is actually a good idea (we should, instead, just update the htlc_minimum_msat and then bounce dust HTLCs, really).

until the corresponding HTLC is irrevocably committed in both sides' commitment transactions: MUST NOT send an update_fulfill_htlc, update_fail_htlc, or update_fail_malformed_htlc.

Copy link
Contributor

@naumenkogs naumenkogs Apr 19, 2020

Choose a reason for hiding this comment

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

+1 to keep the whole bad miners reasoning (but really it's about aggregating dust) somewhere for the reference.

because it's a zero_cost to add Mallory commitment transaction in a block, hashrate won't be wasted

The opportunity to mine some other transaction is lost, right? (assuming the blocks are full). So technically there is a cost I think.

Overall I generally agree with everything.

Copy link
Author

Choose a reason for hiding this comment

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

The opportunity to mine some other transaction is lost, right? (assuming the blocks are full). So technically there is a cost I think.

With current static-fee model, commitment should have competitive fees, and you may not have to bear them if it's not your opening. But yeah assuming attacker is opening channel, there is a slight loss, but if you can reap until max_htlc_value_in_flight, and with wumbo_channel I expect this to increase with time.

Note, as with most LN attacks, attacker is limited by staking coins but for a mining-pool you likely assume they have fresh coins available.

Note, currently max CLTV network value is 2048 blocks IIRC, but even if you mining pool doesn't find a block in this range, you can reset a long-lived HTLC through the network even before block 2048+1. So this attack seems doable by any entity who has reasonable probability to mine a block ever ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any plans to update the doc?

Antoine Riard added 2 commits April 9, 2020 19:12
A too-high dust_limit_satoshis announced by remote party for their
commitment transaction may be exploited.
@ariard ariard force-pushed the 2020-04-checking-dust-inflation branch from 974875a to 31a9ee9 Compare April 9, 2020 23:12
@TheBlueMatt TheBlueMatt added this to the 0.0.12 milestone Sep 27, 2020
@ariard
Copy link
Author

ariard commented Oct 19, 2020

The logic of using a local policy not-announced to our counterparty protect us but there is no way for them to know when they override our max_dust_limit_satoshis. Thus it could trigger easily unexpected channel closure.

I'm working on proposing a spec update extending channel policy as a more robust fix.

@ariard ariard closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check dust inflation
3 participants