Point structure mismanagement Compromises Epoch Accuracy and leads to incorrect reward distributions

Summary

The _checkpoint() function in VotingEscrow is responsible for updating the global supply as well as user-level checkpoints and storing them in the form of a history of point structures.

But the calculation in this function is wrong.

Vulnerability Detail

The VotingEscrow contract maintains a history of checkpoints, each with a Point struct that records the current value of supply/amount of tokens locked (bias and slope) and time (ts) and the block number (blk) at that time. One set of points at user level and one set of points for global supply.

In the _checkPoint() function, the contract has a sub-logic to "Go over weeks to fill epoch history and calculate what the current point is". That uses an initial_last_point variable to store the state of the last checkpoint during the calculation loop and calculate the block number for new epochs. This logic can be seen here

Following is the relevant buggy code :


function _checkpoint(uint256 _tokenId, LockedBalance memory old_locked, LockedBalance memory new_locked) internal {
    // prev code ...
        Point memory last_point = Point({bias: 0, slope: 0, ts: block.timestamp, blk: block.number});
    if (_epoch > 0) {
        last_point = point_history[_epoch];
    }
    uint last_checkpoint = last_point.ts;

    // initial_last_point is used for extrapolation to calculate block number
    // (approximately, for *At methods) and save them
    // as we cannot figure that out exactly from inside the contract
    // @audit-issue : this initialLastPoint get updated when ever lastPoint get updated
 >>  Point memory initial_last_point = last_point;

    uint block_slope = 0; // dblock/dt
    if (block.timestamp > last_point.ts) {
        block_slope = (MULTIPLIER * (block.number - last_point.blk)) / (block.timestamp - last_point.ts);
    }
    // more code ...
}

However, due to Solidity's reference semantics for struct types in memory, the line Point memory initial_last_point = last_point; does not create an independent copy of lastPoint. Instead, initial_last_point and last_point points to the same data in memory. As a result, any subsequent modifications to last_point are also reflected in initial_past_point.

Solidity documentation states: Assigning a struct to a local variable or a function's return variable creates an independent copy if and only if the type of the variable is a storage reference..

This behavior is contrary to the intended use of initial_last_point, which should remain unchanged to accurately calculate the block number for each epoch. now Since initial_last_point is a memory variable that changes when ever last_point changes, this will lead to the following miscalculation:

last_point.ts = t_i;
last_point.blk = initial_last_point.blk + (block_slope * (t_i - initial_last_point.ts)) / MULTIPLIER;

Here, (t_i - initial_last_point.ts) is expected to be the time difference since the last checkpoint, but due to the shared reference, it is always zero, because t_i will always equal initial_last_point.ts ,Consequently, last_point.blk remains the same as initial_last_point.blk, causing all epochs to incorrectly reference the block number of the first checkpoint.

This is incorrect because in RewardsDistributor, we want to be able to correctly find the right checkpoint block in order to read the supply checkpoint values for that block, when rewards are distributed or claimed for past epochs. If the block that is stored is wrong, the corresponding values we access in RewardsDistributor will always be wrong.

The integrity of past supply checkpoint history is compromised, as the supply checkpoint history will be used in _find_timestamp_epoch in RewardsDistributor when checkpointing veSupply before distributing rewards (see Minter) the flow is Voter.distribute => Minter.update_period => RewardsDistributor.checkpoint_total_supply().

See here and here.

Impact

The _checkpoint() function is responsible for updating the global checkpoint into the state variable "point_history". The problem here arises because the global checkpoints are corrupted every time _checkpoint in VotingEscrow gets called.

"point_history" global checkpoint state is used in RewardsDistributor when finding supply checkpoints and distributing rewards based on that, which means all the veSupply checkpoints state in RewardsDistributor is also wrong and leads to incorrect distribution of rewards.

High severity because this causes inaccurate supply tracking leading to incorrect reward distributions everytime distribution is done over the whole life of all rewardsDistributor instances used in Velocimeter protocol.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/63818925987a5115a80eff4bd12578146a844cfd/v4-contracts/contracts/VotingEscrow.sol#L642

Tool used

Manual Review

Recommendation

Declare a new initial_last_point with the same values of last_point, instead of using direct memory reference :


  function _checkpoint(uint256 _tokenId, LockedBalance memory old_locked, LockedBalance memory new_locked) internal {
    // prev code ...
    Point memory last_point = Point({bias: 0, slope: 0, ts: block.timestamp, blk: block.number});
    if (_epoch > 0) {
        last_point = point_history[_epoch];
    }

    uint last_checkpoint = last_point.ts;
    // initial_last_point is used for extrapolation to calculate block number
    // (approximately, for *At methods) and save them
    // as we cannot figure that out exactly from inside the contract
+   Point memory initial_last_point = Point(last_point.bias, last_point.slope, last_point.ts, last_point.blk);
-   Point memory initial_last_point = last_point;

    uint block_slope = 0; // dblock/dt
    if (block.timestamp > last_point.ts) {
        block_slope = (MULTIPLIER * (block.number - last_point.blk)) / (block.timestamp - last_point.ts);
    }
    // more code ...
   }

Duplicate #226

nevillehuang commented 2 months ago

request poc

@dawiddrzala Could you assist in verifying if the impact here stated is correct?

sherlock-admin4 commented 2 months ago

PoC requested from @chinmay-farkya

Requests remaining: 17

dawiddrzala commented 2 months ago

just to add to it POC need to prove that it is impacting any part of the system, ( setting wrong blk number is not enough for it to be issue without impact )