0xMosh - Owner can maliciously revoke the unclaimed tokens even after the vesting duration has been ended .

[sherlock-admin]

0xMosh

medium

Owner can maliciously revoke the unclaimed tokens even after the vesting duration has been ended .

Summary

Revoking functionality is for revoking the vesting funds when inside the vesting duration . But it is not ensured in the revokeAll function . That's why unclaimed tokens can be still revoked after a successful vesting . Which is not acceptable for the recipient .

A malicious owner can take this advantage and revoke the unclaimed tokens even after the vesting duration has been ended and there's no locked funds .

Vulnerability Detail

The revokeAll function in the exccrow contract :

   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);
    }

The main issue here is there's no functionality to check if the vesting has ended or not . If the vesting duration has been ended and some unclaimed tokens are left in the contract for late claiming , vesting could be still revoked by the owner and unclaimed tokens will be sent to the owner . Which is unacceptable for the recipient as the vesting is succesfully finished .

Impact

Owner can maliciously revoke the unclaimed tokens even after the vesting duration has been ended .

Code Snippet

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

Tool used

Manual Review

Recommendation

add a check for ensuring that vesting is still going on :

  function revokeAll() external onlyOwner {
        if (!isFullyRevokable) revert NOT_FULLY_REVOKABLE();
        if (isFullyRevoked) revert ALREADY_FULLY_REVOKED();
+       if (block.timestamp > disabledAt ) revert () ; // Add custiom error defining reason .

[nevillehuang]

Invalid, admins are trusted entities trusted to not be malicious. See point 5. of sherlock rules. It is intended that they retain the power to revoke transfer of tokens at any time.

[sherlock-admin2]

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

pratraut commented:

'invalid as its a known issue'