AlexCzm - Recoverable funds functions send the value to `recipient`

[sherlock-admin]

AlexCzm

medium

Recoverable funds functions send the value to recipient

Summary

VestingEscrow.recoverERC20 and VestingEscrow.recoverEther send the funds to recipient address instead of factory owner which is trusted.

Vulnerability Detail

recoverEther and recoverERC20 allow anyone to recover any stucked funds and their transfer to recipient address. But recipient is not trusted and he can refuse forward the funds to their rightful owner.

Impact

Any funds sent by mistake to escrow contracts, can be lost.

Code Snippet

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

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

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

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

Tool used

Manual Review

Recommendation

Recover funds to factory owner or factory manager. These are considered trusted entities and should forward recovered funds to their rightful owner.

Duplicate of #81

[solimander]

I'd consider updating to owner or manager, but consider this low-risk.

[sherlock-admin2]

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

anticl0ck commented:

Invalid

crc32 commented:

invalid, this is intented design