Summary
onAfterRemoveLiquidity() is invoked by the router to calculate the fees a user must pay when removing liquidity. The issue lies in the fee calculation, where division occurs after multiplication. This approach results in precision loss due to Solidity's natural truncation behavior, causingto collect fewer fees than intended.
Vulnerability Details
onAfterRemoveLiquidity() is implemented 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));
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
});
hookAdjustedAmountsOutRaw = amountsOutRaw;
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 (localData.lpTokenDepositValueChange > 0) {
@> feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
}
else {
feePerLP = (uint256(minWithdrawalFeeBps) * 1e18) / 10000;
}
}
As observed, the function first calculates the change in the LP value and, if the value has increased, determines the corresponding fee using the first if.
However, the issue arises because the decimal adjustment (multiplying by 1e18) is not applied during the calculation of localData.lpTokenDepositValueChange but instead in feePerLP. This causes precision loss, leading to collect fewer fees than intended.
Impact
Due to precision loss in the fee calculation, fewer fees are collected.
POC
To better understand the issue, you can copy the following proof of concept (POC) into Remix and observe the precision loss. Use the following inputs: The inputs simulate an 18% price increase to observe the effect. Execute comparePrecisionLoss().
priceNow = 1000000000000000000 (1e18)
pricePrevious = 123213213213232333
upliftFeeBps = 1000 (10%)
pragma solidity ^0.8.0;
contract PrecisionLoss {
function divisionBeforeMultiplication(
uint256 priceNow,
uint256 pricePrevious,
uint256 upliftFeeBps
) public pure returns (uint256) {
uint256 priceChange = (priceNow - pricePrevious) / pricePrevious;
return (priceChange * upliftFeeBps * 1e18) / 10000;
}
function multiplicationBeforeDivision(
uint256 priceNow,
uint256 pricePrevious,
uint256 upliftFeeBps
) public pure returns (uint256) {
uint256 priceChange = (priceNow - pricePrevious) * 1e18 / pricePrevious;
return priceChange * upliftFeeBps / 10000;
}
function comparePrecisionLoss(
uint256 priceNow,
uint256 pricePrevious,
uint256 upliftFeeBps
) public pure returns (uint256 divBeforeMult, uint256 multBeforeDiv) {
divBeforeMult = divisionBeforeMultiplication(priceNow, pricePrevious, upliftFeeBps);
multBeforeDiv = multiplicationBeforeDivision(priceNow, pricePrevious, upliftFeeBps);
}
}
----------LOGS----------
divBeforeMult = 700000000000000000;
multBeforeDiv 711601267365216490;
As demonstrated, the result obtained by dividing before multiplication causes truncation, leading to a precision loss.
loss= 711601267365216490 - 700000000000000000 / 711601267365216490 = 11601267365216490 wei = 0.0116 eth
As you can see, the amount of loss is not insignificant, and this is just from a single transaction. Over time, with more transactions, this loss will accumulate.
Tools Used
Manual review.
Recommendations
To resolve the issue, adjust the decimals first by multiplying before performing the division.
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);
+ localData.lpTokenDepositValueChange =
+ (((int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue))) * 1e18) /
+ 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;
+ feePerLP =
+ (uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps)) /
+ 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;
}
///code....
}
