Al-Qa-qa - Users can Front-run `VestingEscrow::revokeAll()` preventing owner form taking the `unclaimed` tokens

[sherlock-admin2]

Al-Qa-qa

medium

Users can Front-run VestingEscrow::revokeAll() preventing owner form taking the unclaimed tokens

Summary

Users/receipent can rescue his unclaimed tokens from being revoked, if the owner needs to revoke all tokens (locked and the unclaimed).

Vulnerability Detail

In VestingEscrow, there are two methods used to revoke tokens (preventing receipent from taking them), The organisation may want to do such a thing.

In VestingEscrow::revokeAll(), The Owner of the Factory contract, can revoke all locked tokens and the unclaimed tokens.

VestingEscrow.sol#L177-L189

    function revokeAll() external onlyOwner {
        ...
        uint256 revokable = locked() + unclaimed();
        ...
        token().safeTransfer(_owner(), revocable);
        ...
    }

If the user/receipent has some tokens that he did not claim, the owner should be able to take the locked (unvested) tokens and the unclaimed tokens too.

The user/receipent can prevent his unclaimed tokens getting transferred to owner by firing VestingEscrow::claim() before owner firing VestingEscrow::revokeAll().

VestingEscrow.sol#L136-L144

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

        token().safeTransfer(beneficiary, claimable);
        ...
    }

This will make the unclaimed tokens when revoking all equals zero, where there will be no unclaimed tokens or at least a little amount when the owner VestingEscrow::revokeAll() get fired.

We are aware that the user can claim his tokens anytime, but the user/receipent may want to wait to claim all his tokens in one time, to save gas for example. And in this period he can easily take his unclaimed tokens. But the Organisation (Factory owner) may want to revoke his unclaimed tokens at any time, this may be because of the user behaviour or any other things.

At this condition he can noticed the Organisation (Factory owner) revokeAll() transaction, and rescue his unclaimed tokens.

Impact

Preventing the Organisation from revoking the user/receipent unclaimed tokens

Code Snippet

VestingEscrow.sol#L177-L189 VestingEscrow.sol#L136-L144

Tool used

Manual Review

Recommendation

I think this issue does not have much impact, as unclaimed tokens can be claimed by the user any time, and revokeAll() function may be used a little.

One of the solutions for such a thing is implementing something like Pull over Push pattern.

Instead of letting the user/receipent claim his unclaimed tokens instantaneously, we can let him make a request first for claiming, and after he made the request, he will need to withdraw his unvested tokens after an amount of time (1 hour for example).

By doing this, users will not be able to rescue there unvested tokens if the organisation wanted to revoke all users tokens.

Duplicate of #63

[sherlock-admin2]

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

_rahul commented:

Invalid: Recipient has authorized escrow to be fully revokable to help recover funds (incase of loss of recipient address etc) during setup. Essentially, owner calls revokeAll() to rescue funds for the recipient. In this context, it’s unlikely that will recipient front-run revokeAll().

pratraut commented:

'invalid due to owner/recipient being TRUSTED entity'