QA Report

[code423n4]

(L1) fee can change for ongoing orders

The owner can call setFee(uint256 _fee) and change the fee amount. This changes the fee taken for all orders already filled/exercised.

In some situations, an user may not have filled an order if they knew the fee would end up higher. This situation is alleviated by the fact that the fee is capped at 3%.

Recommendations

The fee can be written in the Order struct and checked that it matches the current correct value during fillOrder. This way when exercising/withdrawing we can use the "order.fee" irrespective to the current global variable.

(L2) fee only for call orders

The protocol collects a fee proportional to the strike.
In the current implementation, this happens only during the withdraw of a exercised call order (or unexercised put) (lines).
I think there should be a fee also for the other order, but it got missing.

Recommendations

Add a similar fee collection during exercise of a put order. Overall, check all possible paths and look if the fees collected make sense.

(L3) Some functions are payable even though they don't accept ether

Functions setBaseURI and setFee are payable but they don't use ether.
If the owner sets msg.value > 0 by mistake, it will get lost.

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

Recommendations

Remove the payable keyword in these functions.

(L4) Positions NFT mints aren't "safe"

During a fill, two ERC721 tokens are minted (lines):

        // create long/short position for maker
        _mint(order.maker, uint256(orderHash));

        // create opposite long/short position for taker
        bytes32 oppositeOrderHash = hashOppositeOrder(order);
        positionId = uint256(oppositeOrderHash);
        _mint(msg.sender, positionId);

If the recipient is a contract, it may misbehave and lose the NFT, since it may expect a onERC721Received call.

Since order.maker can't be a contract (we need its signature), this only applies to msg.sender.

Recommendations

Document that NFTs are minted this way, so contracts interacting with Putty can be aware. _safeMint is not suggested so we can save gas.

(N1) Magic value can be saved as constant

// check duration is valid
require(order.duration < 10_000 days, "Duration too long");

The value 10_000 days can be saved as uint256 private constant MAX_DURATION. This helps the reader keeping track of values.

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

(N2) Order can be cancelled multiple times

The function cancel(Order memory order) can be called multiple times for the same order.
No big problem, the only issue here is that it continues emitting a CancelledOrder, which may lead to unexpected behavior for off-chain software.
Suggested adding:

require(!cancelledOrders[orderHash], "Order already cancelled");

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

[outdoteth]

(L2) fee only for call orders

Duplicate: Fees are only applied on puts if they are expired: #269

(L4) Positions NFT mints aren't "safe"

Duplicate: Contracts that can’t handle ERC721 tokens will lose their Putty ERC721 position tokens: #327

[HickupHH3]

L2 is a dup of #285