Summary
The UpliftOnlyExample contract fails to clear the nftPool mapping when NFTs are burned during removeLiquidityProportional, leading to stale data.
Vulnerability Details
mapping(uint256 => address) public nftPool;
lpNFT.burn(feeDataArray[i].tokenID);
delete feeDataArray[i];
feeDataArray.pop();
The issue exists in the UpliftOnlyExample contract where the nftPool mapping maintains associations between NFT token IDs and pool addresses. When an NFT is burned during liquidity removal, the mapping is not cleared.
function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
previousOwner = super._update(to, tokenId, auth);
if (to != address(0) && previousOwner != address(0)) {
router.afterUpdate(previousOwner, to, tokenId);
}
}
The issue is also related to the LPNFT._update function which doesn't handle the burn case.
POC
Add this to UpliftExample.t.sol and run it forge test --match-test testNftPoolMappingVulnerability -vvvv.
function testNftPoolMappingVulnerability() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
uint256 tokenId = 1;
assertEq(upliftOnlyRouter.nftPool(tokenId), pool, "Initial pool mapping incorrect");
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.startPrank(bob);
upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
vm.stopPrank();
assertEq(upliftOnlyRouter.nftPool(tokenId), pool, "Pool mapping should be cleared but still exists");
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
vm.stopPrank();
assertEq(upliftOnlyRouter.nftPool(tokenId), pool, "New deposit should create new mapping");
UpliftOnlyExample.FeeData[] memory feeData = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
assertEq(feeData.length, 1, "Should only have one active deposit");
assertEq(feeData[0].tokenID, 2, "New deposit should have new tokenId");
}
Trace:
├─ [71649] LPNFT::mint(bob)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: bob, tokenId: 1)
...
├─ [519] upliftOnlyRouter::nftPool(1) [staticcall]
│ └─ ← pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240]
First deposit.
├─ [5912] LPNFT::burn(1)
...
├─ [519] upliftOnlyRouter::nftPool(1) [staticcall]
│ └─ ← pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240]
After withdrawal.
├─ [45749] LPNFT::mint(bob)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: bob, tokenId: 2)
...
├─ [519] upliftOnlyRouter::nftPool(1) [staticcall]
│ └─ ← pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240]
Second deposit
Even though the NFT has been burned, the nftPool mapping still maintains the association between tokenId 1 and the pool address.
Impact
Invalid tokenId can still be used to access pool information.
Tools Used
Recommendations
Implement proper cleanup in removeLiquidityProportional.
function removeLiquidityProportional(...) {
...
lpNFT.burn(feeDataArray[i].tokenID);
delete nftPool[feeDataArray[i].tokenID];
delete feeDataArray[i];
feeDataArray.pop();
...
}
