0xBhumii - Access control vulnerability

[sherlock-admin]

0xBhumii

medium

Access control vulnerability

Summary

In the contract VestingEscrow the recoverERC20 and recoverEther functions does not have any access control specified.

Vulnerability Detail

Having no access control in the recoverERC20 and recoverEther functions means that any address can call these functions, potentially leading to undesired consequences. While the recovered assets go to the contract recipient, there are several considerations and potential risks associated with not having proper access control like

1. Unintended Recovery: Without access control, any address can trigger the recovery functions. This might lead to unintended or unexpected recovery of assets, especially in situations where it was not the contract owner's intention to initiate the recovery.
2. Denial-of-Service (DoS) Attacks: Malicious actors could repeatedly call the recovery functions, causing unnecessary gas consumption and potentially blocking other legitimate transactions. This is a form of a DoS attack, where the contract becomes less responsive due to excessive calls to the recovery functions.
3. Gas Limit Exceedance: If an attacker tries to drain the contract's balance by repeatedly triggering the recovery functions, it could lead to exceeding the block gas limit, causing the transactions to be reverted. This could result in wasted gas fees for the attacker and other users.

Impact

This lack of access control allows malicious actors to exploit the contract, initiating repeated recovery function calls that could result in a Denial-of-Service (DoS) scenario, hindering normal contract operation. Additionally, such repeated calls may lead to gas limit exceedance, causing transaction failures and wasted gas fees. This can also cause unintended recoveries.

Code Snippet

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

       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 Recover any ETH to the recipient.
    function recoverEther() external {
        uint256 amount = address(this).balance;
        if (amount > 0) {
            payable(recipient()).sendValue(amount);
            emit ETHRecovered(amount);
        }
    }

Tool used

Manual Review

Recommendation

By adding the onlyOwner modifier, you restrict the access to these functions to the contract owner, reducing the risk of unauthorized calls and potential issues. Here's an example of how access control can be added to the recoverERC20 and recoverEther functions

 modifier onlyOwner() {
        _checkOwner();
        _;
    }

    function recoverERC20(address token_, uint256 amount) external onlyOwner {
        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 Recover any ETH to the recipient.
    function recoverEther() external onlyOwner {
        uint256 amount = address(this).balance;
        if (amount > 0) {
            payable(recipient()).sendValue(amount);
            emit ETHRecovered(amount);
        }
    }

Duplicate of #81