QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: medium
Valid

exitFee can exceed 100% and revert remove liquidity

hardlydifficult

Summary

A math error calculating fees in UpliftOnlyExample.sol causes removeLiquidityProportional to revert with an underflow error when there has been a significant price change (51x in the POC below). Funds are then stuck in the vault until the price drops back to a supported range.

A workaround exists: users can transfer the LPNFT to another account (and optionally back again) to eliminate fees and therefor avoid this revert as well. However I believe that's a bug to be addressed and not an intentional workaround for this.

Vulnerability Details

The impacted code is in the onAfterRemoveLiquidity hook function.

lpTokenDepositValueChange is a ratio. It's equal to 51 in the POC of the revert below, representing a 51x price increase (or a 52x price change) since deposit:

localData.lpTokenDepositValueChange =
(int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
int256(localData.lpTokenDepositValue);

Source: UpliftOnlyExample.sol#L474-L476

Then that ratio is multiplied by the fee. In the POC below, the fee is 2% so a 50x price increase charges 100%:

feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;

Source: UpliftOnlyExample.sol#L481-L483

The feePerLP multiplied by the amount being removed, so when the feePerLP is > 100% we have a feeAmount > the deposit:

localData.feeAmount += (depositAmount * feePerLP);

Source: UpliftOnlyExample.sol#L495

Now we calculate a feePercentage by comparing that feeAmount to bptAmountIn -- this value is equivalent to the feePerLP calculated above but averaged across LPNFTs:

localData.feePercentage = (localData.feeAmount) / bptAmountIn;

Source: UpliftOnlyExample.sol#L514

The exit fee is calculated using that percentage, which may be > 100% -- resulting in an exitFee > the amount available:

uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);

Source: UpliftOnlyExample.sol#L523

Now when we attempt to subtract that fee from the amount being withdrawn, the transaction reverts with an underflow error on fees > 100%:

hookAdjustedAmountsOutRaw[i] -= exitFee;

Source: UpliftOnlyExample.sol#L530

Impact

When upliftFeeBps is 2% then a 50x price increase charges 100% fees and anything greater will revert. If upliftFeeBps is increased to 5% than it breaks when over a 20x price increase. These are large but not unreasonable price changes and it's possible the price will never come back down within range again.

Assets become locked within the Vault, preventing users from removing liquidity.

