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

Covering impermanent loss allows profiting off asymmetric liquidity provision at expense of reserve holdings #255

Open
code423n4 opened this issue Nov 15, 2021 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly BasePoolV2 bug Something isn't working duplicate This issue or pull request already exists VaderMath VaderPoolV2 VaderReserve VaderRouterV2

Comments

@code423n4
Copy link
Contributor

Handle

hyh

Vulnerability details

Impact

Pool funds will be siphoned out over time as swaps and asymmetric LP provision are balancing each other economically, while with introduction of IL reimbursement a malicious user can profit immediately from out of balance pool with a swap and profit again from IL coverage. This requires locking liquidity to a pool, but still represents an additional profit without additional risk at expense of reserve funds.

Another variant of exploiting this is to add liquidity in two steps: deposit 1 with 0 slip adjustment, perfectly matching current market price, deposit 2 with more Vader than market price suggests, moving pool out of balance with Vader becoming cheaper, then exiting deposit 1 with profit because slip adjustment reduce deposit 2's share issuance and deposit 1's now has more asset claims than before. Deposit 2 then need to wait and exit after some time.

IL is calculated as ((originalAsset * releasedVader) / releasedAsset) + originalVader - ((releasedAsset * releasedVader) / releasedAsset) + releasedVader , i.e. original deposit values without taking account of slip adjustment are used, so providing more Vader in deposit 2 leads to greater IL, which this way have 2 parts: market movements related and skewed liquidity provision related. IL covering compensates for slip adjustments this way.

Proof of Concept

The steps to reproduce are:

  1. add asymmetric LP via mint (with NFT),
  2. either swap gathering profit from pool skew or do symmetric deposit beforehand and exit it now
  3. wait for some period for IL protection to be enabled, then withdraw, having IL covered by reserve fund

Router addLiquidity:
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/router/VaderRouterV2.sol#L114

NFT mint:
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/BasePoolV2.sol#L168

Router removeLiquidity:
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/router/VaderRouterV2.sol#L227

NFT burn:
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L237

IL calculation:
https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/math/VaderMath.sol#L73

Recommended Mitigation Steps

Asymmetric liquidity provision doesn't provide much business value, introducing substantial attack surface, so the core recommendation here is to remove a possibility to add liquidity asymmetrically: instead of penalizing LP with slip adjustment do biggest liquidity addition with 0 slip adjustment that user provided funds allow, and return the remaining part.

This will also guard against cases when user added liquidity with big slip adjustment penalty without malicious intent, not realizing that this penalty will take place, an effect that poses reputational risk to any project using the approach.

Allowing only symmetric liquidity addition removes the described attack surface.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 15, 2021
code423n4 added a commit that referenced this issue Nov 15, 2021
@SamSteinGG SamSteinGG added the duplicate This issue or pull request already exists label Nov 25, 2021
@alcueca
Copy link
Collaborator

alcueca commented Dec 10, 2021

Duplicate of which other issue, @SamSteinGG?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly BasePoolV2 bug Something isn't working duplicate This issue or pull request already exists VaderMath VaderPoolV2 VaderReserve VaderRouterV2
Projects
None yet
Development

No branches or pull requests

4 participants