detectiveking - `recoverERC20` method does not correctly handled Proxied Tokens

[sherlock-admin2]

detectiveking

high

recoverERC20 method does not correctly handled Proxied Tokens

Summary

Proxied tokens can have multiple token addresses. recoverERC20 does not properly handle this case.

Vulnerability Detail

Take a look at this link: https://github.com/d-xo/weird-erc20

Here is a screenshot from there:

In this case, our recoverERC20 function does not correctly handle tokens with multiple addresses due to proxies. Here is what recoverERC20 looks like:

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

Because we allow withdrawing any token and token amount arbitrarily if it is not the provided vesting token, the attacker can simply use the other address for the proxied token, besides the one that is specified in the contract, and withdraw all the funds, even locked ones.

Impact

Drainage of funds in the contract, even locked ones.

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

Either disallow withdrawal of any tokens that are not the vesting token or allow admin to specify a blacklist of tokens that can't be withdrawn.

Duplicate of #62

[sherlock-admin2]

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

_rahul commented:

Invalid: Non standard / weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.