Issue M-1: If the recipient or the owner gets blacklisted by an asset contract they won't be able to recover the ERC20s locked in a contract

Summary

The recoverERC20() functions in the three contracts can leave tokens frozen in the contract if the recipient or owner gets blacklisted on an asset contract.

Vulnerability Detail

If the receiving address gets blacklisted on the asset contract which they want to recover using the recoverERC20 function in VestingEscrow.sol, VestingEscrowFactory.sol or OZVotingAdaptor.sol it will revert and there is no way to retrieve the funds to another address thus leaving them locked up in the contract.

Impact

Medium since there is no ability to recover to another address but probability of recipient or owner getting blacklisted is not high.

Code Snippet

Transferring the tokens to recipient() in VestingEscrow.sol: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrow.sol#L202-L212

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

Transferring the tokens to owner() in VestingEscrowFactory.sol: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L80-L85

   function recoverERC20(address token_, uint256 amount) external {
        if (amount > 0) {
            IERC20(token_).safeTransfer(owner(), amount);
            emit ERC20Recovered(token_, amount);
        }
    }

Transferring the tokens to owner() in OZVotingAdaptor.sol: https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/adaptors/OZVotingAdaptor.sol#L78-L83

   function recoverERC20(address token_, uint256 amount) external {
        if (amount > 0) {
            IERC20(token_).safeTransfer(owner(), amount);
            emit ERC20Recovered(token_, amount);
        }
    }

Tool used

Manual Review

Recommendation

Consider making the functions accept an address to send the tokens to and adding the onlyRecipient or onlyOwner modifier correspondingly similar to the claim() function in VestingEscrow.sol

-   function recoverERC20(address token_, uint256 amount) external {
+   function recoverERC20(address token_, address beneficiary, uint256 amount) external onlyRecipient {
           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);
+             IERC20(token_).safeTransfer(beneficiary, recoverable);
               emit ERC20Recovered(token_, recoverable);
           }
     }

-   function recoverERC20(address token_, uint256 amount) external {
+   function recoverERC20(address token_, address beneficiary, uint256 amount) external onlyOwner{
        if (amount > 0) {
-            IERC20(token_).safeTransfer(owner(), amount);
+            IERC20(token_).safeTransfer(beneficiary, amount);
            emit ERC20Recovered(token_, amount);
        }
    }

Duplicate of #31

sherlock-audit / 2024-01-rio-vesting-escrow-judging

nslavchev - Issue M-1: If the recipient or the owner gets blacklisted by an asset contract they won't be able to recover the ERC20s locked in a contract #56