YearnCurveVaultOperator's depositETH can leave the remainder ETH funds frozen and unaccounted for, then utilized by another caller

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L75-L100

Vulnerability details

depositETH() effectively do not control the utilization of input token and can freeze WETH input funds in native ETH form on the contract balance when Yearn pool doesn't perform liquidity addition for any reason.

Due to presence of the additional WETH -> ETH step, input token control is performed for WETH -> ETH withdrawal only, while actual input token utilization is in ETH terms.

As a result, any ETH residual is left unaccounted on the contract balance.

For example, given some stressed state of the pool and zero vault tokens slippage depositETH() called with, the call will go through with producing no new vault tokens, while all input WETH will become frozen on the contract balance in the ETH form. It can later be utilized by another caller, i.e. the funds can be permanently lost for the original owner as over time given enough number of such events the reconciliation who used whose leftover funds will become increasingly harder.

Setting severity to be high as principal funds are frozen or even lost as an impact conditional on an external state of Yearn pool.

Proof of Concept

depositETH calls getOutputAmounts with weth balance, controlling only weth -> eth withdrawal this way:

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/operators/Yearn/YearnCurveVaultOperator.sol#L75-L100

        uint256 vaultBalanceBefore = IERC20(vault).balanceOf(address(this));
        uint256 ethBalanceBefore = weth.balanceOf(address(this));

        ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer));

        // withdraw ETH from WETH
        withdrawer.withdraw(amount);

        StakingLPVaultHelpers._addLiquidityAndDepositETH(
            vault,
            ICurvePoolETH(pool),
            IERC20(lpToken),
            poolCoinAmount,
            eth,
            amount
        );

        (amounts, tokens) = OperatorHelpers.getOutputAmounts(
            IERC20(address(weth)),
            ethBalanceBefore,
            amount,
            IERC20(vault),
            vaultBalanceBefore,
            minVaultAmount
        );
    }

getOutputAmounts() aims to control input token utilization, but here it will only check WETH balance before and after WETH -> ETH withdrawal and nothing else instead of input token amount used in liquidity addition:

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/OperatorHelpers.sol#L18-L29

    function getOutputAmounts(
        IERC20 inputToken,
        uint256 inputTokenBalanceBefore,
        uint256 expectedInputAmount,
        IERC20 outputToken,
        uint256 outputTokenBalanceBefore,
        uint256 minAmountOut
    ) internal view returns (uint256[] memory amounts, address[] memory tokens) {
        require(
            inputTokenBalanceBefore - inputToken.balanceOf(address(this)) == expectedInputAmount,
            "OH: INVALID_AMOUNT_WITHDRAWED"
        );

Any unused input WETH tokens amount will reside on the contract balance in ETH native form as withdrawer.withdraw put all incoming WETH amount there:

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/Withdrawer.sol#L24-L29

    /// @notice Withdraw native token from wrapper contract
    /// @param amount The amount to withdraw
    function withdraw(uint256 amount) external nonReentrant {
        weth.transferFrom(msg.sender, address(this), amount);
        weth.withdraw(amount);
        Address.sendValue(payable(msg.sender), amount);

ETH leftover funds are left accounted, it is de facto assumed that all ETH is always spent for liquidity addition. ETH remainder funds are frozen this way.

Recommended Mitigation Steps

Consider expanding balance check mechanics to ETH native balance case and use it in depositETH() to control that full ETH amount was used for liquidity addition in line with how this is now controlled for ERC20 tokens:

        require(inputETHBalanceBefore - address(this).balance == expectedInputAmount,
            "OH: INVALID_AMOUNT_WITHDRAWED"
        );

[obatirou]

YearnCurveVaultOperator's depositETH can leave the remainder ETH funds frozen and unaccounted for, then utilized by another caller (disputed).

We are depositing the ETH in the receive function of NestedFactory.