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

Transfering NFT positions should not update the value of the position

draiakoo

Summary

Liquidity removal fees can be lowered by transfer NFT positions right before removing liquidity

Vulnerability Details

When someone removes liquidity, the onAfterRemoveLiquidity hook loops through all the positions from the user to fullfil the whole amount of BPT that the user wants to withdraw. From each position it computes the fee using the following method:

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) {
...
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;
}
}
...
}

It computes the difference of value from the position between the time that the user created the position and the current value. Depending on the difference of value it will compute a higher or lower fee. If there is not change in value it computes a base fee using the minWithdrawalFeeBps.
This can be tricked because when someone transfer the NFT to someone else it updated the value stored in the NFT just as if it would be a new deposit:

function afterUpdate(address _from, address _to, uint256 _tokenID) public {
if (msg.sender != address(lpNFT)) {
revert TransferUpdateNonNft(_from, _to, msg.sender, _tokenID);
}
​
address poolAddress = nftPool[_tokenID];
​
if (poolAddress == address(0)) {
revert TransferUpdateTokenIDInvaid(_from, _to, _tokenID);
}
​
int256[] memory prices = IUpdateWeightRunner(_updateWeightRunner).getData(poolAddress);
uint256 lpTokenDepositValueNow = getPoolLPTokenValue(prices, poolAddress, MULDIRECTION.MULDOWN);
​
FeeData[] storage feeDataArray = poolsFeeData[poolAddress][_from];
​
uint256 feeDataArrayLength = feeDataArray.length;
uint256 tokenIdIndex;
bool tokenIdIndexFound = false;
​
//find the tokenID index in the array
for (uint256 i; i < feeDataArrayLength; ++i) {
if (feeDataArray[i].tokenID == _tokenID) {
tokenIdIndex = i;
tokenIdIndexFound = true;
break;
}
}
​
if (tokenIdIndexFound) {
if (_to != address(0)) {
// Update the deposit value to the current value of the pool in base currency (e.g. USD) and the block index to the current block number
//vault.transferLPTokens(_from, _to, feeDataArray[i].amount);
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
​
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);
​
...
}
}
}

Hence, someone could just transfer his NFT just before removing liquidity in order to make the value difference to 0 and compute only the base fee.

Proof of Concept

  1. A user adds liquidity and an NFT is created with value 5

  2. The value of NFT position goes up to 10

  3. The user want to remove the liquidity but will have to pay a considerable fee

  4. He transfers the NFT to an alt account and then removes the liquidity

  5. Since the value of the NFT position will be updated, only the base fee will be accounted

Impact

Medium, the protocol will receive less fess

Tools Used

Manual review

Recommendations

Transfering the NFTs should NOT update the value of the position upon creating the position

Updates

Lead Judging Commences

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

finding_afterUpdate_bypass_fee_collection_updating_the_deposited_value

Likelihood: High, any transfer will trigger the bug. Impact: High, will update lpTokenDepositValue to the new current value without taking fees on profit.

Support

FAQs

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