eeshenggoh - Malicious attacker able to front run revoke functions and transfer tokens before it happens

[sherlock-admin2]

eeshenggoh

high

Malicious attacker able to front run revoke functions and transfer tokens before it happens

Summary

The VestingEscrow serves as a vesting contract designated for a sole beneficiary, enabling users to claim vested tokens. The administrator holds authority to revoke both unvested and unclaimed rewards.

Vulnerability Detail

By monitoring transactions in the mempool, users can observe the execution of VestingEscrow::revokeUnvested() or VestingEscrow::revokeAll(). Frontrunners have the potential to transfer all funds prior to the completion of these functions. The affected functions include VestingEscrow::claim(), VestingEscrow::recoverERC20(), and VestingEscrow::recoverEther().

Impact

Frontrunning poses a risk of the protocol losing both ether and tokens.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

1) Enfore the Delay Mechanism. TL;DR Allow user to request and Admin may approve funds to be transfered. 2) Use private mem pools in place to submit revokeAll() & revokeUnvested() privately

Duplicate of #63

[sherlock-admin2]

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