aman - [M-1] Vesting Schedule has not check with current Time , which may result in setting the time for vesting in past.

[sherlock-admin]

aman

medium

[M-1] Vesting Schedule has not check with current Time , which may result in setting the time for vesting in past.

Summary

The vesting period may be provided in past mistakenly , allowing users of the escrow to claim all tokens simultaneously, exempt from any vesting restrictions. https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/2d14c2b84b69c53a45c81aa4f907af9617f9a94f/rio-vesting-escrow/src/VestingEscrowFactory.sol#L51-L75

Vulnerability Detail

The factory contract will deploy new Escrow Vesting Contract using deployVestingContract this function takes vestingStart as argument from user. However there is no check on start time to be after or equal to current time, If start time is provided in Past , the user will able to claim all token without any vesting restrictions applied. likelihood is low (if admin or deployer provide time in past mistakenly). impact is high(the fund will be claimed without any vesting restrictions applied).
that is why I rated this vulnerability as Medium.

Impact

The User is able to claim all tokens with out any vesting restrictions applied due to wrong vestingStart time.

Code Snippet

Admin creates new vesting schedule using deployVestingContract function. However admin mistakenly set vestingStart in the past. Since there is no check of if(vestingStart < uint40(block.timestamp)), this transaction is valid and VestingEscrow is created. Vesting recipients calls the `claim`` function and receive entire vest amount.

POC:

```javascript function testPastEscrowTime() public { // wrap block time to 100 days before creation of vesting vm.warp(block.timestamp + 100 days); deployVestingEscrow( VestingEscrowConfig({ amount: 1 ether, recipient: address(this), vestingDuration: 10 days, vestingStart: uint40(block.timestamp-40 days), cliffLength: 1 days, isFullyRevokable: false, initialDelegateParams: ozVotingAdaptor.encodeDelegateCallData(RANDOM_GUY) }) ); assertEq(deployedVesting.locked(), 0 ); assertEq(deployedVesting.unclaimed(), deployedVesting.totalLocked()); vm.prank(recipient); deployedVesting.claim(recipient, type(uint256).max); assertEq(deployedVesting.unclaimed(), 0); } ```

add this test inside VestingEscrow.t.sol and run with command forge test --mt testPastEscrowTime.

Tool used

Manual Review

Recommendation

Add following check in VestingEscrowFactory.sol:deployVestingContract function.

    if(vestingStart < uint40(block.timestamp)) revert WRONG_STARTTIME();

git diff

@@ -14,6 +14,9 @@ interface IVestingEscrowFactory {
     /// @notice Thrown when the amount is zero
     error INVALID_AMOUNT();

+    /// @notice Thrown when the vestingStart is in past
+    error WRONG_STARTTIME();
+

@@ -61,7 +61,8 @@ contract VestingEscrowFactory is IVestingEscrowFactory, Ownable2Step {
         if (cliffLength > vestingDuration) revert INVALID_VESTING_CLIFF();
         if (recipient == address(0)) revert INVALID_RECIPIENT();
         if (amount == 0) revert INVALID_AMOUNT();
-
+        if(vestingStart < uint40(block.timestamp)) revert WRONG_STARTTIME();

Duplicate of #101