detectiveking - Reentrancy in `recoverERC20` allows drainage of all funds, even locked ones

[sherlock-admin]

detectiveking

high

Reentrancy in recoverERC20 allows drainage of all funds, even locked ones

Summary

There is a reentrancy vulnerability in recoverERC20 for tokens that implement a _beforeTokenTransfer method, and could potentially give control back to the attacker.

Vulnerability Detail

recoverERC20 looks something like this:

    function recoverERC20(address token_, uint256 amount) external {
        uint256 recoverable = amount;
        if (token_ == address(token())) {
            uint256 available = token().balanceOf(address(this)) -
                (locked() + unclaimed());
            recoverable = Math.min(recoverable, available);
        }
        if (recoverable > 0) {
            IERC20(token_).safeTransfer(recipient(), recoverable);
            emit ERC20Recovered(token_, recoverable);
        }
    }

Notice that we first calculate the available amount and then send it to the recipient. If the token implements a _beforeTokenTransfer method that somehow gives control over to the attacker, then the attacker can re-enter recoverERC20 and again claim the same amount over and over again, draining the contract despite potentially not having vested the funds.

Impact

Drainage of funds in the contract

Code Snippet

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

Tool used

Manual Review

Recommendation

Make function non reentrant

Duplicate of #81

[sherlock-admin2]

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

pratraut commented:

'invalid due to tokens that are not intended for the contract and if user sent it to it then its their fault'

[sherlock-admin2]

Escalate

This is a valid issue. The README says any type of ERC20 token is allowed.

You've deleted an escalation for this issue.