search

ref-finance / ref-contracts

Smart contracts for Ref Finance
MIT License
96 stars 55 forks source link

Problem caused by unregister LP when removing all liquidity from a pool #36

Open marco-sundsk opened 2 years ago

marco-sundsk commented 2 years ago

It's a true story from some user, who wants to withdraw lp token from farming but failed.

Scenario

First try :

  1. Alice adds liquidity to some pool, let's say pool#1429, and got 1.0 lp token,
  2. Alice stakes 0.5 lp token to farming,
  3. Alice remove all remain liquidity (0.5) from pool#1429,
  4. Alice withdraw lp token from farming. Then, bom.... ERR13_LP_NOT_REGISTERED ；

What happened:
In step 3, as Alice removed all her liquidity, the pool unregister her from its LP to save storage;
So, in step 4, you can not transfer lp token to someone not registered on that token contract.

Resolution:
There is only a mft_balance_of can tell us something about user in the LP token, Maybe we just re-register Alice before she withdraws lp token from farming?

Second try :

  1. Alice adds liquidity to some pool, let say pool#1429, and got 1.0 lp token,
  2. Alice stakes 1.0 lp token to farming,
  3. Alice starts withdraw lp token from farming:
    1. See her lp token remaining using mft_balance_of, and got 0,
    2. Register her into the lp token before withdraw; Then, bom.... ERR14_LP_ALREADY_REGISTERED ；

What's wrong:
For transfer of lp tokens, there is no unregsiter logic in it. So Alice is still hang in there even if she has no lp toke left.

Conclusion and Suggestion

Transferring and remove_liqudity are different at register management logic. It's hard to tell a user whether or not he is registered in a lp token, we may need to add a storage_balance_of interface to lp token here.

referencedev commented 2 years ago

Quick fix would be not to remove any LP positions even if all LP tokens are removed via remove_liquidity.

Full fix requires to actually migrate Account to new version that keeps track of LP positions as well in storage properly. Robert was trying to do that but it is a bit complex as need to maintain old Account model going forward and need to pass through Account information into the Pools as well.