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

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

Open
ecdsa opened this issue Mar 12, 2024 · 5 comments
Open

Comments

@ecdsa
Copy link
Member

ecdsa commented Mar 12, 2024

Scenario:

  • Alice has a channel with Bob
  • Alice initiates a normal swap with Carol.
  • Alice receives a HTLC from Carol, through Bob
  • Alice sends on-chain funds to the swap funding address
  • Alice goes offline.
  • The swap funding transaction gets confirmed.
  • Carol publishes the claim transaction, with the preimage.
  • 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.

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:

  • cancel the swap (using a double spend)
  • run the risk (promise to come back online soon)

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

@SomberNight
Copy link
Member

  • 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:

('c', MIN_FINAL_CLTV_DELTA_FOR_INVOICE),

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
Copy link
Member

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

msg += _("Please remain online until the funding transaction is confirmed.")

@ecdsa
Copy link
Member Author

ecdsa commented Mar 12, 2024

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

yes, this is what I was referring to above

SomberNight added a commit that referenced this issue Mar 12, 2024
This gives more time for the client to come back online.

see #8940

- re note on submarine_swaps.py#L53:
  lnpeer.Peer.maybe_fulfill_htlc only checks against MIN_FINAL_CLTV_DELTA_ACCEPTED(=144),
  so this increased cltv_delta is not enforced when receiving the htlc on ln.
  It is put in the invoice, so the sender is supposed to honour it ofc.
  It would be nice to enforce it (make the check in maybe_fulfill_htlc dependent on
  what was in the invoice).
@SomberNight
Copy link
Member

I think we should adjust this text in qml - which was applicable to the old normal swap:

self.userinfo = ' '.join([
_('Success!'),
_('Your funding transaction has been broadcast.'),
_('The swap will be finalized once your transaction is confirmed.'),
_('You will need to be online to finalize the swap, or the transaction will be refunded to you after some delay.'),
])

SomberNight added a commit to SomberNight/electrum that referenced this issue Mar 13, 2024
@ecdsa
Copy link
Member Author

ecdsa commented Mar 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants