[H-02] Owner does not get any fee when call is expired

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L508-L519 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L366-L371 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L435-L437

Vulnerability details

Owner\creators lose profit by not collecting the fees on half the expired cases (all calls).

The only place where owner receives fee is when withdrawing an exercised call or expired put: https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L494-L506

But when a call expired, no fee has been collected on any part of the order life-cycle.

Proof of Concept

Will not write another test here to show this, but consider the following:

            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L435-L437

The full strike amount is being transferred, and no fee is paid along the whole order life-cycle.

Tools Used

forge, editor, code review

Recommended Mitigation Steps

Decide who should pay the fees in this case. Providing solution for both cases:

A. you can either make the short pay the fee, he should pay the feeAmount when his asset returns to him when withdraw on expired.

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L508-L519 Change block mentioned above to:

            // transfer assets from putty to owner if put is exercised or call is expired
            if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) {
                _transferERC20sOut(order.erc20Assets);
                _transferERC721sOut(order.erc721Assets);

                // for call options the floor token ids are saved in the long position in fillOrder(),
                // and for put options the floor tokens ids are saved in the short position in exercise()
                uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash);
                _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[floorPositionId]);

                if (order.isCall && fee != 0) { //@audit logically guaranteed that !isExercised so no need to check && !isExercised
                    uint256 feeAmount;
                    feeAmount = (order.strike * fee) / 1000;
                    ERC20(order.baseAsset).safeTransferFrom(msg.sender, owner(), feeAmount);
                }

                return;
            }

B. On the other hand you can make the long pay the fee (when he already pays the premium),

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L366-L371 Change code block mentioned above to:

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

                //@audit this add this code
                uint256 feeAmount;
                if (fee != 0) {
                    feeAmount = (order.strike * fee) / 1000;
                    ERC20(order.baseAsset).safeTransferFrom(msg.sender, owner(), feeAmount);
                }

                return positionId;
            }

And if call exercised, long must only pay to complete the strike (strike-feePayed)

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L435-L437 Change code block mentioned above to:

} else {
    uint256 feeAmount;
    if (fee != 0) {
        feeAmount = (order.strike * fee) / 1000;
    }
    ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike - feeAmount);
}

and if expired - no worry as fee already payed.

[GalloDaSballo]

The finding seems correct, the owner doesn't get the fee in 2 / 4 cases

[outdoteth]

Fees should only be paid on exercise as is explained in the comments; https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L135

[HickupHH3]

Agree with sponsor; the intended functionality is for the fee to be applied only upon exercising of the option as stated in the comment. It is correct for no fee to be collected for an expired call.