Submission Details
Severity:
`Vault.removeLiquidity()` with `shouldCallAfterRemoveLiquidity = true` will be reverted.
Summary
A rounding issue in UpliftOnlyExample.sol#onAfterRemoveLiquidity() will cause a transaction revert for users as the protocol mints BPT with insufficient maxAmountIn.
Vulnerability Details
In `UpliftOnlyExample.sol#L541-L542`, the protocol mints BPT with `maxAmountIn = localData.accruedQuantAMMFees`. The `localData.accruedQuantAMMFees` is calculated on `L526` with rounding down. As a result, `maxAmountIn` can be less than the amount needed for minting `bptAmount`.
```solidity
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);
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);
}
```
On lines 541-542, the protocol mints BPT with `maxAmountIn = localData.accruedQuantAMMFees`. The `localData.accruedQuantAMMFees` is calculated on line 526 with rounding down, and output data are all calculated with rounding down. Due to this rounding down, `maxAmountIn` can be less than the amount needed for minting `bptAmount`.
Impact
The users cannot remove liquidity from the protocol, causing a denial of service.
## PoC
Let's consider a scenario where there are 2 tokens in a pool. In the `onAfterRemoveLiquidity()` function, assume `bptAmountIn = 100.00...200e18` and `localData.feeAmount = 1.00...2e18 (* 1e18)`. This results in a `feePercentage` of `1% (0.01e18)`.
For the output tokens `[token0, token1]`, let `amountsOutRaw[0] = 100.00...199e18` and `amountsOutRaw[1] = 100e18`. Assume `adminFeePercent = 50% (0.5e18)`.
On line 541, `accruedQuantAMMFees` becomes `[0.5e18, 0.5e18]`. On line 542, `minBptAmountOut` is calculated as `1.00...2e18 * 0.5 = 0.50...1e18`.
Assuming token0 has 18 decimals and is a standard token, its `decimalScalingFactor` and `tokenRate` are both `1`.
In this case, the amount of token0 required to mint BPT is `ceil(0.50...1e18 * 100.00...199e18 / 100.00...200e18) = 0.50...1e18`, which is greater than `0.5e18 (maxAmountsIn)`. Consequently, this call will revert.
We can see this fact from following codes.
Focus on `Vault.addLiquidity()`
https://github.com/Cyfrin/2024-12-quantamm/blob/main/pkg/vault/contracts/Vault.sol#L507-L591
```solidity
function addLiquidity(
AddLiquidityParams memory params
)
external
onlyWhenUnlocked
withInitializedPool(params.pool)
returns (uint256[] memory amountsIn, uint256 bptAmountOut, bytes memory returnData)
{
....................
uint256[] memory amountsInScaled18;
L569-> (amountsIn, amountsInScaled18, bptAmountOut, returnData) = _addLiquidity(
poolData,
params,
maxAmountsInScaled18
);
// AmountsIn can be changed by onAfterAddLiquidity if the hook charges fees or gives discounts.
// Uses msg.sender as the Router (the contract that called the Vault).
if (poolData.poolConfigBits.shouldCallAfterAddLiquidity()) {
// `hooksContract` needed to fix stack too deep.
IHooks hooksContract = _hooksContracts[params.pool];
amountsIn = poolData.poolConfigBits.callAfterAddLiquidityHook(
msg.sender,
amountsInScaled18,
amountsIn,
bptAmountOut,
params,
poolData,
hooksContract
);
}
}
```
Focus `_addLiquidity()` function
```solidity
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
);
................................
// 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.
L728-> if (amountInRaw > params.maxAmountsIn[i]) {
revert AmountInAboveMax(token, amountInRaw, params.maxAmountsIn[i]);
}
.....................
}
```
By line 728, the transaction can revert. This vulnerability prevents users from removing liquidity from the protocol.
Tools Used
Manual
Recommendations
## Recommended mitigation steps.
Modify `UpliftOnlyExample.sol#onAfterRemoveLiquidity()` function to use rounding up for fee calculations.
https://github.com/Cyfrin/2024-12-quantamm/blob/main/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L431-L570
```solidity
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("")
})
);
+ }
emit ExitFeeCharged(
userAddress,
localData.pool,
IERC20(localData.pool),
localData.feeAmount.mulDown(localData.adminFeePercent) / 1e18
);
}
...................
return (true, hookAdjustedAmountsOutRaw);
}
```
Updates
Lead Judging Commences
n0kto 10 months ago
Submission Judgement Published 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.
Prize pool breakdown
Total prize
49,600 OP
nSLOC
3,00017
OP / LOC
44,640
4,960
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.