Summary
When quantAMMSwapFeeTake is greater than 0, there are certain scenarios where users will not be able to remove liquidity due to multiple instances of rounding down.
Vulnerability Details
Whenever a user removes liquidity, the onAfterRemoveLiquidity function checks if adminFeePercent > 0. If true, the admin fee is added back as liquidity to the weighted pool.
/contracts/hooks-quantamm/UpliftOnlyExample.sol
527:
528: localData.adminFeePercent = IUpdateWeightRunner(_updateWeightRunner).getQuantAMMUpliftFeeTake();
529:
531: for (uint256 i = 0; i < localData.amountsOutRaw.length; i++) {
532: uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);
533: if (localData.adminFeePercent > 0) {
534: localData.accruedQuantAMMFees[i] = exitFee.mulDown(localData.adminFeePercent);
535:
536: }
537:
538: localData.accruedFees[i] = exitFee - localData.accruedQuantAMMFees[i];
539: hookAdjustedAmountsOutRaw[i] -= exitFee;
543: }
544:
545: if (localData.adminFeePercent > 0) {
546: _vault.addLiquidity(
547: AddLiquidityParams({
548: pool: localData.pool,
549: to: IUpdateWeightRunner(_updateWeightRunner).getQuantAMMAdmin(),
550: maxAmountsIn: localData.accruedQuantAMMFees,
551: minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18,
552: kind: AddLiquidityKind.PROPORTIONAL,
553: userData: bytes("")
554: })
555: );
Due to multiple rounding downs, the maxAmountsIn becomes smaller than the amountInRaw, becuase the vault rounds up as shown below.
/contracts/Vault.sol
705:
706: {
707: uint256 amountInScaled18 = amountsInScaled18[i];
708: _ensureValidTradeAmount(amountInScaled18);
709:
710:
711: if (amountsInRaw[i] == 0) {
712:
713:
714: amountInRaw = amountInScaled18.toRawUndoRateRoundUp(
715: poolData.decimalScalingFactors[i],
716: poolData.tokenRates[i]
717: );
719:
720: amountsInRaw[i] = amountInRaw;
721: } else {
722:
723:
724: amountInRaw = amountsInRaw[i];
725: }
726: }
727:
728: IERC20 token = poolData.tokens[i];
729:
730:
731: if (amountInRaw > params.maxAmountsIn[i]) {
732: revert AmountInAboveMax(token, amountInRaw, params.maxAmountsIn[i]);
733: }
Following case describe this issue:
Bob adds liquidity.
Alice adds liquidity.
Alice removes liquidity.
LP adds liquidity.
As a result, Bob and LP are unable to remove their liquidity.
function testWithdrawDOS() public {
vm.startPrank(address(vaultAdmin));
updateWeightRunner.setQuantAMMSwapFeeTake(0.02e18);
vm.stopPrank();
uint256[] memory maxAmountsIn = [uint256(2e18), uint256(2e18)].toMemoryArray();
uint256 bptAmountDeposit = 2e18;
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDeposit, false, bytes(""));
vm.stopPrank();
vm.startPrank(alice);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDeposit, false, bytes(""));
vm.stopPrank();
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.startPrank(alice);
upliftOnlyRouter.removeLiquidityProportional(bptAmountDeposit, minAmountsOut, false, pool);
vm.stopPrank();
vm.expectRevert();
vm.startPrank(bob);
upliftOnlyRouter.removeLiquidityProportional(bptAmountDeposit, minAmountsOut, false, pool);
vm.stopPrank();
vm.startPrank(lp);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDeposit, false, bytes(""));
vm.stopPrank();
vm.expectRevert();
vm.startPrank(lp);
upliftOnlyRouter.removeLiquidityProportional(bptAmountDeposit, minAmountsOut, false, pool);
vm.stopPrank();
}
Run
forge test --mt testWithdrawDOS -vvv
Impact
After the first removal of liquidity, no one else can remove their liquidity if UpliftFee is active.
Tools Used
Unit Testing, Manual Review
Recommendations
short term fix
@@ -523,7 +523,7 @@ contract UpliftOnlyExample is MinimalRouter, BaseHooks, Ownable {
uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);
if (localData.adminFeePercent > 0) {
- localData.accruedQuantAMMFees[i] = exitFee.mulDown(localData.adminFeePercent);
+ localData.accruedQuantAMMFees[i] = exitFee.mulUp(localData.adminFeePercent);
}
long term fix
