`fee` proportional to strike even for unexercised orders

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L495-L506

Vulnerability details

Impact

The protocol takes a fee proportional to the order's strike. This happens during a withdraw:

// transfer strike to owner if put is expired or call is exercised
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    if (fee > 0) {
        feeAmount = (order.strike * fee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

    ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

    return;
}

It doesn't make sense to take a fee proportional to the strike when the order is unexercised. This fee will make some orders unprofitable for both sides.

Proof of Concept

Alice makes a put order with premium 100\$ and strike 10000\$. The current fee is 2%. When Bob fills the order this happens: (i) Alice pays Bob 100\$, (ii) Bob deposits 10000\$. If the option doesn't get exercised, Bob will call withdraw getting 10000*(100-2)=9800\$ back. So at the end, both Alice and Bob end up losing 100\$ each, and the owner gets 200\$.

Clearly, in this situation it would be expected for Bob to win something.

Recommended Mitigation Steps

Since an option can end up unexercised and it doesn't make sense to collect a fee on the strike, the solution is taking a fee on the premium during fillOrder. The fee on the strike should be collected only if the order was exercised.

[GalloDaSballo]

While no loss of funds nor odd behaviour was shown, the finding may be valid in that a non-exercised call is still paying the fee, which may make it less palatable

[outdoteth]

Yes, agree that this is invalid business logic

[outdoteth]

Duplicate: Fees are only applied on puts if they are expired: https://github.com/code-423n4/2022-06-putty-findings/issues/269