QuantAMM

Summary

The UpliftOnlyExample contract relies exclusively on its internal bookkeeping (poolsFeeData) for ownership checks when users withdraw liquidity tokenized as NFTs. Because it does not validate actual NFT ownership via lpNFT.ownerOf(tokenID), an old owner is able to withdraw funds after transferring the NFT to a new owner.

Vulnerability Details

The vulnerability resides in the removeLiquidityProportional function. This function only checks if msg.sender has deposit records in poolsFeeData[pool][msg.sender] but does not confirm if msg.sender still owns the associated NFT at the ERC721 level (lpNFT.ownerOf(tokenId)).

If the afterUpdate hook, which synchronizes poolsFeeData with the actual NFT ownership, is not called during a transfer or fails, the internal data remains outdated. This allows the old owner of the NFT to withdraw liquidity even after transferring the NFT to a new owner.

The root cause lies in the contract's assumption that poolsFeeData is always accurate, which is not guaranteed in scenarios where afterUpdate is bypassed.

• UpliftOnlyExample.sol - removeLiquidityProportional

• Location: The removeLiquidityProportional function checks if there are deposits registered under the user in poolsFeeData[pool][msg.sender], but it does not confirm whether msg.sender is still the real owner of the NFT in the ERC721 contract.

• Cause: If the NFT transfer (e.g., transferFrom) does not call afterUpdate or fails to do so, the records in poolsFeeData remain outdated. The "old owner" is still listed as the owner in poolsFeeData, even though the NFT is now held by another address in the lpNFT contract.

• Effect: The contract does not include require(lpNFT.ownerOf(tokenID) == msg.sender), allowing the old owner, identified internally, to withdraw liquidity that no longer belongs to them.

Impact

The root cause centers on the contract’s failure to confirm real NFT ownership on withdrawal. This allows:

• Unauthorized Withdrawals: The old NFT owner (who has since transferred the NFT) remains recognized in poolsFeeData and can still initiate a withdrawal.

• High Financial Risk: If significant liquidity is tokenized, the new NFT owner loses funds, and the old owner gains them without rightful entitlement.

• Desynchronization: Internal bookkeeping and actual ERC721 ownership become unsynchronized, producing confusion and potentially undermining trust in the system.

Proof of Concept

Below is a test named testVulnerability_MissingAfterUpdateAllowsOldOwnerWithdraw that illustrates the issue. It deposits liquidity for Bob, then force-changes the NFT’s ownership to Alice without calling afterUpdate. At this point, Bob is no longer the real owner at the ERC721 level, but the contract still considers him eligible to withdraw:

• Add the following test to pkg/pool-hooks/test/foundry/UpliftExample.t.sol:

Explanation

1. Step 1: Bob deposits liquidity and receives an NFT (tokenId: 1). The deposit is recorded in poolsFeeData[pool][bob].

2. Step 2: The NFT is force-transferred to Alice by manipulating storage. Now, Alice owns the NFT in the lpNFT contract, but poolsFeeData is not updated.

3. Step 3: Bob successfully withdraws liquidity because the contract does not verify real ownership via lpNFT.ownerOf.

Logs confirm that Bob's final withdraw call succeeds, even though Alice is the real NFT owner

Tools Used

• Foundry: Used to develop and run tests validating that Bob, the old NFT holder, withdraws after ownership has changed.

• Manual Code Review: Confirms that removeLiquidityProportional never checks lpNFT.ownerOf(tokenID).

Recommendations

1. Check Real Ownership:
   Implement a direct check in removeLiquidityProportional similar to:

   require(lpNFT.ownerOf(tokenId) == msg.sender, "Not the real owner");

   This prevents an old owner from withdrawing.

2. Enforce afterUpdate:
   In the LPNFT contract, automatically invoke afterUpdate in _beforeTokenTransfer or _afterTokenTransfer so poolsFeeData stays aligned with actual ownership.

3. Remove Redundant Bookkeeping:
   Consider using lpNFT.ownerOf(tokenId) to drive all ownership checks instead of storing parallel records in poolsFeeData. This avoids potential desynchronization.

4. Implement _verifyRealNftOwnership in removeLiquidityProportional
   Incorporate a dedicated function (e.g., _verifyRealNftOwnership) to confirm that the caller truly holds the NFT at the ERC721 level. For instance:

   // Private function that iterates each deposit record and verifies NFT ownership

   function _verifyRealNftOwnership(

   address pool,

   address user,

   uint256 bptAmountIn

   ) private view {

   FeeData[] memory userFeeData = poolsFeeData[pool][user];

   uint256 totalFound;

   ​

   for (uint256 i = 0; i < userFeeData.length; i++) {

   uint256 tokenId = userFeeData[i].tokenID;

   if (lpNFT.ownerOf(tokenId) != user) {

   revert WithdrawalByNonOwner(user, pool, bptAmountIn);

   }

   totalFound += userFeeData[i].amount;

   }

   ​

   // Prevent the user from withdrawing more than the total they truly own

   if (bptAmountIn > totalFound) {

   revert WithdrawalByNonOwner(user, pool, bptAmountIn);

   }

   }

   ​

   function removeLiquidityProportional(

   uint256 bptAmountIn,

   uint256[] memory minAmountsOut,

   bool wethIsEth,

   address pool

   ) external payable returns (uint256[] memory amountsOut) {

   _verifyRealNftOwnership(pool, msg.sender, bptAmountIn);

   ...

   }

   This code ensures the old owner cannot withdraw if the NFT is no longer in their possession.

Validation Through Testing

• After adding the recommended mitigation function _verifyRealNftOwnership to the UpliftOnlyExample contract, include the following test in pkg/pool-hooks/test/foundry/UpliftExample.t.sol:

Test Result

Result: The logs show that after Bob deposits and receives an NFT (tokenId = 1), we forcibly reassign the NFT to Alice’s address without calling afterUpdate. When Bob tries to withdraw, the contract checks real NFT ownership and correctly prevents Bob from withdrawing, confirming the mitigation works as intended.