Native ETH can be sent with ERC20 orders

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L322-L340 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L348-L364 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L425-L437

Vulnerability details

The fillOrder and exercise functions are payable and will accept and wrap native ETH as WETH if the order's baseAsset is WETH and the caller provides native ETH:

PuttyV2#fillOrder

        // transfer premium to whoever is short from whomever is long
        if (order.isLong) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
        } else {
            // handle the case where the user uses native ETH instead of WETH to pay the premium
            if (weth == order.baseAsset && msg.value > 0) {
                // check enough ETH was sent to cover the premium
                require(msg.value == order.premium, "Incorrect ETH amount sent");

                // convert ETH to WETH and send premium to maker
                // converting to WETH instead of forwarding native ETH to the maker has two benefits;
                // 1) active market makers will mostly be using WETH not native ETH
                // 2) attack surface for re-entrancy is reduced
                IWETH(weth).deposit{value: msg.value}();
                IWETH(weth).transfer(order.maker, msg.value);
            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
            }
        }

PuttyV2#fillOrder

        // filling long put: transfer strike from taker to contract
        if (order.isLong && !order.isCall) {
            // handle the case where the taker uses native ETH instead of WETH to deposit the strike
            if (weth == order.baseAsset && msg.value > 0) {
                // check enough ETH was sent to cover the strike
                require(msg.value == order.strike, "Incorrect ETH amount sent");

                // convert ETH to WETH
                // we convert the strike ETH to WETH so that the logic in exercise() works
                // - because exercise() assumes an ERC20 interface on the base asset.
                IWETH(weth).deposit{value: msg.value}();
            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

            return positionId;
        }

PuttyV2#exercise

            // transfer strike from exerciser to putty
            // handle the case where the taker uses native ETH instead of WETH to pay the strike
            if (weth == order.baseAsset && msg.value > 0) {
                // check enough ETH was sent to cover the strike
                require(msg.value == order.strike, "Incorrect ETH amount sent");

                // convert ETH to WETH
                // we convert the strike ETH to WETH so that the logic in withdraw() works
                // - because withdraw() assumes an ERC20 interface on the base asset.
                IWETH(weth).deposit{value: msg.value}();
            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

However, users may intentionally or accidentally send native ETH to these functions even when the baseAsset is not WETH. In these cases, both the ERC20 baseAsset and native ETH will be transferred to the contract. Any native ETH sent will be locked in the contract. This scenario requires user error, but it's possible to prevent altogether.

Impact: Any native ETH accidentally provided with an ERC20 order will be locked in the contract.

Recommendation: Add a check to ensure that the order's baseAsset must be WETH if the caller sends native ETH to payable functions. This will prevent callers from sending (and losing) native ETH under any other conditions.

        // check that baseAsset is WETH if caller provided native ETH
        if(msg.value > 0) require(order.baseAsset == weth, "baseAsset is not WETH");

Test case:

    function testNativeETHCanBeSentWithERC20Orders() public {
        // arrange
        PuttyV2.Order memory order = defaultOrder();
        order.baseAsset = address(link);
        order.isLong = false;
        bytes memory signature = signOrder(babePrivateKey, order);

        deal(address(this), order.premium);
        deal(address(link), address(this), order.premium);
        link.approve(address(p), order.premium);
        uint256 contractETHBalanceBefore = address(p).balance;
        uint256 takerETHBalanceBefore = address(this).balance;
        uint256 takerLinkBalanceBefore = link.balanceOf(address(this));
        uint256 makerLinkBalanceBefore = link.balanceOf(order.maker);

        // act
        p.fillOrder{value: order.premium}(order, signature, floorAssetTokenIds);

        // assert
        // ETH deducted from caller balance
        assertEq(
            takerETHBalanceBefore - address(this).balance,
            order.premium,
            "Should have transferred ETH from taker to contract"
        );

        // ETH balance stuck in contract
        assertEq(
            address(p).balance,
            order.premium,
            "Should have transferred ETH from taker to contract"
        );

        // LINK transferred from taker
        assertEq(
            takerLinkBalanceBefore - link.balanceOf(address(this)),
            order.premium,
            "Should have transferred LINK from taker"
        );

        // LINK transferred to maker
        assertEq(
            link.balanceOf(order.maker) - makerLinkBalanceBefore,
            order.premium,
            "Should have transferred LINK to maker"
        );
    }

[rotcivegaf]

Duplicate of #226

[outdoteth]

Duplicate: Native ETH can be lost if it’s not utilised in exercise and fillOrder: https://github.com/code-423n4/2022-06-putty-findings/issues/226