Summary
The nftPool[tokenID] mapping is not reset to 0 after the corresponding tokenID is burned. As a result, querying the mapping with the burned tokenID still returns the pool address associated with the tokenID. This creates a discrepancy because the tokenID no longer exists, and the mapping should return 0.
Vulnerability Details
After user remove liquidity the NFTs are burned if amount is 0 but the issue is that the NFT is burned but if we query nftPool[tokenID] it will return the pool address
which may be misleading.
POC: add this in UpliftExample.t.sol
function test_return_pool_after_token_burn() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), dai.balanceOf(bob)].toMemoryArray();
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
LPNFT lpNft = upliftOnlyRouter.lpNFT();
uint256 bptAmountDepositBob = 1e18;
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDepositBob, false, bytes(""));
upliftOnlyRouter.removeLiquidityProportional(bptAmountDepositBob, minAmountsOut, false, pool);
vm.stopPrank();
address poolAddress = upliftOnlyRouter.nftPool(1);
assertEq(poolAddress, pool);
}
function addLiquidityProportional(
address pool,
uint256[] memory maxAmountsIn,
uint256 exactBptAmountOut,
bool wethIsEth,
bytes memory userData
) external payable saveSender(msg.sender) returns(uint256[] memory amountsIn) {
nftPool[tokenID] = pool;
}
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) {
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;
}
}
}
Impact
queries for burned tokenID will return misleading information.
Recommendations
Reset Mapping Value:
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) {
// snip...
// 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);
+ nftPool[feeDataArray[i].tokenID] = address(0);
delete feeDataArray[i];
feeDataArray.pop();
if (localData.amountLeft == 0) {
break;
}
} else {
feeDataArray[i].amount -= localData.amountLeft;
localData.feeAmount += (feePerLP * localData.amountLeft);
break;
}
/ snip...
}
