Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V-PHX-VUL-006] Split deposit should target the new pool ratio #204

Closed
ueco-jb opened this issue Feb 1, 2024 · 1 comment · Fixed by #289 · May be fixed by #271
Closed

[V-PHX-VUL-006] Split deposit should target the new pool ratio #204

ueco-jb opened this issue Feb 1, 2024 · 1 comment · Fixed by #289 · May be fixed by #271
Assignees
Labels
audit bug - high bug with high impact
Milestone

Comments

@ueco-jb
Copy link
Member

ueco-jb commented Feb 1, 2024

https://veridise.notion.site/Split-deposit-should-target-the-new-pool-ratio-c04d2ccddf1c433abfe48bc2912f6a1a

file: pool/src/contract.rs
location: fn split_deposit_based_on_pool_ratio

The function split_deposit_based_on_pool_ratio tries to match the amount deposited to the ratio of balances held by the pool. However, the target ratio is based on the pool ratio before the swap occurs.

Impact After the swap occurs, the ratio changes, and so the user is rewarded fewer LP shares and ends up losing equity even on a fee-less pool.

Recommendation At a minimum, we recommend updating target_ratio during the computation of final_offer_amount and final_ask_amount so that target_ratio is the ratio of pool balances after the swap, not before (e.g., target_ratio = Decimal::from_ratio(b_pool - swapped_b, a_pool + swapped_a) if the swapped asset is tokenA.

However, we would prefer to see the use of a precise formula for computing split_deposit_based_on_pool_ratio rather than the current binary search method.

@ueco-jb ueco-jb added the bug - high bug with high impact label Feb 1, 2024
@ueco-jb ueco-jb added this to the Audit Review milestone Feb 1, 2024
@ueco-jb ueco-jb self-assigned this Feb 1, 2024
@ueco-jb ueco-jb linked a pull request Feb 1, 2024 that will close this issue
@gangov gangov changed the title Pool: Split deposit should target the new pool ratio [V-PHX-VUL-006] Split deposit should target the new pool ratio Feb 6, 2024
@gangov gangov added the audit label Feb 6, 2024
@gangov gangov linked a pull request Mar 19, 2024 that will close this issue
@ueco-jb
Copy link
Member Author

ueco-jb commented Apr 29, 2024

We have decided to disable the feature allowing to provide liquidity on the single side for now.
We will enable it after getting to the bottom of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment