Submission Details
Severity:
The uplift Fee can apply if price increase by up to 99%
Summary
The purpose of the Router/hook contract is to charge a fee when a user removes liquidity if the price has increased after the deposit. However, due to incorrect scaling, the uplift fee will not be applied if the price increases by up to 99%.
Vulnerability Details
When a user deposits assets, the router/hook contract fetches the price from the oracle, calculates the deposit value, and stores it in the FeeData mapping.
contracts/hooks-quantamm/UpliftOnlyExample.sol:249
249: //this requires the pool to be registered with the QuantAMM update weight runner
250: //as well as approved with oracles that provide the prices
251: uint256 depositValue = getPoolLPTokenValue(
252: IUpdateWeightRunner(_updateWeightRunner).getData(pool),
253: pool,
254: MULDIRECTION.MULDOWN
255: );
256:
257: poolsFeeData[pool][msg.sender].push(
258: FeeData({
259: tokenID: tokenID,
260: amount: exactBptAmountOut,
261: //this rounding favours the LP
262: lpTokenDepositValue: depositValue, // 0.5e18
263: //known use of timestamp, caveats are known.
264: blockTimestampDeposit: uint40(block.timestamp),
265: upliftFeeBps: upliftFeeBps
266: })
267: );
268:
At line 262, the current USD value is stored. When a user removes liquidity, the asset's price is recalculated and compared with the stored price. If there is an increase in price, the uplift fee is applied.
contracts/hooks-quantamm/UpliftOnlyExample.sol:474
474: localData.lpTokenDepositValueNow = getPoolLPTokenValue(localData.prices, pool, MULDIRECTION.MULDOWN);
475:
476: FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
477: localData.feeDataArrayLength = feeDataArray.length;
478: localData.amountLeft = bptAmountIn;
479: for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
480: localData.lpTokenDepositValue = feeDataArray[i].lpTokenDepositValue;
483: localData.lpTokenDepositValueChange =
484: ((int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) *1e18) /
485: int256(localData.lpTokenDepositValue);
486: uint256 feePerLP;
487: // if the pool has increased in value since the deposit, the fee is calculated based on the deposit value
488: if (localData.lpTokenDepositValueChange > 0) {
491: feePerLP =
492: (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
493: 10000;
494: }
495: // if the pool has decreased in value since the deposit, the fee is calculated based on the base value - see wp
496: else {
497: //in most cases this should be a normal swap fee amount.
498: //there always myst be at least the swap fee amount to avoid deposit/withdraw attack surgace.
499: feePerLP = (uint256(minWithdrawalFeeBps) * 1e18) / 10000;
500: }
The issue here is that if the price increases by 99%, no uplift fee will be applied.
Let’s suppose:
1. Deposit price at time of deposit = `1e18`, so deposit value = `1e18`.
2. Price at withdrawal time = `1.99e18`, so deposit value = `1.99e18`.
3. Calculation: `(1.99e18 - 1e18) / 1e18 = 0.99e18`.
Since Solidity downcasts `0.99e18` to `0`, no uplift fee will be applied.
Coded POC :
function test_price_increase_but_no_uplift_fee_applied() public {
// @audit-issue : poc for issue 1 where the price increate upto 99% but no uplift fee applied
// 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();
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < tokens.length; ++i) {
prices[i] = int256(i) * 1.99e18; // increate the price by 99%
}
updateWeightRunner.setMockPrices(pool, prices);
uint256 nftTokenId = 0;
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
BaseVaultTest.Balances memory balancesBefore = getBalances(bob);
vm.startPrank(bob);
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
BaseVaultTest.Balances memory balancesAfter = getBalances(bob);
uint256 feeAmountAmountPercent = ((bptAmount / 2) *
((uint256(upliftOnlyRouter.minWithdrawalFeeBps()) * 1e18) / 10000)) / ((bptAmount / 2));
uint256 amountOut = (bptAmount / 2).mulDown((1e18 - feeAmountAmountPercent));
// Bob gets original liquidity with no fee applied because of precision loss.
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");
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob).length, 0, "bptAmount 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");
}
run with command : forge test --mt test_price_increase_but_no_uplift_fee_applied
Impact
Due to the incorrect calculation, no uplift fee will be applied even if the price increases by up to 99%.
Tools Used
Manual Review
Recommendations
The best solution for this issue could be to scale the value by 1e18 and then divide by 1e18 at the final step, as shown below:
contract UpliftOnlyExample is MinimalRouter, BaseHooks, Ownable {
for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
localData.lpTokenDepositValue = feeDataArray[i].lpTokenDepositValue;
console.log("localData.lpTokenDepositValue" , uint256(localData.lpTokenDepositValue));
-
+ //i.e => (1.99e18 - 1e18)*1e18/1e18 =>0.99e18
localData.lpTokenDepositValueChange =
- (int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
- int256(localData.lpTokenDepositValue);
+ ((int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) * 1e18) /
+ int256(localData.lpTokenDepositValue); // now lpTokenDepositValueChange in 1e18 scale
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) { // @note : it will not work with bps less than 1
- console.log("UpLift Fee Applied");
+ // i.e (990000000000000000 * 5000 * 1e18 / 10000) / 1e18 => 0.45e18 which is exaclty 50% per profit
feePerLP =
- (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
- 10000;
+ ((uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
+ 10000)/1e18;
Updates
Lead Judging Commences
n0kto 10 months ago
Submission Judgement Published Assigned finding tags:
finding_onAfterRemoveLiquidity_lpTokenDepositValueChange_rounding_error_100%_minimum
Likelihood: High, every call to the function (withdraw) Impact: Low/Medium, uplift fees will be applied only when the price of one asset is doubled but fixed fees will still be collected.
Prize pool breakdown
Total prize
49,600 OP
nSLOC
3,00017
OP / LOC
44,640
4,960
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.