Bbash - `recoverEther` function fails if the recipient is a smart contract with no fallback function in `VestingEscrow.sol`

[sherlock-admin]

Bbash

medium

recoverEther function fails if the recipient is a smart contract with no fallback function in VestingEscrow.sol

Summary

In the VestingEscrow.sol contract, the recoverEther function will fail if the recipient is a smart contract with no fallback function and there will be no way to recover Ether sent to this contract.

Vulnerability Detail

If the recipient address is a smart contract (instead of an externally owned account), the recoverEther function will work. However, the recipient contract should have a fallback function to handle the incoming Ether. If the recipient is a contract and does not have a payable fallback function, the sendValue call will fail. Hence, there will be no way to recover Eth sent to the VestingEscrow.sol contract if the recipient address is a smart contract with no fallback function. Any ETH that is accidentally sent to this contract will be locked. In summary, the recoverEther function should generally work with a smart contract address as recipient, but it's important to be aware of the recipient contract's behavior, especially regarding the fallback function.

Impact

Code Snippet

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

    /// @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

Adding onlyRecipient modifier and allowing the recoverEther function to accept a beneficiary address as input can be a flexible and secure approach. This way, the authorized entity can specify the destination address for the recovered Ether.

[nevillehuang]

Invalid, user input error not valid based on sherlock rules when admin is deploying contract and setting incorrect recipient address.

4. User input validation: User input validation to prevent user mistakes is not considered a valid issue.

[sherlock-admin2]

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

pratraut commented:

'valid as with no fallback or receive in recipient contract result in reverted tx'