Excess ETH payments may be locked in `RubiconRouter`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L381-L409 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L453-L472 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L494-L517

Vulnerability details

The RubiconRouter offerWithETH, depositWithETH and swapWithETH functions all accept ETH transfers greater than or equal to the expected payment amount. Since the excess amount is not deposited or refunded, it will be locked in the router contract if a caller sends excess ETH.

RubiconRouter#offerWithETH

    // Pay in native ETH
    function offerWithETH(
        uint256 pay_amt, //maker (ask) sell how much
        // ERC20 nativeETH, //maker (ask) sell which token
        uint256 buy_amt, //maker (ask) buy how much
        ERC20 buy_gem, //maker (ask) buy which token
        uint256 pos //position to insert offer, 0 should be used if unknown
    ) external payable returns (uint256) {
        require(
            msg.value >= pay_amt,
            "didnt send enough native ETH for WETH offer"
        );
        uint256 _before = ERC20(buy_gem).balanceOf(address(this));
        WETH9(wethAddress).deposit{value: pay_amt}();
        uint256 id = RubiconMarket(RubiconMarketAddress).offer(
            pay_amt,
            ERC20(wethAddress),
            buy_amt,
            buy_gem,
            pos
        );
        uint256 _after = ERC20(buy_gem).balanceOf(address(this));
        if (_after > _before) {
            //return any potential fill amount on the offer
            ERC20(buy_gem).transfer(msg.sender, _after - _before);
        }
        return id;
    }

RubiconRouter#depositWithETH

    // Deposit native ETH -> WETH pool
    function depositWithETH(uint256 amount, address targetPool)
        external
        payable
        returns (uint256 newShares)
    {
        IERC20 target = IBathToken(targetPool).underlyingToken();
        require(target == ERC20(wethAddress), "target pool not weth pool");
        require(msg.value >= amount, "didnt send enough eth");

        if (target.allowance(address(this), targetPool) == 0) {
            target.approve(targetPool, amount);
        }

        WETH9(wethAddress).deposit{value: amount}();
        newShares = IBathToken(targetPool).deposit(amount);
        //Send back bathTokens to sender
        ERC20(targetPool).transfer(msg.sender, newShares);
    }

RubiconRouter#swapWithETH

    function swapWithETH(
        uint256 pay_amt,
        uint256 buy_amt_min,
        address[] calldata route, // First address is what is being payed, Last address is what is being bought
        uint256 expectedMarketFeeBPS
    ) external payable returns (uint256) {
        require(route[0] == wethAddress, "Initial value in path not WETH");
        uint256 amtWithFee = pay_amt.add(
            pay_amt.mul(expectedMarketFeeBPS).div(10000)
        );
        require(
            msg.value >= amtWithFee,
            "must send enough native ETH to pay as weth and account for fee"
        );
        WETH9(wethAddress).deposit{value: amtWithFee}();
        return
            _swap(
                pay_amt,
                buy_amt_min,
                route,
                expectedMarketFeeBPS,
                msg.sender
            );
    }

Recommendation: Since expected payment amounts can be calculated exactly by the caller, require exact payment amounts for these functions.

[KenzoAgada]

Duplicate of #15.

[bghughes]

Duplicate of #15