Summary
The afterUpdate function does not handle the case correclty where a user transfers their position (NFT) to themselves (_from == _to). This results in incorrect updates to the poolsFeeData array, leading to data corruption and locked funds.
Vulnerability Details
When a user transfers their position to themselves, the array undergoes faulty reordering, causing an overwritten position and locking of funds in the contract.
uint256 feeDataArrayLength = feeDataArray.length;
uint256 tokenIdIndex;
bool tokenIdIndexFound = false;
for (uint256 i; i < feeDataArrayLength; ++i) {
if (feeDataArray[i].tokenID == _tokenID) {
tokenIdIndex = i;
tokenIdIndexFound = true;
break;
}
}
if (tokenIdIndexFound) {
if (_to != address(0)) {
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);
if (tokenIdIndex != feeDataArrayLength - 1) {
for (uint i = tokenIdIndex + 1; i < feeDataArrayLength; i++) {
delete feeDataArray[i - 1];
feeDataArray[i - 1] = feeDataArray[i];
}
}
delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();
}
}
https://github.com/Cyfrin/2024-12-quantamm/blob/a775db4273eb36e7b4536c5b60207c9f17541b92/pkg/pool-hooks/contracts/hooks-quantamm/UpliftOnlyExample.sol#L614
For a user with position amounts[100, 200, 300], transferring their first position (100) to themselves results in [200, 300, 0]. The original first position (100) is overwritten, and a zero entry remains in the array.
function testSelfTransfer() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.startPrank(bob);
uint256 depositAmount = bptAmount / 150;
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, depositAmount, false, bytes(""));
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, 2 * depositAmount, false, bytes(""));
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, 3 * depositAmount, false, bytes(""));
vm.stopPrank();
LPNFT lpNft = upliftOnlyRouter.lpNFT();
vm.prank(bob);
lpNft.transferFrom(bob, bob, 1);
vm.stopPrank();
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].amount,
2 * depositAmount,
"bptAmount mapping should be 2x"
);
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[1].amount,
3 * depositAmount,
"bptAmount mapping should be 3x"
);
assertEq(
upliftOnlyRouter.getUserPoolFeeData(pool, bob)[2].amount,
0,
"bptAmount mapping should be 0"
);
}
Impact
The overwritten or deleted position cannot be accessed or retrieved by the user, effectively locking funds in the contract.
Tools Used
Manual
Recommendations
Explicitly handle self-transfers to either disallow or skip processing them.
