spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.38k stars 3.07k forks source link

user may lose funds in a submarine swap, if their channel is force closed. #8940

Open ecdsa opened 6 months ago

ecdsa commented 6 months ago

Scenario:

I do not think this is fixable; this is a consequence of the new protocol. (note: In the old protocol, Alice sends on-chain funds before she receives the HTLC. She has the preimage and settles the HTLC instantly).

Note that the GUI currently shows a dialog instructing the user to stay online until the funding transaction is confirmed. However, that dialog is shown when the user initiates the swap, and the risk might not be understood. I think we should also show a dialog when the GUI is closed.

If Alice shuts down her client while a swap is ongoing, the GUI could show a dialog offering several choices:

edit: cancelling the swap does not guarantee that the swap funding transaction will not be mined

SomberNight commented 6 months ago
  • Time passes, and the HTLC between Alice and Bob expires. Bob force closes the channel.
  • More time passes, and the CSV delay expires, Bob now redeems the HTLC.

There is no CSV delay (unless Bob broadcasts a revoked ctx). Timing out an offered HTLC in a ctx works by broadcasting an "HTLC-Timeout" tx - the only timelock involved works by the "HTLC-Timeout" tx being presigned with a specific nLocktime (which is set to the expiration time of the HTLC).


  • Time passes, and the HTLC between Alice and Bob expires. Bob force closes the channel.

We could increase the cltv_delta in the invoice created by Alice: https://github.com/spesmilo/electrum/blob/ff5048752839056b0dc81f6c3b5b3a67948dbcb6/electrum/lnworker.py#L2177 This value is currently ~1 day (147 blocks). We could increase it to 3-5 days. (increase it just for this specific use-case of normal swaps though, so it needs some refactoring)

SomberNight commented 6 months ago

note: there is already this text, though only in the desktop qt gui: https://github.com/spesmilo/electrum/blob/ff5048752839056b0dc81f6c3b5b3a67948dbcb6/electrum/gui/qt/main_window.py#L2693

ecdsa commented 6 months ago

note: there is already this text, though only in the desktop qt gui:

yes, this is what I was referring to above

SomberNight commented 6 months ago

I think we should adjust this text in qml - which was applicable to the old normal swap: https://github.com/spesmilo/electrum/blob/58a1bdfec7c2956121d3ccedada388e54b751ce1/electrum/gui/qml/qeswaphelper.py#L387-L392

ecdsa commented 6 months ago

Note: we cannot rely on displaying a message on exit, because it will not work on Android, where the app can be killed by the system. I suggest we display a warning icon next to the unconfirmed transaction in the history, and a message in the tx details. For consistency, we should do the same on qt.