`ILOVest`'s vest schedule is incorrectly validated which breaks key invariant of Vesting

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-06-vultisig/blob/main/src/base/ILOVest.sol#L43

Vulnerability details

Impact

Stakeholders can bypass the vesting invariant and withdraw tokens early.

Proof of Concept

Vesting is a critical functionality provided by ILOPool for investors and the project.

During the initialization of ILOPool, vestingConfigs are validated through the _validateSharesAndVests function.

90:       _validateSharesAndVests(_project.launchTime, params.vestingConfigs);

The _validateSharesAndVests function calls _validateVestSchedule to validate the vesting schedule.

    function _validateSharesAndVests(uint64 launchTime, VestingConfig[] memory vestingConfigs) internal pure {
        uint16 totalShares;
        uint16 BPS = 10000;
        for (uint256 i = 0; i < vestingConfigs.length; i++) {
            // SNIP: Validation
@->         _validateVestSchedule(launchTime, vestingConfigs[i].schedule);
            totalShares += vestingConfigs[i].shares;
        }
        // total shares should be exactly equal BPS
        require(totalShares == BPS, "TS");
    }

Here's the implementation of _validateVestSchedule:

    function _validateVestSchedule(uint64 launchTime, LinearVest[] memory schedule) internal pure {
        require(schedule[0].start >= launchTime, "VT");
        uint16 BPS = 10000;
        uint16 totalShares;
        uint64 lastEnd;
        uint256 scheduleLength = schedule.length;
        for (uint256 i = 0; i < scheduleLength; i++) {
            // vesting schedule must not overlap
@->         require(schedule[i].start >= lastEnd, "VT");
            lastEnd = schedule[i].end;
            // we need to subtract fist in order to avoid int overflow
            require(BPS - totalShares >= schedule[i].shares, "VS");
            totalShares += schedule[i].shares;
        }
        // total shares should be exactly equal BPS
        require(totalShares == BPS, "VS");
    }

Important to note here:

• The current implementation assumes that if schedule[i].start >= lastEnd, the vesting schedule works correctly without overlap. However, it does not check if schedule[i].end > schedule[i].start.

Without this check, the following scenario can occur:

• schedule[i].start = 100 for a recipient when launch time is t = 80.
• Project can smartly set schedule[i].end = 80.

This bypasses the overlap check. Code Flow will be:

1. for i = 0:

   • schedule[i].start >= lastEnd is true as lastEnd is 0 initially.
   • lastEnd is set to 80.

2. for i = 1:

   • Any value above 80 is allowed for schedule[i].start.

This allows projects to bypass the vesting invariant and withdraw tokens early while Investors can be stuck with there tokens vested. Here's a proof of concept (PoC):

1. Add _getLinearVestingBypass function in Mock.t.sol and update schedule in _getVestingConfigs:

