Shaheen - `revokeAll` is useless without delay mechanism on `claim()`, as the recipient can frontrun revokeAll TRX to withdraw unclaimed tokens

[sherlock-admin2]

Shaheen

medium

revokeAll is useless without delay mechanism on claim(), as the recipient can frontrun revokeAll TRX to withdraw unclaimed tokens

Summary

revokeAll is useless without delay mechanism on claim(), as the recipients can frontrun revokeAll TRXs to withdraw unclaimed tokens.

Vulnerability Detail

revokeAll() function is in place so the org deploying the vesting escrow can clawback all the tokens, when something written into a legal contract surpasses. If the user breaks any legal conditions or do something malicious with the voting power, the owner of the contract will call revokeAll() which will take locked + unclaimed tokens out of the contract.

The problem is, user can easily frontrun the revokeAll() TRX to withdraw unclaimed tokens and break the legal contract.

Proof-of-Concept

1. Alex, a recipient, have 50k unclaimed tokens & 25k locked tokens
2. Owner notices some legal contract surpass or malicious activity from Alex
3. Owner calls revokeAll() to clawback Alex all 75k tokens
4. Alex who was listening to the mempool, sees Owner revokeAll Trx
5. Alex calls claim() to withdraw all the unclaimed tokens with high Trx Gas fee than revokeAll Trx
6. Due to higher gas fee Alex's TRX gets executed first, succesfully frontrunning owner's revokeAll Trx
7. Alice gets 50k tokens and owner only gets 25k tokens instead of 75k tokens expected

Code Proof-of-Concept

    function testRevokeAllFrontunClaimAll_PoC() public {
        vm.warp(endTime + 5);

        vm.prank(recipient);
        deployedVesting.claim(recipient, type(uint256).max);

        vm.startPrank(factory.owner());
        vm.expectRevert();
        deployedVesting.revokeAll();
        vm.stopPrank();

        assertEq(deployedVesting.unclaimed(), 0);
        assertEq(token.balanceOf(recipient), amount);
    }

Impact

• Revoking mechanism can be bypassed by any user.

Code Snippet

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

Tool used

🦅

Recommendation

There should be a withdrawal delay mechanism on claim() which don't give chance to the users to frontrun revokeAll. Or Aware the org's owner about this issue and encourage them to use private mempool for revoking. The former is more recomnded as private pools are not really "private". A very good implementation of delay mechanism can be found here Thanks!

Duplicate of #63

[sherlock-admin2]

3 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().

Shaheen commented:

Valid Medium, good Catch

pratraut commented:

'valid as malicious recipient can steal unclaimed tokens'