Access control vulnerability

Summary

In the contract VestingEscrowFactory and OZVotingAdaptor 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 owner, there are several considerations and potential risks associated with not having proper access control like

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.
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.
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 opens avenues for 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/VestingEscrowFactory.sol#L80C1-L94C6

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

    function recoverEther() external {
        uint256 amount = address(this).balance;
        if (amount > 0) {
            payable(owner()).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 using the OpenZeppelin Ownable contract:

modifier onlyOwner() {
    require(msg.sender == owner(), "Not the contract owner");
    _;
}

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

function recoverEther() external onlyOwner {
    uint256 amount = address(this).balance;
    if (amount > 0) {
        payable(owner()).sendValue(amount);
        emit ETHRecovered(amount);
    }
}

Duplicate of #8

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

0xBhumii - Access control vulnerability #73