itsabinashb - VestingEscrow::user can claim token after vesting for minimum time

[sherlock-admin2]

itsabinashb

medium

VestingEscrow::user can claim token after vesting for minimum time

Summary

The way claim() of VestingEscrow.sol contract designed it allows an user to vest for minimum time and claim the token after that.

Vulnerability Detail

The claim() does not have any check for minimum withdrawing time limit. For this behaviour an user can can open a vesting position for very minimum amount of time and claim the token. To see this in test comment out this and this line, create a test file in test folder and paste this test case in it:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.23;

import {TestUtil} from 'test/lib/TestUtil.sol';
import {ERC20Token} from 'test/lib/ERC20Token.sol';
import 'forge-std/console.sol';

contract Audit is TestUtil {
  address owner = vm.addr(1);
  address manager = vm.addr(2);
  address user1 = vm.addr(3);

  function setUp() external {
    setUpProtocol(ProtocolConfig(owner, manager));
  }

function test_minVestTime() external {
    vm.startPrank(user1);
    deployVestingEscrow(VestingEscrowConfig(10 ether, user1, 1 minutes, uint40(block.timestamp), 30, true, ''));
    vm.warp(1 minutes + block.timestamp);
    deployedVesting.claim(user1, 10 ether);
    assertEq(token.balanceOf(user1), 10 ether);
  }
}

In this test the user1 vested for 1 minute. If we run the test we can see it will successfully pass:

Running 1 test for test/Audit2.t.sol:Audit
[PASS] test_minVestTime() (gas: 385390)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.64ms

The user even can open multiple vesting for such minimum time and get all vested amount in total. Replace this test function with previous test function:

function test_minVestTime() external {
    vm.startPrank(user1);
    deployVestingEscrow(VestingEscrowConfig(10 ether, user1, 1 minutes, uint40(block.timestamp), 30, true, ''));
    vm.warp(1 minutes + block.timestamp);
    deployedVesting.claim(user1, 10 ether);
    deployVestingEscrow(VestingEscrowConfig(10 ether, user1, 1 minutes, uint40(block.timestamp), 30, true, ''));
    vm.warp(1 minutes + block.timestamp);
    deployedVesting.claim(user1, 10 ether);
    deployVestingEscrow(VestingEscrowConfig(10 ether, user1, 1 minutes, uint40(block.timestamp), 30, true, ''));
    vm.warp(1 minutes + block.timestamp);
    deployedVesting.claim(user1, 10 ether);
    console.log("user1 balance:", token.balanceOf(user1));
    assertEq(token.balanceOf(user1), 30 ether);
  }

Result:

[PASS] test_minVestTime() (gas: 716580)
Logs:
  user1 balance: 30000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.89ms

Impact

An user can open a vesting position for very minimum amount of time and can claim tokens.

Code Snippet

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

   Tool used

Manual Review, Foundry

Recommendation

Put a minimum time limit in claim() so that nobody can claim before a certain time.

[nevillehuang]

Similar to #33, at any given point of time, only the recipient set by the admin of the vesting contract can invoke claim() at any time as indicated by the onlyRecipient modifier. Since user1 here is the recipient set by the deployer, it is intended that he can claim any unclaimed tokens based on vesting schedule set by admin. He cannot decide how long to veest

[sherlock-admin2]

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

pratraut commented:

'invalid as warden demonstrated owner only deploying escrow and claiming tokens after vesting period over'