Function setFee can be called after fillOrder

[code423n4]

Lines of code

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

Vulnerability details

Impact

Function setFee can be called at any time, including after fillOrder has been called. This could be deemed unfair to the user in the Short Position, since they created the order at a particular point in time.

The impact is that they may end up paying more fees than they expected when they call withdraw.

Proof of Concept

• User creates order and fee is currently 5 (i.e. 0.5%).
• Function fillOrder is called on the order.
• Owner calls setFee increasing it to 30 (i.e. 3%)
• User in short position calls withdraw (either when Call and Exercised, or when Put and not Exercised). They end up paying 3% in fees instead of 0.5%.

Recommended Mitigation Steps

Add a new mapping variable that records the fee at the time fillOrder is called.

mapping (uint256 => uint256) public positionFees;

Then make the following edits to fillOrder and withdraw.

Edit for function fillOrder:

        Order memory order,
        bytes calldata signature,
        uint256[] memory floorAssetTokenIds
    ) public payable returns (uint256 positionId) {
...
positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;
// Saves fee at at time fillOrder called
positionFees[order.isLong ? positionId : uint256(orderHash)] = fee;
...

Edit for function withdraw:

function withdraw(Order memory order) public {
...
if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
    // send the fee to the admin/DAO if fee is greater than 0%
    uint256 feeAmount = 0;
    uint256 positionFee = positionFees[uint256(orderHash)];
    if (positionFee > 0) {
        feeAmount = (order.strike * positionFee) / 1000;
        ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
    }

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

[outdoteth]

Duplicate: Admin can change fee at any time for existing orders: #422