QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: low
Invalid

Decrementing loop overflow causing DoS in `onAfterRemoveLiquidity()`

TaiChi Audit Group

Summary

Decrementing loop overflow causing DoS in `onAfterRemoveLiquidity()`

Vulnerability Details

In the `onAfterRemoveLiquidity()` :


https://github.com/Cyfrin/2024-12-quantamm/blob/main/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L471

for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i)

When `i` decrements to 0 and the `--i` operation is executed, it triggers an overflow check, causing a revert and preventing the correct processing of the user's exit fee after withdrawing liquidity.

Add the following test in `UpliftOnlyExampleTest`:

function testOnAfterRemoveLiquidityDoS() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.prank(owner);
vault.forceUnlock();
vm.expectRevert(stdError.arithmeticError);
upliftOnlyRouter.onAfterRemoveLiquidity(
address(upliftOnlyRouter),
pool,
RemoveLiquidityKind.PROPORTIONAL,
bptAmount,
minAmountsOut,
minAmountsOut,
minAmountsOut,
bytes("")
);
}



[PASS] testOnAfterRemoveLiquidityDoS() (gas: 514284)

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 44.03ms (2.28ms CPU time)

Recommendations

Modify it as follows:

for (uint256 i = localData.feeDataArrayLength ; i > 0; --i)
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_onAfterRemoveLiquidity_loop_underflow

That’s definitely not the best way to handle that but there is indeed no impact. If someone tries to get more than their deposits, it must revert, and thanks to that "fancy mistake"(or genius code ?), it does.

Appeal created

icebear Submitter
10 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_onAfterRemoveLiquidity_loop_underflow

That’s definitely not the best way to handle that but there is indeed no impact. If someone tries to get more than their deposits, it must revert, and thanks to that "fancy mistake"(or genius code ?), it does.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.