0xRstStn - `createVestingSchedule` allows to create vesting periods with a duration longer than the maximum (i.e. 1800 days)

[sherlock-admin4]

0xRstStn

medium

createVestingSchedule allows to create vesting periods with a duration longer than the maximum (i.e. 1800 days)

Summary

In contract ZivoeRewardsVesting, function createVestingSchedule is to be used for creating vesting periods. The maximum number of days for a vesting period is 1800 days but in reality much longer periods can be created.

Vulnerability Detail

One of the inputs to the function createVestingSchedule is daysToVest. This input is the number of days for the entire vesting period. Within createVestingSchedule, there is a statement requiring daysToVest to be less or equal to 1800 days:

require(daysToVest <= 1800 days, "ZivoeRewardsVesting::createVestingSchedule() daysToVest > 1800 days");

The problem comes because daysToVest is already defined in days so it is later multiplied by days when incorporated in vestingScheduleOf:

vestingScheduleOf[account].end = block.timestamp + daysToVest * 1 days;

So daysToVest could be up to 155,520,000 and still be valid.

Impact

Although this function is restricted and can only be invoked by ZVL or ITO (i.e. onlyZVLOrITO modifier), if by mistake it is invoked containing daysToVest in seconds instead of days, it will go through. Also, if the vesting created is non-revokable, the vesting schedule cannot be ended.

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/ZivoeRewardsVesting.sol#L381-L425

function createVestingSchedule(
        address account,
        uint256 daysToCliff,
        uint256 daysToVest,
        uint256 amountToVest,
        bool revokable
    ) external onlyZVLOrITO {
        require(
            !vestingScheduleSet[account], "ZivoeRewardsVesting::createVestingSchedule() vestingScheduleSet[account]"
        );
        require(
            IERC20(vestingToken).balanceOf(address(this)) - vestingTokenAllocated >= amountToVest,
            "ZivoeRewardsVesting::createVestingSchedule() amountToVest > vestingToken.balanceOf(address(this)) - vestingTokenAllocated"
        );
        require(daysToVest <= 1800 days, "ZivoeRewardsVesting::createVestingSchedule() daysToVest > 1800 days"); //@audit here daysToVest <= 1800 days. Since daysToVest is measured in days and 1800 days is meaured in seconds, daysToVest can be way bigger than intended. Same for daysToCliff
        require(daysToCliff <= daysToVest, "ZivoeRewardsVesting::createVestingSchedule() daysToCliff > daysToVest");
        require(
            IZivoeITO_ZivoeRewardsVesting(IZivoeGlobals_ZivoeRewardsVesting(GBL).ITO()).seniorCredits(account) == 0
                && IZivoeITO_ZivoeRewardsVesting(IZivoeGlobals_ZivoeRewardsVesting(GBL).ITO()).juniorCredits(account) == 0,
            "ZivoeRewardsVesting::createVestingSchedule() seniorCredits(_msgSender) > 0 || juniorCredits(_msgSender) > 0"
        );

        vestingScheduleSet[account] = true;
        vestingTokenAllocated += amountToVest;

        vestingScheduleOf[account].start = block.timestamp;
        vestingScheduleOf[account].cliff = block.timestamp + daysToCliff * 1 days;
        vestingScheduleOf[account].end = block.timestamp + daysToVest * 1 days;
        vestingScheduleOf[account].totalVesting = amountToVest;
        vestingScheduleOf[account].vestingPerSecond = amountToVest / (daysToVest * 1 days);
        vestingScheduleOf[account].revokable = revokable;

        emit VestingScheduleCreated(
            account,
            vestingScheduleOf[account].start,
            vestingScheduleOf[account].cliff,
            vestingScheduleOf[account].end,
            vestingScheduleOf[account].totalVesting,
            vestingScheduleOf[account].vestingPerSecond,
            vestingScheduleOf[account].revokable
        );

        _stake(amountToVest, account);
    }

Tool used

Foundry In this test, a vesting schedule with daysToCliff of 30 days and daysToVestof 120 days is invoked by mistake. It can be seen it goes through and creates it.

To run this test, include it within Test_ZivoeRewardsVesting.sol

function test_ZivoeRewardsVesting_createVestingSchedule_daysToVest_aboveMax() public {
        assert(zvl.try_createVestingSchedule(address(vestZVE), address(poe), 30 days, 120 days, 100 ether, false));
        (
            uint256 start,
            uint256 cliff,
            uint256 end,
            uint256 totalVesting,
            uint256 totalWithdrawn,
            uint256 vestingPerSecond,
            bool revokable
        ) = vestZVE.viewSchedule(address(poe));

        console.log(
            "cliff: %d days from start => %d years from start", (cliff - start) / 1 days, (cliff - start) / 1 days / 365
        );
        console.log(
            "end: %d days from start => %d years from start", (end - start) / 1 days, (end - start) / 1 days / 365
        );
    }

The logs resulted from this test is:

Ran 1 test for src/TESTS_Core/Test_ZivoeRewardsVesting.sol:Test_ZivoeRewardsVesting [PASS] test_ZivoeRewardsVesting_createVestingSchedule_daysToVest_aboveMax() (gas: 367656) Logs:

cliff: 2592000 days from start => 7101 years from start end: 10368000 days from start => 28405 years from start

Recommendation

Remove days from the require statement where daysToVest is checked to be <= 1800 days

require(daysToVest <= 1800, "ZivoeRewardsVesting::createVestingSchedule() daysToVest > 1800 days");

Duplicate of #151

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

low, dup of #30. ZVL is trusted admin which is expected to provide correct values anyway. So the severity is low for incorrect sanity check which doesn't influence anything.