0xk3y - Inadequate Validation of `startTime` parameter in VestingEscrowFactory Contract

[sherlock-admin]

0xk3y

medium

Inadequate Validation of startTime parameter in VestingEscrowFactory Contract

Summary

The VestingEscrowFactory lacks input validation for the vestingStart parameter in deployVestingContract() which is responsible for deploying escrow contracts. The lack of a check on vestingStart may lead to an overestimation of vested and locked token amounts, impacting the functionality of various contract methods.

Vulnerability Detail

VestingEscrowFactory.sol L51-69

function deployVestingContract(
        uint256 amount,
        address recipient,
        uint40 vestingDuration,
        uint40 vestingStart,
        uint40 cliffLength,
        bool isFullyRevokable,
        bytes calldata initialDelegateParams
    ) external returns (address escrow) {
        if (vestingDuration == 0) revert INVALID_VESTING_DURATION();
        if (cliffLength > vestingDuration) revert INVALID_VESTING_CLIFF();
        if (recipient == address(0)) revert INVALID_RECIPIENT();
        if (amount == 0) revert INVALID_AMOUNT();

        escrow = vestingEscrowImpl.clone(
            abi.encodePacked(
                address(this), token, recipient, vestingStart, vestingStart + vestingDuration, cliffLength, amount
            )
        );

VestingEscrow.sol L275

function _totalVestedAt(uint256 time) internal pure returns (uint256) {
        uint40 _startTime = startTime();
        uint256 _totalLocked = totalLocked();
        if (time < _startTime + cliffLength()) {
            return 0;
        }
        return Math.min(_totalLocked * (time - _startTime) / (endTime() - _startTime), _totalLocked);
    }

if a value from the past is mistakenly passed when deploying an escrow, _totalVestedAt() on the escrow contract could overstate the amount of tokens vested.

VestingEscrow.sol L123

function unclaimed() public view returns (uint256) {
        if (isFullyRevoked) return 0;

        uint256 claimTime = Math.min(block.timestamp, disabledAt);
        return _totalVestedAt(claimTime) - totalClaimed;
    }

VestingEscrow.sol L137

function claim(address beneficiary, uint256 amount) external onlyRecipient returns (uint256) {
        uint256 claimable = Math.min(unclaimed(), amount);
        totalClaimed += claimable;

        token().safeTransfer(beneficiary, claimable);
        emit Claim(beneficiary, claimable);

        return claimable;
    }

This overstatement would affect the value returned in the unclaimed() function, allowing users to claim more tokens than they should before the intended vesting period.

VestingEscrow.sol L127

function locked() public view returns (uint256) {
        if (block.timestamp >= disabledAt) return 0;

        return totalLocked() - _totalVestedAt(block.timestamp);
    }

VestingEscrow.sol L167

function revokeUnvested() external onlyOwnerOrManager {
        uint256 revokable = locked();
        if (revokable == 0) revert NOTHING_TO_REVOKE();

        disabledAt = uint40(block.timestamp);

        token().safeTransfer(_owner(), revokable);
        emit UnvestedTokensRevoked(msg.sender, revokable);
    }

VestingEscrow.sol L181

function revokeAll() external onlyOwner {
        if (!isFullyRevokable) revert NOT_FULLY_REVOKABLE();
        if (isFullyRevoked) revert ALREADY_FULLY_REVOKED();

        uint256 revokable = locked() + unclaimed();
        if (revokable == 0) revert NOTHING_TO_REVOKE();

        isFullyRevoked = true;
        disabledAt = uint40(block.timestamp);

        token().safeTransfer(_owner(), revokable);
        emit VestingFullyRevoked(msg.sender, revokable);
    }

Additionally, the locked() function would understate the amount intended to be locked. This understatement affects the calculation of the revokable amount in revokeUnvested() and revokeAll(), potentially limiting the ability of the Owner or Manager to revoke tokens correctly or disabling the revocation functionality entirely via revert.

Impact

Incorrect startTime may overstate vested amounts and enable early claiming and limit the ability of the owner or manager to revoke tokens

Code Snippet

See above.

Tool used

Manual Review

Recommendation

Implement a validation check for the startTime parameter in the VestingEscrowFactory contract. This check should ensure that startTime is greater than the current block timestamp.

Duplicate of #101

[sherlock-admin2]

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

pratraut commented:

'valid as check on a start time is must to prevent owner deploying vesting contract with start time from past'