QuantAMM
QuantAMM
49,600 OP
View results
Previous Next
Submission Details
Severity: medium
Valid

The user will lost his liquidity if he transfers the LP NFT to himself

biakia

Summary

In the afterUpdate function of the UpliftOnlyExample contract, there exists a logical flaw when an NFT's ownership is updated but the _from and _to addresses are the same. If the NFT is being updated and transferred to the same address (i.e., _from == _to), the contract will incorrectly delete the feeDataArray , lending to loss of LP NFT.

Vulnerability Details

The function afterUpdate is a post-transfer hook that is called after an overridden _update function in an LPNFT contract:

/// @inheritdoc ERC721
function _update(address to, uint256 tokenId, address auth) internal override returns (address previousOwner) {
previousOwner = super._update(to, tokenId, auth);
//_update is called during mint, burn and transfer. This functionality is only for transfer
if (to != address(0) && previousOwner != address(0)) {
//if transfering the record in the vault needs to be changed to reflect the change in ownership
router.afterUpdate(previousOwner, to, tokenId);
}
}

In afterUpdate function, it will reorder the entire array by the following code:

if (tokenIdIndex != feeDataArrayLength - 1) {
//Reordering the entire array could be expensive but it is the only way to keep true FILO
for (uint i = tokenIdIndex + 1; i < feeDataArrayLength; i++) {
delete feeDataArray[i - 1];
feeDataArray[i - 1] = feeDataArray[i];
}
}
​
delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();

However, this function does not handle the scenario where _from == _to. If _from and _to are the same address, the function will attempt to:

  1. Update the FeeData entry for the _tokenID.

  2. Push the updated FeeData entry into the poolsFeeData array for the same address (_to == _from).

  3. Reorder the FeeData array for _from by shifting elements and deleting the last entry.

This could result in duplicate entries or incorrect state updates in the poolsFeeData array, as the same FeeData entry is being pushed into the array while the original entry is still being processed.

Consider Bob has the folliwng LP NFT:

[
{ tokenID: 123, lpTokenDepositValue: 100, blockTimestampDeposit: 1000, upliftFeeBps: 10 },
{ tokenID: 456, lpTokenDepositValue: 200, blockTimestampDeposit: 2000, upliftFeeBps: 20 }
]

He transfers tokenID=123 NFT to himself, this NFT will be pushed into the poolsFeeData array:

[
{ tokenID: 123, lpTokenDepositValue: 100, blockTimestampDeposit: 1000, upliftFeeBps: 10 },
{ tokenID: 456, lpTokenDepositValue: 200, blockTimestampDeposit: 2000, upliftFeeBps: 20 },
{ tokenID: 123, lpTokenDepositValue: 100, blockTimestampDeposit: 1000, upliftFeeBps: 10 }
]

And then the function will reorder the poolsFeeData array by shifting elements. After reordering, the array becomes:

[
{ tokenID: 456, lpTokenDepositValue: 200, blockTimestampDeposit: 2000, upliftFeeBps: 20 }
{ tokenID: 456, lpTokenDepositValue: 200, blockTimestampDeposit: 2000, upliftFeeBps: 20 },
{ tokenID: 123, lpTokenDepositValue: 100, blockTimestampDeposit: 1000, upliftFeeBps: 10 }
]

And then the function will execute the following code:

delete feeDataArray[feeDataArrayLength - 1];
feeDataArray.pop();

To note that the feeDataArrayLength is queried before the push action:

FeeData[] storage feeDataArray = poolsFeeData[poolAddress][_from];
​
uint256 feeDataArrayLength = feeDataArray.length;
...
...
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);

So the feeDataArrayLength is 2 instead of 3, so the actual delete logic is :

delete feeDataArray[2 - 1];
feeDataArray.pop();

the poolsFeeData array becomes:

[
{ tokenID: 456, lpTokenDepositValue: 200, blockTimestampDeposit: 2000, upliftFeeBps: 20 }
{ tokenID: 0, lpTokenDepositValue: 0, blockTimestampDeposit: 0, upliftFeeBps: 0 },
]

We can see that the NFT( tokenID=123 ) has been deleted.

POC

You can add following test to UpliftExample.t.sol :

//@audit user will lost his liquidity if he transfer the LP NFT to himself
function testSelfTransferDeposits() public {
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.startPrank(bob);
uint256 bptAmountDeposit = bptAmount / 150;
uint256[] memory tokenIndexArray = new uint256[]();
for (uint256 i = 0; i < 2; i++) {
tokenIndexArray[i] = i + 1;
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmountDeposit, false, bytes(""));
skip(1 days);
}
UpliftOnlyExample.FeeData[] memory bobFees = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
assertNotEq(bobFees[0].tokenID, 0, "first tokenId should not be 0");
assertNotEq(bobFees[1].tokenID, 0, "second tokenId should not be 0");
vm.stopPrank();
​
LPNFT lpNft = upliftOnlyRouter.lpNFT();
​
vm.startPrank(bob);
​
lpNft.transferFrom(bob, bob, 1);
​
bobFees = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
assertNotEq(bobFees[0].tokenID, 0, "first tokenId should not be 0");
assertNotEq(bobFees[1].tokenID, 0, "second tokenId should not be 0");
vm.stopPrank();
}
​

run forge test --match-test testSelfTransferDeposits :

Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[FAIL: second tokenId should not be 0: 0 == 0] testSelfTransferDeposits() (gas: 958680)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 16.24ms (893.13µs CPU time)

We can see that the second LP will be deleted.

In summary, because the afterUpdate function does not check if _from is equal to _to, it unintentionally performs updates and reaches a reordering logic that is irrelevant when no ownership change occurs. This can result in critical loss of liquidity NFT.

Impact

The impact is HIGH because it will cause funds loss and the likelihood is MEDIUM, so the severity is HIGH.

Tools Used

Manual Review

Recommendations

Consider following fix:

function afterUpdate(address _from, address _to, uint256 _tokenID) public {
if (msg.sender != address(lpNFT)) {
revert TransferUpdateNonNft(_from, _to, msg.sender, _tokenID);
}
if (_from == _to){return;}
...
...
Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_afterUpdate_erase_self_transfer

Likelihood: Low, when users wants to transfer tokens they already own to themselves. Impact: High, funds loss.

Appeal created

foufrix Auditor
10 months ago
atharv181 Auditor
10 months ago
0xpep7 Auditor
10 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_afterUpdate_erase_self_transfer

Likelihood: Low, when users wants to transfer tokens they already own to themselves. Impact: High, funds loss.

Support

FAQs

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