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

[Bug]: FullRange.addLiquidity: wrong amount of tokens minted #68

Closed
ferencdg opened this issue Oct 22, 2023 · 2 comments · May be fixed by #128
Closed

[Bug]: FullRange.addLiquidity: wrong amount of tokens minted #68

ferencdg opened this issue Oct 22, 2023 · 2 comments · May be fixed by #128
Labels
bug Something isn't working

Comments

@ferencdg
Copy link

ferencdg commented Oct 22, 2023

Describe the bug

UniswapV4ERC20(pool.liquidityToken).mint(params.to, liquidity);

I am not sure this can be called bug or a design decision, but imagine the following scenario:

  1. January 1st userA addLiquidity and gets 100 shares minted
  2. Between January 1 - June 30th lots of swaps are happening, but no one removes liquidity
  3. On June 30th the pool price is the same as it was on January 1st, and userB adds the same liquidity as userA and gets the same 100 shares minted
  4. Now if userB redeems, he can run away with all the fees generated between January 1st - June 30th

The problem is that 'rebalancing' is only called during removing liquidity from the pool, so the fees are not accounted for unless someone withdraws liquidity. So the fairness of the fee distribution depends on how often the withdrawLiquidity method is called.

Expected Behavior

easiest fix would be to call rebalance before depositing too, then mint: userSuppliedLiquidity*totalAmountOfTokensMinted/allLiquidityHeldByThePool

To Reproduce

No response

Additional context

No response

@ferencdg ferencdg added the bug Something isn't working label Oct 22, 2023
@Jun1on Jun1on linked a pull request Jun 24, 2024 that will close this issue
@Jun1on
Copy link
Contributor

Jun1on commented Jun 24, 2024

The problem is actually worse.
Let's say:

  1. userA adds a lot of liquidity
  2. lots of swaps generate 1 ETH and 1000 USDC of fees, but no one removes liquidity yet
  3. userB adds 1.001 ETH and 1001 USDC of liquidity but it only costs 0.001 ETH and 1 USDC to do so
    • because modifyLiquidity accrues fees, manager.modifyLiquidity returns a delta of (1 - 1.001) ETH and (1000 - 1001) USDC
    • note: userB must add at least 1 ETH and 1000 USDC or else the function call will revert as manager.modifyLiquidity will return a positive delta
  4. Now userB redeems for 1.001 ETH and 1001 USDC, yoinking all the fees instantly

@Jun1on Jun1on removed a link to a pull request Jun 24, 2024
@Jun1on Jun1on linked a pull request Jun 24, 2024 that will close this issue
@saucepoint
Copy link
Collaborator

Hi! thanks for flagging -- going to close this issue because:

hook examples no longer live in the main branch

custom accounting will allow for a more v2-style curve with v2 interfaces. reference implementation https://github.com/hensha256/v2-on-v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants