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

Users are charged too much `exitFee` in `UpliftOnlyExample::onAfterRemoveLiquidity` function when `localData.lpTokenDepositValueChange > 0` and can cause underflow error if `lpTokenDepositValueChange` increase too much.

kazan

Summary

Users are charged too much exitFee in UpliftOnlyExample::onAfterRemoveLiquidity when localData.lpTokenDepositValueChange > 0 and can cause underflow error if lpTokenDepositValueChange increase too much.

Vulnerability Details

  • In UpliftOnlyExample::onAfterRemoveLiquidity, whenever user remove their liquidity, they will be charged an exitFee based on feePerLP.

  • And the feePerLP is calculated based on lpTokenDepositValueChange. UpliftOnlyExample.sol#L480-L484

if (localData.lpTokenDepositValueChange > 0) {
feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
}

  • While the idea of charging feePerLP based on how much increase of deposit tokens value is a fair idea. But the implementation of the above code has a problem:

    • The feePerLP will increases proportionally with the lpTokenDepositValueChange. Developers may forget that even if feePerLP is a fixed percentage, the exitFee tokens value still increases proportionally with the lpTokenDepositValueChange.

  • So, the value of exitFee will increase exponentially due to:

    1. The value of the tokens increases.

    2. The amount of fee tokens increases.

Impact

  • Users will be charged too much exitFee if lpTokenDepositValueChange > 0.

  • And in a worse scenario, user can not withdraw their liquidity when localData.lpTokenDepositValueChange > (10000 / upliftFeeBps). Leading to feePerLP > 1e18, the amount of exitFee exceed hookAdjustedAmountsOutRaw and cause underflow error with this line of code:

hookAdjustedAmountsOutRaw[i] -= exitFee;

PoC

  • If upliftFeeBps = 200, lpTokenDepositValueChange rise to 51 (price increase 52 times). User can not call removeLiquidityProportional function.

  • Place this test in UpliftExample.t.sol

  • Then in /2024-12-quantamm/pkg/pool-hooks run forge test --mt test_tooMuch_exitFee_userCanNotWithdraw -vvvv. Look into the terminal, the removeLiquidityProportional transaction will revert with [Revert] panic: arithmetic underflow or overflow (0x11) error.

function test_tooMuch_exitFee_userCanNotWithdraw() public {
LPNFT lpNft = upliftOnlyRouter.lpNFT();
​
// Add liquidity so Bob has BPT to remove liquidity.
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)]
.toMemoryArray();
​
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount,
false,
bytes('')
);
vm.stopPrank();
​
// Price increased 52 times
skip(1 days);
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < tokens.length; ++i) {
prices[i] = int256(i) * 52e18;
}
updateWeightRunner.setMockPrices(pool, prices);
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.startPrank(bob);
​
// Bob can not remove liquidity due to underflow
vm.expectRevert();
upliftOnlyRouter.removeLiquidityProportional(
bptAmount,
minAmountsOut,
false,
pool
);
vm.stopPrank();
}

Tools Used

  • Manual review.

  • Foundry

Recommendations

When calculating feePerLP:

  • Do not multifly upliftFeeBps with lpTokenDepositValueChange.

if (localData.lpTokenDepositValueChange > 0) {
- feePerLP =
- (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
- 10000;
+ feePerLP = (uint256(feeDataArray[i].upliftFeeBps) * 1e18) / 10000;
}

  • Or, have a maxWithdrawalFeeBps variable.

+ uint64 public immutable maxWithdrawalFeeBps = 1000; // 10%
.
.
.
if (localData.lpTokenDepositValueChange > 0) {
feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
}
+ if (feePerLP > (maxWithdrawalFeeBps * 1e18) / 10000) {
+ feePerLP = (maxWithdrawalFeeBps * 1e18) / 10000;
+ }
​
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!