Summary
The UpliftOnlyExample hook allows transfer deposit NFT to oneself. Because this situation is incorrectly handled in afterUpdate it puts the mapping used for tracking NFTs an invalid state (clears the values and leaves an empty entry). User loses control of all funds related to the NFT and he can't withdraw any remaining NFTs unless transfered to a different account.
Vulnerability Details
LPNFT used for tracking deposits does not prevent transfers where from == to . But the corresponding afterUpdate which performs changes from the old to the new owner does not correctly handle this situation.
Here is an excerpt from UpliftOnlyExample - afterUpdate function where the deposit state is transferred to the new owner.
FeeData[] storage feeDataArray = poolsFeeData[poolAddress][_from];
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();
}
}
Impact
Users who transfer to self will lose any value associated with the NFT - Lose of value.
Because all the transferred NFT mapping values are zeroed-out, this include tokenID. The NFT is no longer transferrable or withdrawable. So essentially unremovable. - State pollution.
The zeroed out NFT will always be at the end of the mapping and left-over NFTs will also be stuck and unwithdrawable. They can only be recovered if transfered to another address. - Extra gas spending to recover any remaining value.
Argument could be made that there is no reason to do this and apart from a mistaken transfer this should not happen. But currently the code also resets fee data (lpTokenDepositValue) on transfer. Meaning the user can avoid paying higher upliftFeeBps and only pay minWithdrawalFeeBps if he continuesly moves the NFT. This creates a real incetive to try transferring to self, to avoid double transfer (own wallet -> secondary wallet -> own wallet) for value resetting.
Lose of value (depending on deposit can be high) + incetive do so (evade fees) - high.
Tools Used
Manual review + foundary tests using a modified testRemoveLiquidityDoublePositivePriceChange (run from UpliftOnlyExampleTest).
The test imitates the same example described in "vulnerability details". User creates 3 deposits and transfers the second one (ID 2) to self.
Resulting in deposits 1 and 3 being correct but the last deposit containing only zeroed out values
function testNftTransferToSelf() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(alice), usdc.balanceOf(alice)].toMemoryArray();
uint256 input = 1e18;
vm.prank(alice);
uint256[] memory amountsIn = upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, input, false, bytes(""));
vm.prank(alice);
amountsIn = upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, input, false, bytes(""));
vm.prank(alice);
amountsIn = upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, input, false, bytes(""));
int256[] memory prices = new int256[]();
prices[0] = 0;
prices[1] = 20e18;
updateWeightRunner.setMockPrices(pool, prices);
LPNFT nft = LPNFT(upliftOnlyRouter.lpNFT());
UpliftOnlyExample.FeeData[] memory aliceFeeData = upliftOnlyRouter.getUserPoolFeeData(pool, alice);
console2.log("---BEFORE TRANSFER---");
console2.log("aliceFeeData length", aliceFeeData.length);
console2.log("aliceFeeData[0] tokenID", aliceFeeData[0].tokenID);
console2.log("aliceFeeData[1] tokenID", aliceFeeData[1].tokenID);
console2.log("aliceFeeData[2] tokenID", aliceFeeData[2].tokenID);
vm.prank(alice);
nft.transferFrom(address(alice), address(alice), 2);
aliceFeeData = upliftOnlyRouter.getUserPoolFeeData(pool, alice);
console2.log("---AFTER TRANSFER---");
console2.log("Total deposits", aliceFeeData.length);
console2.log("Deposit 1");
console2.log("aliceFeeData amount", aliceFeeData[0].amount);
console2.log("aliceFeeData tokenID", aliceFeeData[0].tokenID);
console2.log("aliceFeeData lpTokenDepositValue", aliceFeeData[0].lpTokenDepositValue);
console2.log("aliceFeeData blockTimestampDeposit", aliceFeeData[0].blockTimestampDeposit);
console2.log("aliceFeeData upliftFeeBps", aliceFeeData[0].upliftFeeBps);
console2.log("copied Deposit 3");
console2.log("aliceFeeData amount", aliceFeeData[1].amount);
console2.log("aliceFeeData tokenID", aliceFeeData[1].tokenID);
console2.log("aliceFeeData lpTokenDepositValue", aliceFeeData[1].lpTokenDepositValue);
console2.log("aliceFeeData blockTimestampDeposit", aliceFeeData[1].blockTimestampDeposit);
console2.log("aliceFeeData upliftFeeBps", aliceFeeData[1].upliftFeeBps);
console2.log("zeroed Deposit 3");
console2.log("aliceFeeData amount", aliceFeeData[2].amount);
console2.log("aliceFeeData tokenID", aliceFeeData[2].tokenID);
console2.log("aliceFeeData lpTokenDepositValue", aliceFeeData[2].lpTokenDepositValue);
console2.log("aliceFeeData blockTimestampDeposit", aliceFeeData[2].blockTimestampDeposit);
console2.log("aliceFeeData upliftFeeBps", aliceFeeData[2].upliftFeeBps);
}
The test prints out:
---BEFORE TRANSFER---
aliceFeeData length 3
aliceFeeData[0] tokenID 1
aliceFeeData[1] tokenID 2
aliceFeeData[2] tokenID 3
---AFTER TRANSFER---
Total deposits 3
Deposit 1
aliceFeeData amount 1000000000000000000
aliceFeeData tokenID 1
aliceFeeData lpTokenDepositValue 500000000000000000
aliceFeeData blockTimestampDeposit 1682899200
aliceFeeData upliftFeeBps 200
copied Deposit 3
aliceFeeData amount 1000000000000000000
aliceFeeData tokenID 3
aliceFeeData lpTokenDepositValue 500000000000000000
aliceFeeData blockTimestampDeposit 1682899200
aliceFeeData upliftFeeBps 200
zeroed Deposit 3
aliceFeeData amount 0
aliceFeeData tokenID 0
aliceFeeData lpTokenDepositValue 0
aliceFeeData blockTimestampDeposit 0
aliceFeeData upliftFeeBps 0
Recommendations
The is no benefit in supporting transfers to self so the easiest solution would be to prevent them in LPNFT:
function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
if (to == previousOwner) {
revert("Can't transfer to self");
}
if (to != address(0) && previousOwner != address(0)) {
router.afterUpdate(previousOwner, to, tokenId);
}
}
Otherwise add explicit handling for when user transfers to self
function afterUpdate () {
...
if (tokenIdIndexFound) {
if (_from == _to) {
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
} else if (_to != address(0)) {
...
}
}
