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

Incorrect uplift fee calculation leads to LPs incurring more fees than expected

honour

Summary

According to the docs and white paper, the uplift fee is calculated only on the uplift/increase in the BPT value since deposit.

README

Each deposit tracks its LP token value in a base currency. On withdrawal, any uplift since deposit will be the subject of the fee.

White paper (page 19)

Withdrawal fees charged to LPs only on the increase in value LPs have received over the value they would have if they had HODLed

However the current Implementation does not follow this rule and the uplift fee calculated can exceed the fee % of the LPs uplift

Vulnerability Details

In UpliftOnlyExample::onAfterRemoveLiquidity,

function onAfterRemoveLiquidity(
//...
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
​
// ...SNIP...
​
//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;
}
}
​
// ...SNIP...
return (true, hookAdjustedAmountsOutRaw);
}

The issue is that on Ln#495 ( localData.feeAmount += (depositAmount * feePerLP)) in the case of an uplift the fee is applied over the entire BPT amount instead of only the portion equvalent to the uplift value.

Consider the following case:
Alice is withdrawing 1000 BPT, BPT value at deposit was 5 usd and BPT value now is 10 usd. The upliftFeeBps is 1000 bps = 10%

By Definition
Alice initial BPT value is 5000 usd (1000 * 5 usd) and her current BPT value is 10000 usd (1000 * 10 usd).
That's 5000 usd (100%) increase which is equivalent to 500 BPT in curent value (5000 / 10).
Alice uplift fee should now be 10% of her uplift of 500 BPT which is 50 BPT.

However from current Implementation

that's 100% increase
, or 10%

Alice uplift fee is now calculated as

100 BPT is 20% of the uplift of 500 BPT, meaning Alice pays double the expected fees because the uplift fee is taken from the entire depositAmount

Impact

Medium - incorrect uplift fee calculation , and as a result LPs incur more fees than expected

Tools Used

Manual Review

Recommendations

Uplift should be charged only on the uplifted portion of the depositAmount, in this case that can be calculated as


where,
$ $

This is effectively a single line change in the onAfterRemoveLiquidity function

function onAfterRemoveLiquidity(
...
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
​
// ...SNIP...
​
//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);
+ int256(localData.lpTokenDepositValueNow);
​
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
// ...SNIP...
return (true, hookAdjustedAmountsOutRaw);
}
Updates

Lead Judging Commences

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

finding_onAfterRemove_fees_are_applied_on_entire_amount

Likelihood: High, every withdraw with benefits. Impact: High, more fees taken and can even transform a benefit in loss.

Support

FAQs

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

Give us feedback!