ain
ain copied to clipboard
DeFiCh
massive swap (relative to liquidity) destroys the pool (just happened on DIS)
As just happened on mainnet after DIS was added: the pool was still nearly empty and someone tried to swap 4500$ into DIS, moving the pool by 20897985072.5% basically clearing the DIS side.
state of the pool afterwards:
"69": {
"symbol": "DIS-DUSD",
"name": "dDIS-Decentralized USD",
"status": true,
"idTokenA": "65",
"idTokenB": "15",
"reserveA": 2.7e-7,
"reserveB": 5595.15430529,
"commission": 0.002,
"totalLiquidity": 0.03880735,
"reserveA/reserveB": 0,
"reserveB/reserveA": 20722793723.296295,
"tradeEnabled": false,
"ownerAddress": "8UAhRuUFCyFUHEPD7qvtj8Zy2HxF5HH5nb",
"blockCommissionA": 0,
"blockCommissionB": 0,
"rewardPct": 0,
"rewardLoanPct": 0,
"creationTx": "a6808654ee52eeec740ffa7984b22f0ecddf211c4a5bc24283d72ad5a6e532eb",
"creationHeight": 1755379
}
since reservesA is now < SLOPE_SWAP_RATE you can't even bring it back cause it always returns "Lack of Liquidity". and adding enough liquidity will need huge amounts of $ (roughly 200k) which will get lost due to IL afterwards.
@kuegi: Thanks for opening an issue, it is currently awaiting triage.
The triage/accepted label can be added by foundation members by writing /triage accepted in a comment.
Details
I am a bot created to help the DeFiCh developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the DeFiCh/oss-governance-bot repository.
Yep, that's normal, pool isn't balanced.
yes, but I think we shouldn't allow a swap that leaves the pool in an untradeable state.
As just happened on mainnet after DIS was added: the pool was still nearly empty and someone tried to swap 4500$ into DIS, moving the pool by 20897985072.5% basically clearing the DIS side.
state of the pool afterwards:
"69": { "symbol": "DIS-DUSD", "name": "dDIS-Decentralized USD", "status": true, "idTokenA": "65", "idTokenB": "15", "reserveA": 2.7e-7, "reserveB": 5595.15430529, "commission": 0.002, "totalLiquidity": 0.03880735, "reserveA/reserveB": 0, "reserveB/reserveA": 20722793723.296295, "tradeEnabled": false, "ownerAddress": "8UAhRuUFCyFUHEPD7qvtj8Zy2HxF5HH5nb", "blockCommissionA": 0, "blockCommissionB": 0, "rewardPct": 0, "rewardLoanPct": 0, "creationTx": "a6808654ee52eeec740ffa7984b22f0ecddf211c4a5bc24283d72ad5a6e532eb", "creationHeight": 1755379 }since reservesA is now < SLOPE_SWAP_RATE you can't even bring it back cause it always returns "Lack of Liquidity". and adding enough liquidity will need huge amounts of $ (roughly 200k) which will get lost due to IL afterwards.
Thanks for the finding and analysis. I see it in the same way. The code allowed to move into a system state, where exit condition is nearly impossible to full fill. This must be solved.
Maybe additional to solving this bug, we should use a different ramp-up phase: After launch of new pools adding liquidity is enable as a first function (no swap). So, only people minting the new dToken can join the pool. After a certain liquidity threshold, the swap function will be enabled. This brings more stability to the system and reduces the risk for users loosing money due to bigger pool ratio changes. What do you think?
Maybe additional to solving this bug, we should use a different ramp-up phase: After launch of new pools adding liquidity is enable as a first function (no swap). So, only people minting the new dToken can join the pool. After a certain liquidity threshold, the swap function will be enabled. This brings more stability to the system and reduces the risk for users loosing money due to bigger pool ratio changes. What do you think?
Less a technical question but consensus, so maybe better we discuss this on reddit. But in general I agree: when new pools are introduced, they should be added in a deactivate (so no trading, but adding liquidity is possible) for maybe a day(?). then ppl added liquidity and trading can start once a certain threshold of liquidity (f.e. 10k$ ?) is reached.
I like the idea of the ramp up phase, but I also agree with @kuegi that this should be discussed elsewhere.
Regarding the bug, in my opinion the comparison with SLOPE_SWAP_RATE should probably also be done for the result of the swap. (But if the ramp up is changed as suggested by @DanielZirkel this may not be necessary any longer.)
Moreover, here, I think it would suffice to do
if (reserveT < SLOPE_SWAP_RATE)
instead of
if (reserveA < SLOPE_SWAP_RATE || reserveB < SLOPE_SWAP_RATE)
I.e., it doesn't matter if the reserve of the from-token is small since we are going to increase this with the swap and hence this side cannot underflow. This change would not have prevented the issue, but it would give us an easy way out since it would allow shifting the pool ratio back by swapping DIS to DUSD regardless of the amount of DIS in the pool.
Moreover, here, I think it would suffice to do
if (reserveT < SLOPE_SWAP_RATE)instead of
if (reserveA < SLOPE_SWAP_RATE || reserveB < SLOPE_SWAP_RATE)I.e., it doesn't matter if the reserve of the from-token is small since we are going to increase this with the swap and hence this side cannot underflow. This change would not have prevented the issue, but it would give us an easy way out since it would allow shifting the pool ratio back by swapping DIS to DUSD regardless of the amount of DIS in the pool.
agreed on the slope changes.
I don't think that it gets irrelevant with a better ramp up, cause even with lots of liquidity, it could happen that someone overestimates it and triggers the problem. technically we should be ready.
I don't think that it gets irrelevant with a better ramp up, cause even with lots of liquidity, it could happen that someone overestimates it and triggers the problem. technically we should be ready.
So true! Better safe than sorry :-)