Skip to content

Conversation

piobab
Copy link
Collaborator

@piobab piobab commented Aug 18, 2023

The slippage parameter is not explicitly sanity checked. The parameter could be set 100% in which case the user could receive no output coins as a result.

Remediation
The slippage parameter should be checked if it’s less than 1 and not exceed maximum amount e.g. max slippage 10%.

@piobab piobab requested review from brimigs and thec00n August 18, 2023 10:40
Copy link
Contributor

@brimigs brimigs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

@thec00n
Copy link

thec00n commented Aug 21, 2023

Introducing max slippage could be problematic when a pool has low liquidity. I think we should assert that the upstream function sets a max slippage but not limit at a certain percentage.

@piobab
Copy link
Collaborator Author

piobab commented Aug 21, 2023

function sets a max slippage but not limit at a certain percentage.

Wdym?

I think we don't want to work on low liquidity pool.

@thec00n
Copy link

thec00n commented Aug 22, 2023

It's fine as long as pools have enough liquidity. I just wanted to note that it can become a problem with low liquidity pools. The assumption here is that red-bank will not operate on pools with low liquidity.

@piobab
Copy link
Collaborator Author

piobab commented Aug 22, 2023

@thec00n yeap, for sure but I think low liquidity pools are not our goal.

@piobab piobab merged commit 28b6905 into develop Aug 22, 2023
@piobab piobab deleted the fix/MP-2851-max-slippage branch September 14, 2023 08:09
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

Successfully merging this pull request may close these issues.

3 participants