Krace - `VestingEscrow` cannot work properly with multiple addresses token

[sherlock-admin2]

Krace

medium

VestingEscrow cannot work properly with multiple addresses token

Summary

Should the token() within the VestingEscrow involve multiple addresses, it becomes possible for anyone to transfer all remaining tokens, including the locked() and unclaimed() ones, to the designated recipient().

Vulnerability Detail

The recoverERC20 function exclusively transfers the surplus token() within the contract, ensuring sufficient token() remains for both locked() and unclaimed() tokens.

However, there exists a token with multiple addresses, enabling the transfer of the same token with different addresses. An attacker could exploit this by utilizing a distinct address to circumvent the checks and transfer all tokens to the recipient().

    function recoverERC20(address token_, uint256 amount) external {
        uint256 recoverable = amount;
//@audit if token() is multiple addresses, anyone could transfer all the token() to the recipient()
        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);
        }
    }

Impact

The locked() and unclaimed() token in the VestingEscrow could be transferred to the recipient() incorrectly, causing the entire contract to not work properly.

Code Snippet

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

Tool used

Manual Review

Recommendation

It's recommended to add a check in function recoverERC20 to make sure that the token().bakanceOf(this) after transferring is equal to or greater than the locked() + unclaimed().

diff --git a/rio-vesting-escrow/src/VestingEscrow.sol b/rio-vesting-escrow/src/VestingEscrow.sol
index 1fdd103..09cfdb2 100644
--- a/rio-vesting-escrow/src/VestingEscrow.sol
+++ b/rio-vesting-escrow/src/VestingEscrow.sol
@@ -207,6 +207,7 @@ contract VestingEscrow is IVestingEscrow, Clone {
         }
         if (recoverable > 0) {
             IERC20(token_).safeTransfer(recipient(), recoverable);
+            if(token().balanceOf(address(this)) < locked() + unclaimed()) revert TOO_MANY_TOKEN_RECOVERED();
             emit ERC20Recovered(token_, recoverable);
         }
     }

Duplicate of #62

[solimander]

Invalid. The vesting escrow is not designed to work with tokens that have multiple addresses.

[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.