The workaround is clunky at best, and may be considered a bug to be removed (I'll report that as a separate issue).

POC

This POC updates UpliftExample.t.sol, modifying testRemoveLiquidityDoublePositivePriceChange which was testing a 2x price change. We increase the price change to 52x and confirm it reverts.

Additionally we can validate the transferFrom workaround by uncommenting that section and noting that it no longer reverts.

diff --git a/pkg/pool-hooks/test/foundry/UpliftExample.t.sol b/pkg/pool-hooks/test/foundry/UpliftExample.t.sol
index d253722..8994689 100644
--- a/pkg/pool-hooks/test/foundry/UpliftExample.t.sol
+++ b/pkg/pool-hooks/test/foundry/UpliftExample.t.sol
@@ -648,90 +648,28 @@ contract UpliftOnlyExampleTest is BaseVaultTest {
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < tokens.length; ++i) {
- prices[i] = int256(i) * 2e18;
+ prices[i] = int256(i) * 52e18; // 52 fails, 51 succeeds since the fee is 2%. 51 is a 50x increase and 100% fee.
}
updateWeightRunner.setMockPrices(pool, prices);
- uint256 nftTokenId = 0;
+ uint256 nftTokenId = 1; // This was set incorrectly in the original test
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
BaseVaultTest.Balances memory balancesBefore = getBalances(bob);
+ // Workaround by transferring to eliminate fees:
+ // LPNFT lpNft = upliftOnlyRouter.lpNFT();
+ // vm.prank(bob);
+ // lpNft.transferFrom(bob, alice, nftTokenId);
+ // vm.prank(alice);
+ // lpNft.transferFrom(alice, bob, nftTokenId);
+
vm.startPrank(bob);
+ vm.expectRevert(stdError.arithmeticError);
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
- BaseVaultTest.Balances memory balancesAfter = getBalances(bob);
-
- uint256 feeAmountAmountPercent = ((bptAmount / 2) *
- ((uint256(upliftOnlyRouter.upliftFeeBps()) * 1e18) / 10000)) / ((bptAmount / 2));
-
- /*
- Bob has doubled his value.
- Uplift fee is taken on only the uplift.
- Given each BPT is worth double now, the fee is 2% of the original value.
- Bob has 1000e18 in BPT, so the fee is 20e18.
- Bob should get 980e18 in DAI and USDC.
- */
-
- uint256 amountOut = (bptAmount / 2).mulDown((1e18 - feeAmountAmountPercent));
-
- // Bob gets original liquidity with no fee applied because of full decay.
- assertEq(
- balancesAfter.bobTokens[daiIdx] - balancesBefore.bobTokens[daiIdx],
- amountOut,
- "bob's DAI amount is wrong"
- );
- assertEq(
- balancesAfter.bobTokens[usdcIdx] - balancesBefore.bobTokens[usdcIdx],
- amountOut,
- "bob's USDC amount is wrong"
- );
-
- // Pool balances decrease by amountOut.
- assertEq(
- balancesBefore.poolTokens[daiIdx] - balancesAfter.poolTokens[daiIdx],
- amountOut,
- "Pool's DAI amount is wrong"
- );
- assertEq(
- balancesBefore.poolTokens[usdcIdx] - balancesAfter.poolTokens[usdcIdx],
- amountOut,
- "Pool's USDC amount is wrong"
- );
-
- //As the bpt value taken in fees is readded to the pool under the router address, the pool supply should remain the same
- assertEq(balancesBefore.poolSupply - balancesAfter.poolSupply, bptAmount, "BPT supply amount is wrong");
-
- // Same happens with Vault balances: decrease by amountOut.
- assertEq(
- balancesBefore.vaultTokens[daiIdx] - balancesAfter.vaultTokens[daiIdx],
- amountOut,
- "Vault's DAI amount is wrong"
- );
- assertEq(
- balancesBefore.vaultTokens[usdcIdx] - balancesAfter.vaultTokens[usdcIdx],
- amountOut,
- "Vault's USDC amount is wrong"
- );
- // Hook balances remain unchanged.
- assertEq(balancesBefore.hookTokens[daiIdx], balancesAfter.hookTokens[daiIdx], "Hook's DAI amount is wrong");
- assertEq(balancesBefore.hookTokens[usdcIdx], balancesAfter.hookTokens[usdcIdx], "Hook's USDC amount is wrong");
-
- // Router should set all lp data to 0.
- //User has extracted deposit, now deposit was deleted and popped from the mapping
- assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 0, "bptAmount mapping should be 0");
- //assertEq(upliftOnlyRouter.bptAmount(nftTokenId), 0, "bptAmount mapping should be 0");
- //assertEq(upliftOnlyRouter.startTime(nftTokenId), 0, "startTime mapping should be 0");
-
- assertEq(upliftOnlyRouter.nftPool(nftTokenId), address(0), "pool mapping should be 0");
-
- assertEq(
- BalancerPoolToken(pool).balanceOf(address(upliftOnlyRouter)),
- 0,
- "upliftOnlyRouter should hold no BPT"
- );
- assertEq(balancesAfter.bobBpt, 0, "bob should not hold any BPT");
+ // Removing assertions from the original test since this reverts now.
}
function testRemoveWithNonOwner() public {

Recommendation

Bound the fee to be <= a reasonable max, e.g.

uint MAX_FEE_PERCENTAGE = 10e16; // 10%
if (localData.feePercentage > MAX_FEE_PERCENTAGE) {
localData.feePercentage = MAX_FEE_PERCENTAGE;
}
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_onAfterRemoveLiquidity_feePerLP_no_max_can_reach_100%

Likelihood: Low, will happen only if benefits are huge. (x50 to take 100%, x25 to take 50%). Impact: feePerLP will indefinitely increase, taking users profits and finaly DoS the function.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!