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

Function within `UpliftOnlyExample.sol` named `onAfterRemoveLiquidity()` will always revert.

abdu1242112

Summary

Within a contract UpliftOnlyExample.sol function onAfterRemoveLiquidity() will always revert because of the implementation of for loop within it.

Vulnerability Details

Within onAfterRemoveLiquidity() function, for loop's uint variable "i" is decremented which will cause it to underflow and end up whole function to revert.

function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind,
uint256 bptAmountIn,
uint256[] memory,
uint256[] memory amountsOutRaw,
uint256[] memory,
bytes memory userData
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
.
.
.
@> for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
.
.
.
.

Link to code

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

Proof Of Concept

Copy this test function in any test file and run it.

function testforloop() public {
// Random array
uint8[5] memory a = [1,2,3,4,5];
​
for(uint256 b = a.length - 1; b >= 0; --b){
//empty
}
​
}

This test function will revert with this error:

[FAIL: panic: arithmetic underflow or overflow (0x11)] testforloop() (gas: 828)

Impact

This line will cause function to revert.

Likelihood : HIGH

Tools Used

Manual Review

Recommendations

Increment array variable to prevent underflow error.

function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind,
uint256 bptAmountIn,
uint256[] memory,
uint256[] memory amountsOutRaw,
uint256[] memory,
bytes memory userData
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
.
.
.
.
- for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
​
+ uint256 arrsize = localData.feeDataArrayLength;
​
+ for (uint256 i = 0; i < arrsize; 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.

Support

FAQs

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

Give us feedback!