Recipients can claim all tokens not in intended pattern, if startTime mistakenly is set to past.

Summary

The VestingEscrowFactory contract allows the creation of a vesting schedule with a start time that is set in the past. This means the recipient could potentially claim all their vested tokens immediately, which might not be the intended behavior by the administrators of the contract.

Vulnerability Detail

The unclaimed function in the VestingEscrow contract calculates the amount of tokens that the recipient can claim at a given time. It does this by checking the time when the recipient wants to claim the tokens (claimTime) and comparing it with the vestingStart time.

The claimTime is determined by the smaller value between the current block timestamp (block.timestamp) and the disabledAt timestamp. The disabledAt timestamp is the time when the vesting schedule ends.

The _totalVestedAt(claimTime) function calculates the total number of tokens that have vested at claimTime. If claimTime is after the end of the vesting period, then all tokens would have vested. However, if claimTime is during the vesting period, only a portion of the tokens would have vested.

So, if vestingStart is set in the past, _totalVestedAt(claimTime) would return the total amount of tokens if all tokens have vested at claimTime. This means the unclaimed function would return the total amount of tokens, minus the totalClaimed amount.

Therefore, the claim function would allow the recipient to claim all their tokens at once, even before the vesting schedule starts. This is the vulnerability.

If vestingStart is not set in the past, the claimTime would be in the future relative to vestingStart, and _totalVestedAt(claimTime) would return the correct amount of tokens that have vested at claimTime. Therefore, the unclaimed function would return the correct amount of tokens that the recipient can claim, and the claim function would correctly update the totalClaimed amount.

Impact

Likelihood of this scenario is very less, but impact can be very high Because of this mistake by administrators , recipients will be able to claim all at once, which is not intended , as there gradual release of tokens with time is being intended.

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L51

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, a check should be added in the deployVestingContract function to ensure that the start time is in the future as give below:

require(vestingStart > block.timestamp, "Start time must be in the future");

This will prevent the creation of vesting schedules with a start time in the past, thus preventing the recipient from claiming more tokens than they should be able to at a given time .

It is more dangerous to not include this check because transaction will simply succeed even though vestingStart is set to past which means that there is a chance of administrators not noticing this.

Just this one single addition of code would be prevent from unintended claiming if any by chance mistake happens.

Duplicate of #101

sherlock-audit / 2024-01-rio-vesting-escrow-judging

bhilare_ - Recipients can claim all tokens not in intended pattern, if startTime mistakenly is set to past. #47