denzi_ - Incorrect Updation of _checkpoints[account] in `ZivoeRewardsVesting::revokeVestingSchedule()` can mess up accounting of votes

[sherlock-admin4]

denzi_

medium

Incorrect Updation of _checkpoints[account] in ZivoeRewardsVesting::revokeVestingSchedule() can mess up accounting of votes

Summary

The _checkpoints[account] in ZivoeRewardsVesting::revokeVestingSchedule() is not updated correctly, which can lead to incorrect accounting of votes.

Vulnerability Detail

Vestings are created for a user in ZivoeRewardsVesting contract when the ITO calls ZivoeRewardsVesting::createVestingSchedule. At the end of this function, a function call is present :

    _stake(amountToVest, account);

The _stake function contains the following block of code

        require(amount > 0, "ZivoeRewardsVesting::_stake() amount == 0");

        _totalSupply = _totalSupply.add(amount);
        _writeCheckpoint(_totalSupplyCheckpoints, _add, amount);
        _writeCheckpoint(_checkpoints[account], _add, amount);
        _balances[account] = _balances[account].add(amount);

Here we update the _totalSupplyCheckpoints and _checkpoints[account] by the amount which is to be vested to the account.

In revokeVestingSchedule(), we have the following code

    uint256 amount = amountWithdrawable(account);
        uint256 vestingAmount = vestingScheduleOf[account].totalVesting;

        vestingTokenAllocated -= amount;

        vestingScheduleOf[account].totalWithdrawn += amount;
        vestingScheduleOf[account].totalVesting = vestingScheduleOf[account].totalWithdrawn;
        vestingScheduleOf[account].cliff = block.timestamp - 1;
        vestingScheduleOf[account].end = block.timestamp;

        vestingTokenAllocated -= (vestingAmount - vestingScheduleOf[account].totalWithdrawn);

        _totalSupply = _totalSupply.sub(vestingAmount);

        _writeCheckpoint(_totalSupplyCheckpoints, _subtract, vestingAmount); // @audit-info reduces _totalSupplyCheckpoints by totalVesting
        _writeCheckpoint(_checkpoints[account], _subtract, amount); // @audit-issue reduces _checkpoints[account] by accrued vesting so far
        // more code below

The issue is that the code updates _totalSupplyCheckpoints and _checkpoints[account]in the _stake() with the vestingScheduleOf[account].totalVesting amount given by the ITO. The totalVesting amount will slowly accrue towards the user as the block.timestamp reaches the vestingScheduleOf[account].end. So this means the amountWithdrawable() will only return the totalVesting if block.timestamp has passed vestingScheduleOf[account].end

_checkpoints[account] being subtracted by amount which is the so far withdrawable vesting amount causes incorrect accounting of votes. Withdrawable vesting amount will not equal to totalVesting which was initially staked for the user

Proof of Concept

Remove the onlyZVLOrITO modifier from createVestingSchedule() and revokeVestingSchedule(). Also remove the require check for credits in createVestingSchedule. Create a new test file in zivoe-core-foundry directory called ZivoeRewardsVesting.t.sol and add the code from the gist

Proof of Concept

Impact

User will have more voting power than they should, this also messes up the account for _totalSupplyCheckpoints. It should never be possible for _checkpoints[account] > _totalSupplyCheckpoints furthermore the User has manipulated voting power which they can use without having anything staked.

Code Snippet

createVestingSchedule()

_stake()

[revokeVestingSchedule()][https://github.com/sherlock-audit/2024-03-zivoe/blob/main/zivoe-core-foundry/src/ZivoeRewardsVesting.sol#L429-L467]

Tool used

Manual Review

Recommendation

Change the following line of code in revokeVestingSchedule()

        _writeCheckpoint(_totalSupplyCheckpoints, _subtract, vestingAmount);
-       _writeCheckpoint(_checkpoints[account], _subtract, amount);
+       _writeCheckpoint(_checkpoints[account], _subtract, vestingAmount);

Duplicate of #118

[sherlock-admin2]

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

panprog commented:

high, dup of #118, incorrect checkpoints amount subtracted during revokeVestingSchedule causes permanent inflated amount of user votes.