BlueWallet / LndHub

Wrapper for Lightning Network Daemon. It provides separate accounts for end-users
http://LndHub.io
MIT License
793 stars 198 forks source link

Successfully withdrew same funds twice #3

Closed btcsessions closed 4 years ago

btcsessions commented 5 years ago

Hey guys, just wanted to let you know I was able to send the same funds to an outside wallet (Wallet of Satoshi) twice. I was trying to send around 800,000 sats out. When it didn't execute quickly, I tried again with a few less sats. To my surprise both of them ended up executing and landing in the other wallet. To be sure that the funds were legitimate I sent them out from Wallet of Satoshi to my Casa Node, and they landed no problem.

I stumbled upon this accidentally, so I thought you should know, given that other people may be unknowingly doing the same thing.

Overtorment commented 5 years ago

Thanks! So basically you spent same funds twice? Okay, I know which direction to dig.

On Sun, Jan 6, 2019 at 03:10 btcsessions notifications@github.com wrote:

Hey guys, just wanted to let you know I was able to send the same funds to an outside wallet (Wallet of Satoshi) twice. I was trying to send around 800,000 sats out. When it didn't execute quickly, I tried again with a few less sats. To my surprise both of them ended up executing and landing in the other wallet. To be sure that the funds were legitimate I sent them out from Wallet of Satoshi to my Casa Node, and they landed no problem.

I stumbled upon this accidentally, so I thought you should know, given that other people may be unknowingly doing the same thing.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/BlueWallet/BlueWallet/issues/213, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0x-dG0MvEPSIJVrQ6bwjyGGUmSOOAGks5vAWk7gaJpZM4ZyPF7 .

btcsessions commented 5 years ago

Yes indeed. It definitely wasn't intentional, but it seemed that the transaction was taking some time to execute. I don't know how long the wait was, but I was able to go into Wallet of Satoshi and create a new payment request. When I navigated back to Blue, it didn't show any movement of funds, so I pasted in the new code.

In the end my wallet showed a negative balance. -8 sat.

nothingmuch commented 5 years ago

FWIW, some armchair debugging: https://twitter.com/mHaGqnOACyFm0h5/status/1081760599397289985

I don't think redis is a very appropriate choice of backend due to its fiddly atomicity and durability guarantees

nothingmuch commented 5 years ago

though the likelyhood of a successfully withdrawing multiple times is reduced by that change, there are still possible races in d3100a1

Overtorment commented 5 years ago

Locks need improvement

Overtorment commented 5 years ago

@nothingmuch does this look good? Really appreciate your input!

nothingmuch commented 5 years ago

yes, i think with setnx this now guarantees atomicity in regards to lock acquisition, so mutual exclusion should be assured

one remaining concern is that the 2 minute expiry window seems a bit risky, because if lnd is hanging during payment, but eventually it goes through, the likelyhood that the user will retry increases, so i think that this failure mode is not as unlikely as the product of the probability of an lnd side delay and the repeated attempts by the user but that those two events would be highly correlated.

a workaround for that would be to refresh the lock at intervals in the Lock class, i.e. to still have expiry from the redis side, but extend it at regular intervals from the node side, so that as long as a request is still in flight it the lock won't be cleared.

an edge case that should be investigated is:

  1. acquire lock, setting expiry
  2. release lock
  3. re-acquire lock
  4. previous expiry triggers before setting new expiry

in other words, does deleting a key also clear any associted expiration time on the redis side? i am not sure from reading its API documentation, the discussion of how it expires keys seems to imply that that would not be a problem since it seems that is stored as part of the value.

that said, i still worry about the choice of redis itself, i haven't been able to find information about how heroku sets it up, and whether or not it runs in append only mode, with fsync after every operation, so there might be a possibility for balances or the states of invoices to go out of sync between lnd and the database, or if there's any replication (and from the little i know about redis, the way it handles replication is not exactly safe).

Overtorment commented 5 years ago

Redis runs in redislabs with persistance enabled, flushing binary log every second if I remember correctly. Also, with replication for failover.

I need to reread what you wrote about locking couple more times and give it a thought :-) Thanks!

On Wed, Jan 9, 2019 at 15:30 Yuval Kogman notifications@github.com wrote:

yes, i think with setnx this now guarantees atomicity in regards to lock acquisition, so mutual exclusion should be assured

one remaining concern is that the 2 minute expiry window seems a bit risky, because if lnd is hanging during payment, but eventually it goes through, the likelyhood that the user will retry increases, so i think that this failure mode is not as unlikely as the product of the probability of an lnd side delay and the repeated attempts by the user but that those two events would be highly correlated.

a workaround for that would be to refresh the lock at intervals in the Lock class, i.e. to still have expiry from the redis side, but extend it at regular intervals from the node side, so that as long as a request is still in flight it the lock won't be cleared.

an edge case that should be investigated is:

  1. acquire lock, setting expiry
  2. release lock
  3. re-acquire lock
  4. previous expiry triggers before setting new expiry

in other words, does deleting a key also clear any associted expiration time on the redis side? i am not sure from reading its API documentation, the discussion of how it expires keys seems to imply that that would not be a problem since it seems that is stored as part of the value.

that said, i still worry about the choice of redis itself, i haven't been able to find information about how heroku sets it up, and whether or not it runs in append only mode, with fsync after every operation, so there might be a possibility for balances or the states of invoices to go out of sync between lnd and the database, or if there's any replication (and from the little i know about redis, the way it handles replication is not exactly safe).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/BlueWallet/LndHub/issues/3#issuecomment-452737488, or mute the thread https://github.com/notifications/unsubscribe-auth/AB0x-fQCpiLo2xsCy1E_i8xFdzbyCZ_Fks5vBgsWgaJpZM4Zyci9 .

lessless commented 4 years ago

Hi guys,

Can you please elaborate on severity of this issue? My understanding is that user was a able to spend more funds from an underlying LND wallet than it was associated with his "account". Is this correct?

ncoelho commented 4 years ago

We saw a couple of these cases when we launched lndhub one year ago, they were fixed at the time. We haven’t seen any more similar issues in the last 12 months

Overtorment commented 4 years ago

Yep, all issues were fixed about a year ago, Lndhub is in feature freeze since then.

lessless commented 4 years ago

@Overtorment thanks for clarification. Isn’t it time to close the issue then?