QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: high
Valid

`Vault.removeLiquidity()` with `shouldCallAfterRemoveLiquidity = true` will be reverted.

danielarmstrong

Summary

Vault.removeLiquidity() with shouldCallAfterRemoveLiquidity = true will be reverted.
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L431-L570

Vulnerability Details

UpliftOnlyExample.sol#onAfterRemoveLiquidity() function is as follows.

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));
​
...
​
514@> 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++) {
523@> uint256 exitFee = localData.amountsOutRaw[i].mulDown(localData.feePercentage);
​
if (localData.adminFeePercent > 0) {
526@> 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(),
541@> maxAmountsIn: localData.accruedQuantAMMFees,
542@> minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18,
kind: AddLiquidityKind.PROPORTIONAL,
userData: bytes("")
})
);
...
}
​
...
​
return (true, hookAdjustedAmountsOutRaw);
}

On L541~542, the protocol mints bpt with maxAmountIn = localData.accruedQuantAMMFees.
The localData.accruedQuantAMMFees is calculated on L526 with rounding down.
And output data are all calculated with rounding down.
Because of rounding down, maxAmountIn can be less than amount needed for minting bptAmount.

Proof of Concept

We say that there are 2 tokens in a pool. In onAfterRemoveLiquidity(), And bptAmountIn = 100.00...200e18 and localData.feeAmount = 1.00...2e18(* 1e18). So feePercentage is 1%(0.01e18).
For output tokens [token0, token1], amountsOutRaw[0] = 100.00...199e18, amountsOutRaw[1] = 100e18.
We say that adminFeePercent = 50%(=0.5e18).
Then, on L541, accruedQuantAMMFees becomes [0.5e18, 0.5e18].
And on L542, minBptAmountOut is 1.00...2e18 * 0.5 = 0.50...1e18.
We say that decimals of token0 is 18 and is standard token.
Then, its decimalScalingFactor and tokenRate are all 1.
In this case, the amount of token0 to mint bpt is ceil(0.50...1e18 * 100.00...199e18 / 100.00...200e18) = 0.50...1e18 > 0.5e10(maxAmountsIn).
So this call is reverted.

We can see this fact from following codes.

Vault.addLiquidity() function is as follows.

function addLiquidity(
AddLiquidityParams memory params
)
external
onlyWhenUnlocked
withInitializedPool(params.pool)
returns (uint256[] memory amountsIn, uint256 bptAmountOut, bytes memory returnData)
{
...
​
// The bulk of the work is done here: the corresponding Pool hook is called, and the final balances
// are computed. This function is non-reentrant, as it performs the accounting updates.
//
// Note that poolData is mutated to update the Raw and Live balances, so they are accurate when passed
// into the AfterAddLiquidity hook.
//
// `amountsInScaled18` will be overwritten in the custom case, so we need to pass it back and forth to
// encapsulate that logic in `_addLiquidity`.
uint256[] memory amountsInScaled18;
@> (amountsIn, amountsInScaled18, bptAmountOut, returnData) = _addLiquidity(
poolData,
params,
maxAmountsInScaled18
);
​
...
}

_addLiquidity() function is as follows.

function _addLiquidity(
PoolData memory poolData,
AddLiquidityParams memory params,
uint256[] memory maxAmountsInScaled18
)
internal
nonReentrant
returns (
uint256[] memory amountsInRaw,
uint256[] memory amountsInScaled18,
uint256 bptAmountOut,
bytes memory returnData
)
{
LiquidityLocals memory locals;
locals.numTokens = poolData.tokens.length;
amountsInRaw = new uint256[](locals.numTokens);
// `swapFeeAmounts` stores scaled18 amounts first, and is then reused to store raw amounts.
uint256[] memory swapFeeAmounts;
​
if (params.kind == AddLiquidityKind.PROPORTIONAL) {
@> bptAmountOut = params.minBptAmountOut;
// Initializes the swapFeeAmounts empty array (no swap fees on proportional add liquidity).
swapFeeAmounts = new uint256[](locals.numTokens);
​
@> amountsInScaled18 = BasePoolMath.computeProportionalAmountsIn(
poolData.balancesLiveScaled18,
_totalSupply(params.pool),
bptAmountOut
);
} else if (params.kind == AddLiquidityKind.DONATION) {
poolData.poolConfigBits.requireDonationEnabled();
​
swapFeeAmounts = new uint256[](maxAmountsInScaled18.length);
bptAmountOut = 0;
amountsInScaled18 = maxAmountsInScaled18;
} ...
​
// At this point we have the calculated BPT amount.
if (bptAmountOut < params.minBptAmountOut) {
revert BptAmountOutBelowMin(bptAmountOut, params.minBptAmountOut);
}
​
_ensureValidTradeAmount(bptAmountOut);
​
for (uint256 i = 0; i < locals.numTokens; ++i) {
uint256 amountInRaw;
​
// 1) Calculate raw amount in.
{
uint256 amountInScaled18 = amountsInScaled18[i];
_ensureValidTradeAmount(amountInScaled18);
​
// If the value in memory is not set, convert scaled amount to raw.
if (amountsInRaw[i] == 0) {
// amountsInRaw are amounts actually entering the Pool, so we round up.
// Do not mutate in place yet, as we need them scaled for the `onAfterAddLiquidity` hook.
@> amountInRaw = amountInScaled18.toRawUndoRateRoundUp(
poolData.decimalScalingFactors[i],
poolData.tokenRates[i]
);
​
amountsInRaw[i] = amountInRaw;
} else {
// Exact in requests will have the raw amount in memory already, so we use it moving forward and
// skip downscaling.
amountInRaw = amountsInRaw[i];
}
}
​
IERC20 token = poolData.tokens[i];
​
// 2) Check limits for raw amounts.
728@> if (amountInRaw > params.maxAmountsIn[i]) {
revert AmountInAboveMax(token, amountInRaw, params.maxAmountsIn[i]);
}
​
...
}
​
...
}

By L728, the call can be reverted.

Impact

From this vulnerability, a user cannot remove liquidity from protocol.

Tools Used

Manual review

Recommendations

We have to modify UpliftOnlyExample.sol#onAfterRemoveLiquidity() function as follows.

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));
​
...
​
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);
++ uint256 exitFee = localData.amountsOutRaw[i].mulUp(localData.feePercentage);
if (localData.adminFeePercent > 0) {
-- localData.accruedQuantAMMFees[i] = exitFee.mulDown(localData.adminFeePercent);
++ localData.accruedQuantAMMFees[i] = exitFee.mulUp(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) {
++ uint256 bptAmount = localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18;
++ if(bptAmount > 100){
++ bptAmount -= 100;
++ }else{
++ bptAmount = 0;
++ }
++ if(bptAmount > 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("")
++ })
++ );
++ }
-- _vault.addLiquidity(
-- AddLiquidityParams({
-- pool: localData.pool,
-- to: IUpdateWeightRunner(_updateWeightRunner).getQuantAMMAdmin(),
-- maxAmountsIn: localData.accruedQuantAMMFees,
-- minBptAmountOut: localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18, // @audit: this calculation is not suitable yet.
-- kind: AddLiquidityKind.PROPORTIONAL,
-- userData: bytes("")
-- })
-- );
...
}
​
...
​
return (true, hookAdjustedAmountsOutRaw);
}
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_onAfterRemoveLiquidity_vault.addLiquidity_rounding_precision_DoS

Likelihood: High, multiple rounding down and little value can trigger the bug Impact: High, DoS the removal.

Support

FAQs

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