search

DeFiCh / ain

DeFi Blockchain - enabling decentralized finance on Bitcoin
https://defichain.com
MIT License
408 stars 122 forks source link

Pool Slippage Protection is taking spot price instead of actual price #521

Closed muirglacier closed 2 years ago

muirglacier commented 3 years ago

Just wanted to address an issue, the slippage protection as defined here https://github.com/DeFiCh/ain/blob/master/src/masternodes/poolpairs.cpp#L389-L393 is not working correctly in all cases.

Right now, all it does is take the spot price and compare it to the maximum price the trader specified in his transaction. There are some cases and scenarios, where this can go completely wrong.

Imagine you are a malicious masternode, and there is a low liquidity pool pair. You could add heaps of liquidity, and wait for an unsuspected trader to place his order. The app will show him some potential trade result accounting for the slippage with the current liquidity in the pool, and he will most likely go ahead with the trade.

Now, a malicious masternode could wait for a PoolSwap transaction, and "sandwitch" it with a "Remove Liquidity" -> Victims Pool Swap -> Attackers Arbitrage -> "Add Liquidity" Transaction.

The reason this works is because the spot price stays the same on "remove liquidity", but slippage gets a LOT higher the less liquidity is in the pool. However, slippage protection is not looking at the liquidity, just at the spot price.

Not critical as there is enough liquidity in the "important pools", hence I didnt submit it as a bug bounty.

My suggestion is: Instead of specifying MAX PRICE, let the user specify MIN TOKENS RETURNED.

(This is consensus critical, maybe keep it low priority for one of the future hard forks)

defichain-bot commented 3 years ago

@muirglacier: 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](https://github.com/DeFiCh) developers manage community feedback and contributions. You can check out my [manifest file](https://github.com/DeFiCh/ain/blob/master/.github/governance.yml) 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](https://github.com/DeFiCh/oss-governance-bot) repository.
uzyn commented 3 years ago

/triage accepted

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kuegi commented 2 years ago

it seems like the additional check in fort canning fixed this already: https://github.com/DeFiCh/ain/blob/06f2ed8edaec7ea0741ec66e1210b1e8549dec23/src/masternodes/mn_checks.cpp#L3642-L3648