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

fix: taking round down into consideration when calculates swap fee #14

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

chefburger
Copy link
Collaborator

@chefburger chefburger commented Apr 16, 2024

A bug found when adding invariant test which could cause collect function revert

Context

Considering for the same range, there are multiple LP tokens, let say LP1, LP2, LP3 ...

  1. LP1 is minted first, and then there is a swap inside the range which brings some fee.
  2. LP2 is minted after. Since LP2 is minted in the same range through nfp, so those two shares the same position in v4-core. The following state changes are made:
    CleanShot 2024-04-16 at 18 13 10@2x
  3. Now let's consider if the fee accumulated in step1 is small enough, then feesOwed0, feesOwed1 can be 0 due to the round down calculation but feeGrowthInside0LastX128 and feeGrowthInside1LastX128 in v4-core has been updated anyway. (the fee accumulated in step1 has in fact been chopped)
  4. However the field feeGrowthInside0LastX128 and feeGrowthInside1LastX128 in LP1 remains the original value because the position is untouched in the perspective of nfp contract.
  5. More swaps happen and more LP are added in the same position, feeGrowthInside0LastX128 and feeGrowthInside1LastX128 in v4-core will be constantly updated and the fee accumulated is collected by NFP in the form of vaultToken but the amount has been truncated due the the reason above.
  6. There exists a state that (feeGrowthInside0FromV4Core - feeGrowthInsideFromNfp) * liquidity / Q128 > 1, but the fee accumulated is still less than 1 in fact it's 0 after round down.
  7. At this time if LP1 calls collect, the tx will revert

How v3 handles it

Looks back to v3 I notice it's using similar workaround

CleanShot 2024-04-16 at 18 26 37@2x
CleanShot 2024-04-16 at 18 27 37@2x

ChefMist
ChefMist previously approved these changes Apr 17, 2024
Copy link
Collaborator

@ChefMist ChefMist left a comment

Choose a reason for hiding this comment

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

👍

curious, this can mean that the last folk who seldom collect might lose more than a few wei. eg.

1. we have 100 pool with `weth` as one of the token
2. over time when we do `resetAccumulatedFee()`, we get minted 100 eth 
3. when its user turn to collect 0.1 eth, he might collect nothing due to all the rounding  

though 0.1 eth is a bit exaggerated, it might be a very long period before we hit that large number.


(potential option -- might be more gas compared to calling vault )

  1. we have mapping which track feeAccumulated poolId -> tickUpper -> tickLower whenever we call resetAccumulatedFee

  2. instead of calling vault.balanceOf(..) we check the mapping and deduct from there

@chefburger
Copy link
Collaborator Author

👍

curious, this can mean that the last folk who seldom collect might lose more than a few wei. eg.

1. we have 100 pool with `weth` as one of the token
2. over time when we do `resetAccumulatedFee()`, we get minted 100 eth 
3. when its user turn to collect 0.1 eth, he might collect nothing due to all the rounding  

though 0.1 eth is a bit exaggerated, it might be a very long period before we hit that large number.

(potential option -- might be more gas compared to calling vault )

  1. we have mapping which track feeAccumulated poolId -> tickUpper -> tickLower whenever we call resetAccumulatedFee
  2. instead of calling vault.balanceOf(..) we check the mapping and deduct from there

Record the process for future reference:

We did try this method but unfortunately since the logic here brought extra sstore and gas shot up from 199k to 242k. We decided to revert back after discussing

@chefburger chefburger merged commit 35f17ef into main Apr 17, 2024
2 checks passed
@chefburger chefburger deleted the fix/cl-nfp-collect branch April 17, 2024 10:15
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.

2 participants