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

localData.lpTokenDepositValueChange logic can cause an underflow in uplift fee calculation

jsondoge

Summary

Due to invalid formula used for localData.lpTokenDepositValueChange calculation, the following logic is prone to underflowing.

Vulnerability Details

Excerpt from UpliftOnlyExample.sol - onAfterRemoveLiquidity

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

We'll focus on price increase, since decrease will simply ignore the change in further execution.

The above math results in the localData.lpTokenDepositValueChange to be equivalent of "How many TIMES value changed minus one".

And further used in feePerLp calculation:

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

In the tests upliftFeeBps is set to 2% (200). Here we already can see that if the value increased 51 times (lpTokenDepositValueChange = (51 - 1) = 50) the result will be feePerLP = 1e18. Which is equal to a 100% fee and any further increase will cause it to go over 100% here:

...
if (feeDataArray[i].amount <= localData.amountLeft) {
​
uint256 depositAmount = feeDataArray[i].amount;
​
localData.feeAmount += (depositAmount * feePerLP); // feePerLp * depositAmount and below feePercentage cancel eachother out (in case they match)
...
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft); // feePerLp * depositAmount and below feePercentage cancel eachother out (in case they match)
break;
}
}
​
localData.feePercentage = (localData.feeAmount) / bptAmountIn; // <- feePercentage becomes = feePerLp = 1e18 < equivalent to 100%<
...

And in turn eventually cause an underflow here:

for (uint256 i = 0; i < localData.amountsOutRaw.length; i++) {
uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage); // feePercentage is above 100%
​
...
hookAdjustedAmountsOutRaw[i] -= exitFee; // exitFee is more than amounts out
...

Impact

In crypto industry it's not rare for tokens to shoot up in price. This will cause fee to become more than the total depositAmount and underflow. Adding the fact that deposits are stored and later cleared using FILO - the user deposit can stay uncleared for long periods of time which only increases the probably to hit the underflow. This will result in the inability to withdraw liquidity.

Tools Used

Manual Review + Foundry Test

Below is a modified testRemoveLiquidityDoublePositivePriceChange

import "forge-std/console2.sol";
...
​
function testRemoveLiquidity52TimesPriceIncrease() public {
// Add liquidity so bob has BPT to remove liquidity.
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
​
console2.log("bptAmount", bptAmount / 1e18); // i - received 2000 * 1e18 (deducted 1000 * 1e18 of each token)
​
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 1, "bptAmount mapping should be 1");
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].amount, bptAmount, "bptAmount mapping should be 0");
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].blockTimestampDeposit,
block.timestamp,
"bptAmount mapping should be 0"
);
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].lpTokenDepositValue,
500000000000000000,
"should match sum(amount * price)"
);
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].upliftFeeBps, 200, "fee");
​
// Initial total value of deposit: 5 * 1e17 (half tokens worth 0 and half worth 1e18)
// After price increase value is : 26 * 1e18 (half tokens worth 0 and half worth 52 * 1e18)
// 52 times increase (260 / 5 = 52). And feePerLp = 51 * 200 * 1e18 / 10000 = 1.02 * 1e18 (above 100%)
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < tokens.length; ++i) {
prices[i] = int256(i) * 52 * 1e18; // TIMES multiplier
}
updateWeightRunner.setMockPrices(pool, prices);
​
uint256 nftTokenId = 0;
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
​
BaseVaultTest.Balances memory balancesBefore = getBalances(bob);
​
vm.startPrank(bob);
console2.logString("--------------------------------Removing liquidity--------------------------------");
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
console2.logString("--------------------------------Liquidity removed--------------------------------");
vm.stopPrank();
}

The pring "---Liquidity removed---" is never reached because of the underflow. If the "TIMES multiplier" is decreased it will again pass.

Recommendations

If the current fee scaling is intended which sponsors confirmed. Then the only way to limit the underflow is limit the maximum feePerLp.

So something along the lines of:

if (localData.lpTokenDepositValueChange > 0) {
feePerLP = (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
​
feePerLP = feePerLP > 1e18 ? 1e18 : fePerLP; // Limit max fee to 100% or other fixed limit.
}
Updates

Lead Judging Commences

n0kto Lead Judge 4 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.