VestingEscrow::locked tokens is not tracked correctly which results less token to be revoked by owner

Summary

Token locked amount for all vesting position under a factory is not tracked properly, as a result when owner will call revokeAll() he will not get the full locked token amount.

Vulnerability Detail

The locked() and unclaimed() in VestingEscrow.sol contract is responsible to calculate the total revokeable amount for owner. But locked() does not track all vested position, it only track the latest vesting position, and the unclaimed() always returns 0. As a result when the owner call revokeAll() then he only gets the latest vested amount. To see this scenario in test comment out this & this line of TestUtil.sol contract, create a test file in test directory and paste this test case:

// 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);
  address user2 = vm.addr(4);
  address user3 = vm.addr(5);

  function setUp() external {
    setUpProtocol(ProtocolConfig(owner, manager));
  }
function test_lockedAndUnclaimed() external {
    vm.startPrank(user1);
    deployVestingEscrow(VestingEscrowConfig(3 ether, user1, 10 days, uint40(block.timestamp), 5 days, true, ''));
    vm.stopPrank();
    vm.startPrank(user2);
    deployVestingEscrow(VestingEscrowConfig(2 ether, user2, 10 days, uint40(block.timestamp), 5 days, true, ''));
    vm.stopPrank();
    vm.startPrank(user3);
    deployVestingEscrow(VestingEscrowConfig(1 ether, user3, 10 days, uint40(block.timestamp), 5 days, true, ''));
    vm.stopPrank();
    console.log("locked:", deployedVesting.locked());
    console.log("total locked:", deployedVesting.totalLocked());
    console.log("Unclaimed:", deployedVesting.unclaimed());
    vm.startPrank(owner);
    deployedVesting.revokeAll();
    vm.stopPrank();
    console.log("Balance of owner after revoking all:", token.balanceOf(owner));
  }
}

If we run this test we will get this result:

[PASS] test_lockedAndUnclaimed() (gas: 742782)
Logs:
  locked: 1000000000000000000
  total locked: 1000000000000000000
  Unclaimed: 0
  Balance of owner after revoking all: 1000000000000000000

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

You can see the total locked amount is showing: 1000000000000000000 but it should show the total amount vested by user1, user2 and user3 which is 6000000000000000000. Another thing you can see that unclaimed value is showing 0. Most importantly the owner only could revoke: 1000000000000000000. The issue could be in revokeAll(), the function looks like:

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

You can see the revokable is calculated like this: uint256 revokable = locked() + unclaimed();, but locked() returns the number of locked token of a recipient which I think should not consider, In my opinion totalLocked() should be considered for this because the description of the function says - /// @notice The total amount of tokens locked., but this also is not giving right data, you can see that in provided test case.

Impact

Owner is able to revoke very less amount than he should.

Code Snippet

Manual Review, Foundry

Recommendation

Track the locked amount properly.

nevillehuang commented 9 months ago

Invalid, there is only one vested position locked determined during vesting contract deployment. unclaimed() will not always return 0. It will only return zero when owner revoke token flow as seen here

sherlock-admin2 commented 9 months ago

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

pratraut commented:

'invalid as issue demonstrated is deploying escrow contract multiple time and it holding the respective tokens sent to it and user is only checking locked tokens in last deployed contract'

itsabinashb commented 9 months ago

Invalid, there is only one vested position locked determined during vesting contract deployment. unclaimed() will not always return 0. It will only return zero when owner revoke token flow as seen here

But when 3 users stake then all 3 locked position should be revokable in total correct? Like in the test 3 user staked 6 ether in total but the owner only can revoke 1 ether, is it expected? Please see here, it is saying revoke all tokens.

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

itsabinashb - VestingEscrow::locked tokens is not tracked correctly which results less token to be revoked by owner #78