QA Report

[code423n4]

The term strike may lead to confusion

Usually the term "strike" means the following:

A strike price is a set price at which a derivative contract can be bought or sold when it is exercised

Refer to strike price.

However, here the "strike" seems to mean the total amount when exercised.

498-501:
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

342-379:
        // filling short put: transfer strike from maker to contract
        if (!order.isLong && !order.isCall) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
            return positionId;
        }

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

        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }

Suggestion: Using a different term, like settlement, etc.

Fee is solely on the seller side

Another way might have more acceptance for users if the fee is evenly shared by the seller and buyer.

    function withdraw(Order memory order) public {
        \\ ...
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }
        \\ ...
    }        

Suggestion: Split the fee for both the buyer and seller.

No 0 check for the underlying assets

The transfer is only necessary when the order has the corresponding parts. Some orders may just have 1 underlying, not all of ERC20/ERC721/floortoken.

            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);

            _transferERC20sOut(order.erc20Assets);
            _transferERC721sOut(order.erc721Assets);
            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);

Suggestion: Checking if the order underlying is 0 length, and only perform transfer when the underlying exists.

Duplicate underlying assets can be added

80-81:
        ERC20Asset[] erc20Assets;
        ERC721Asset[] erc721Assets;

            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
366-379:
        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }

Suggestion: Checking for the duplication of the underlying assets, and merging the same token.

batchFillOrder() requires multiple signatures, could be simplied to one

The idea behind function batchFillOrder() might be save some efforts for the user. However, it still needs to sign every single transaction. A simplier way could be signing the batch orders. And in the function fillOrder(), the verification process can be added with the batch signed flag.