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

UpliftOnlyExample.sol :: onAfterRemoveLiquidity() amountLeft is not checked to ensure it equals 0, allowing users to withdraw more tokens than intended.

ivanfitro

Summary

onAfterRemoveLiquidity() is used to remove liquidity. The issue arises because amountLeft (the liquidity to be removed) is not checked to ensure it is 0. This oversight allows users to withdraw more tokens than intended.

Vulnerability Details

onAfterRemoveLiquidity() is implemented as folows:

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) {
address userAddress = address(bytes20(userData));
​
AfterRemoveLiquidityData memory localData = AfterRemoveLiquidityData({
pool: pool,
bptAmountIn: bptAmountIn,
amountsOutRaw: amountsOutRaw,
minAmountsOut: new uint256[](amountsOutRaw.length),
accruedFees: new uint256[](amountsOutRaw.length),
accruedQuantAMMFees: new uint256[](amountsOutRaw.length),
currentFee: minWithdrawalFeeBps,
feeAmount: 0,
prices: IUpdateWeightRunner(_updateWeightRunner).getData(pool),
lpTokenDepositValueNow: 0,
lpTokenDepositValueChange: 0,
lpTokenDepositValue: 0,
tokens: new IERC20[](amountsOutRaw.length),
feeDataArrayLength: 0,
amountLeft: 0,
feePercentage: 0,
adminFeePercent: 0
});
// We only allow removeLiquidity via the Router/Hook itself so that fee is applied correctly.
hookAdjustedAmountsOutRaw = amountsOutRaw;
​
//this rounding faxvours the LP
localData.lpTokenDepositValueNow = getPoolLPTokenValue(localData.prices, pool, MULDIRECTION.MULDOWN);
​
FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
localData.feeDataArrayLength = feeDataArray.length;
localData.amountLeft = bptAmountIn;
for (uint256 i = localData.feeDataArrayLength - 1; i >= 0; --i) {
localData.lpTokenDepositValue = feeDataArray[i].lpTokenDepositValue;
​
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;
}
​
// if the deposit is less than the amount left to burn, burn the whole deposit and move on to the next
@> if (feeDataArray[i].amount <= localData.amountLeft) {
uint256 depositAmount = feeDataArray[i].amount;
localData.feeAmount += (depositAmount * feePerLP);
​
localData.amountLeft -= feeDataArray[i].amount;
​
lpNFT.burn(feeDataArray[i].tokenID);
​
delete feeDataArray[i];
​
feeDataArray.pop();
​
@> if (localData.amountLeft == 0) {
break;
}
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft);
break;
}
}
​
localData.feePercentage = (localData.feeAmount) / bptAmountIn;
​
hookAdjustedAmountsOutRaw = localData.amountsOutRaw;
localData.tokens = _vault.getPoolTokens(localData.pool);
​
localData.adminFeePercent = IUpdateWeightRunner(_updateWeightRunner).getQuantAMMUpliftFeeTake();
​
// Charge fees proportional to the `amountOut` of each token.
for (uint256 i = 0; i < localData.amountsOutRaw.length; i++) {
uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);
​
if (localData.adminFeePercent > 0) {
localData.accruedQuantAMMFees[i] = exitFee.mulDown(localData.adminFeePercent);
}
​
localData.accruedFees[i] = exitFee - localData.accruedQuantAMMFees[i];
hookAdjustedAmountsOutRaw[i] -= exitFee;
// Fees don't need to be transferred to the hook, because donation will redeposit them in the Vault.
// In effect, we will transfer a reduced amount of tokensOut to the caller, and leave the remainder
// in the pool balance.
}
​
if (localData.adminFeePercent > 0) {
_vault.addLiquidity(
AddLiquidityParams({
pool: localData.pool,
to: IUpdateWeightRunner(_updateWeightRunner).getQuantAMMAdmin(),
maxAmountsIn: localData.accruedQuantAMMFees,
minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18,
kind: AddLiquidityKind.PROPORTIONAL,
userData: bytes("")
})
);
emit ExitFeeCharged(
userAddress,
localData.pool,
IERC20(localData.pool),
localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18
);
}
​
if (localData.adminFeePercent != 1e18) {
// Donates accrued fees back to LPs.
_vault.addLiquidity(
AddLiquidityParams({
pool: localData.pool,
to: msg.sender, // It would mint BPTs to router, but it's a donation so no BPT is minted
maxAmountsIn: localData.accruedFees, // Donate all accrued fees back to the pool (i.e. to the LPs)
minBptAmountOut: 0, // Donation does not return BPTs, any number above 0 will revert
kind: AddLiquidityKind.DONATION,
userData: bytes("") // User data is not used by donation, so we can set it to an empty string
})
);
}
​
return (true, hookAdjustedAmountsOutRaw);
}

The function iterates through the array to withdraw the requested liquidity (amountLeft) by subtracting the liquidity deposited in each NFT until amountLeft == 0. However, the issue is that this condition is only used to break the loop, assuming that all necessary NFTs were burned to extract the requested liquidity. If amountLeft != 0, the transaction does not revert, allowing users to withdraw more tokens than intended.

This happens because there is no check to ensure that amountLeft == 0, which would confirm that the combined liquidity of the NFTs is sufficient to meet the withdrawal request.

Finally, the function returns hookAdjustedAmountsOutRaw, representing the adjusted amount after applying fees. This flaw allows users to withdraw more liquidity than their NFTs actually hold, leading to unintended behavior.

Impact

Users can withdraw more than intended stealing funds from the contract.

POC

To better understand the issue, let's use an example:

  1. Bob owns 1 NFT with amount = 100.

  2. onAfterRemoveLiquidity() is called by the router with amountsOutRaw = 1000 and bptAmountIn = 1000.

  3. The function iterates to calculate the fees. Since there is only 1 NFT available to burn, the calculation results in localData.amountLeft = 1000 - 100 = 900 and the fee is determined to be 50.

  4. The loop finishes, leaving localData.amountLeft = 900, meaning there is still 900 liquidity left to be withdrawn. However, because there is no check to ensure amountLeft == 0, the execution continues despite the insufficient NFTs.

  5. The amountsOutRaw are adjusted, resulting in hookAdjustedAmountsOutRaw = 1000 - 10 = 990.

  6. As a result, the user receives 990 tokens but only burns 100 worth of liquidity, effectively taking more tokens than they should.

Tools Used

Manual review.

Recommendations

To resolve the issue, add a check to ensure that localData.amountLeft == 0. This will verify that the NFTs provide sufficient liquidity to cover the requested removal amount; otherwise, revert the transaction.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

invalid_removing_liquidity_never_check_if_amount_is_owned_by_the_sender

Read bugs with that tag: invalid_onAfterRemoveLiquidity_loop_underflow Because of that implementation, trying to remove more will lead to an underflow.

Support

FAQs

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

Give us feedback!