+     function _getLinearVestingBypass() internal pure returns (IILOVest.LinearVest[] memory linearVestConfigs) {
+         linearVestConfigs = new IILOVest.LinearVest[](2);
+         linearVestConfigs[0] = IILOVest.LinearVest({
+                     shares: 3000, // 30% 
+                     start: LAUNCH_START,
+                     end: LAUNCH_START + 1
+             });
+         linearVestConfigs[1] = IILOVest.LinearVest({
+                     shares: 7000, // 70% 
+                     start: VEST_START_1,
+                     end: VEST_END_1
+             });
+     }

      function _getVestingConfigs() internal pure returns (IILOVest.VestingConfig[] memory vestingConfigs) {
        vestingConfigs = new IILOVest.VestingConfig[](4);
        vestingConfigs[0] = IILOVest.VestingConfig({
                    shares: 2000, // 20%
                    recipient: address(0),
                    schedule: _getLinearVesting()
            });
        vestingConfigs[1] = IILOVest.VestingConfig({
                    shares: 3000, // 30%
                    recipient: TREASURY_RECIPIENT,
-                   schedule: _getLinearVesting()
+                   schedule: _getLinearVestingBypass()
            });

2. Add testClaimPoC function in ILOPool.t.sol:

    function testClaimPoC() external {
        _launch();
        vm.warp(LAUNCH_START + 1);
        uint256 tokenId = IILOPool(iloPool).tokenOfOwnerByIndex(TREASURY_RECIPIENT, 0);

        (uint128 unlockedLiquidity, uint128 claimedLiquidity) = IILOVest(iloPool).vestingStatus(tokenId);
        assertEq(uint256(unlockedLiquidity), 16200000000000000001756);
    }

3. Run the testClaimPoC test of ILOPool.t.sol:

  forge test --match-test testClaimPoC

Result of Test:

    Ran 1 test for test/ILOPool.t.sol:ILOPoolTest
    [PASS] testClaimPoC() (gas: 2818529)
    Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 340.90ms (3.16ms CPU time)

    Ran 1 test suite in 345.59ms (340.90ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

The test successfully allowed the stakeholder (TREASURY_RECIPIENT in this case) to claim the tokens at launch time while investors still cannot withdraw funds until the VEST_START_0 period.

Tools Used

VS Code

Recommended Mitigation Steps

Add validation in _validateVestSchedule to ensure schedule[i].end >= schedule[i].start:

    function _validateVestSchedule(uint64 launchTime, LinearVest[] memory schedule) internal pure {

        // SNIP

        for (uint256 i = 0; i < scheduleLength; i++) {
            // vesting schedule must not overlap
-           require(schedule[i].start >= lastEnd, "VT");
+           require(schedule[i].start >= lastEnd && schedule[i].end >= schedule[i].start, "VT");
            lastEnd = schedule[i].end;
            // we need to subtract fist in order to avoid int overflow
            require(BPS - totalShares >= schedule[i].shares, "VS");
            totalShares += schedule[i].shares;
        }

Assessed type

Context

[Breeje16]

Hi @alex-ppg,

The validator has rejected the issue with the following rationale:

Working as intended as circumvented by lastEnd. Project admin should not commit such mistakes,Owner mistake in setting start and end. QA at best.

However, there is an incorrect assumption by the validator: the project admin is trusted.

• ILOManager is a general-purpose contract that can be used by any project for fundraising, not just Vultisig. So Owner of ILOManager is trusted but Project Admin can be from any project (may or may not be trusted). Even in Readme here, there is no mention of Project owner to be trusted.

• The essence of the documentation here is that any project can set up their ILO, and the locking of tokens will "adhere" to the vesting schedule.

• The schedule mechanism in the code below implies that the Project Treasury's (or any other) vesting schedule must start after the end of the investor's vesting schedule, which is an important invariant from the investor's point of view. But This report shows how Project owners can break this invariant.

    for (uint256 i = 0; i < scheduleLength; i++) {
        // vesting schedule must not overlap
@->     require(schedule[i].start >= lastEnd, "VT");
        lastEnd = schedule[i].end;
        // we need to subtract fist in order to avoid int overflow
        require(BPS - totalShares >= schedule[i].shares, "VS");
        totalShares += schedule[i].shares;
    }

So IMO, If ILOManager was built only for Vultisig, then this issue should have been QA as the validator suggested. However, the reported vulnerability allows an arbitrary project that might not be trusted to break the invariant (claim the tokens before investors and immediately after the launch). This is a straight forward Rug Pull Attack Vector and hence should be a valid issue.

[alex-ppg]

Hey @Breeje16, thank you for the PJQA contribution. I will preface all validation repository finding responses by stating that they are not evaluated by judges directly and are only evaluated by the validators if they are deemed unsatisfactory.

The project administrator is a privileged role and the relevant SC guidelines around administrator mistakes apply here. Attempting to argue otherwise goes against the ethos of vulnerability reporting as vulnerabilities should always be reported in the category they are meant to.

This paragraph is included in all of my responses and confirms that no further feedback is expected in this submission as PJQA has concluded. You are free to refute any of my statements factually, however, I strongly implore you to do this with actual code references and examples.