QA Report

[code423n4]

Executive Summary

Code is well written, fully documented and coverage is high

The codebase could benefit with a few small refactoring to further simplify and to save extra gas

Use treasury for receiving funds

Most of the times a DAO ends up separating the owner, the Multisig with execution power, from the treasury the multisig to handle fees and funds.

The contract is currently using owner to handle settings as well as receive funds, it may be preferable to separate that by adding treasury

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

Simplify Value Comparison - Saves some bytecode and makes logic leaner

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

        if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {

Can be refactored with

        // transfer strike to owner if put is expired or call is exercised
        if (order.isCall == isExercised) {

As in both case they were both true or both false

Consequence of Smart Comparison Above - Saves 20 to 80 gas (avg is 60)

Because of the gas save above we can refactor the entire block to an if / else

        if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
            // etc...

            return;
        }

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

            return;
        }

Can be refactored to

        if (order.isCall == isExercised)) {
                // transfer assets from putty to owner if put is exercised or call is expired

                // 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);
        } else {
                // transfer assets from putty to owner if put is exercised or call is expired

                _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]);
        }

Lock-in Fee with Order Creation

TL;DR: Fee could change mid order, it's also more gas efficient to store them in the order struct

Fee for orders are calculated at time of withdraw, however the order taker may have accepted the order at a time in which the fee was different, this can create a difference in their PnL.

Because order validation is done on creation and only the hash is stored onChain, it may be desirable to add a order.fee to the order struct, and then use that fee instead of the one from storage.

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

This will also save 2.1k in by avoiding one SLOAD during withdraw

Exercise - order.isLong can fail earlier

Can move require(order.isLong, "Can only exercise long positions"); to the first line to make it fail faster

This would be consistent with how withdraw is written

Slightly incorrect EIP-712 Definition

Quoting the standard for a Struct

Definition of encodeType
The type of a struct is encoded as name ‖ "(" ‖ member₁ ‖ "," ‖ member₂ ‖ "," ‖ … ‖ memberₙ ")" where each member is written as type ‖ " " ‖ name. For example, the above Mail struct is encoded as Mail(address from,address to,string contents).

It follows that the definition provided:

      bytes32 public constant ORDER_TYPE_HASH =
        keccak256(
            abi.encodePacked(
                "Order(",
                "address maker,",
                "bool isCall,", 
                // etc...
                "ERC721Asset[] erc721Assets",
                ")",

Is incorrect (if slightly), as the EIP-712 compliant implementation would have the parenthesis on a separate line. While the closing parenthesis would be with the last entry

ERC-712 Compliant

            abi.encodePacked(
                "Order",
                "(", // etc...
                "ERC721Asset[] erc721Assets)"

I've tested on 0.8.13 and because of encodePacked there will be no difference in the produced type

Malicious Token Risk

Am adding this because from my experience other wardens will submit such findings, and I don't want to lose points, although I don't believe there's any real risk beside the need to inform end-users that any token can be used with the Smart Contract.

Because of the open ended nature, any malicious token (revert on transfer, wrong accounting, deceiving name), could be used, and it's up to the caller (maker / taker) to verify all the addresses.

I think the finding should be at most Non-Critical (Informational)

Avoid Magic_Values

These values would be easier to maintain, understand and parse through if they were constants, ideally in ALL_CAPS

• 30
• 1000

Public Visibility for External Functions

Most of these functions are not called by the contract, yet they have public visibility, there is no additional cost to this, however you'd always want to mark functions meant to be called from outside as external

• acceptCounterOffer
• batchFillOrder
• withdraw
• exercise

[outdoteth]

Lock-in Fee with Order Creation

Duplicate: Admin can change fee at any time for existing orders: https://github.com/code-423n4/2022-06-putty-findings/issues/422

Malicious Token Risk

Duplicate: Setting malicious or invalid erc721Assets, erc20Assets or floorTokens prevents the option from being exercised: https://github.com/code-423n4/2022-06-putty-findings/issues/50

[outdoteth]

high quality report

[outdoteth]

Question for "Slightly incorrect EIP-712 Definition".

Is incorrect (if slightly), as the EIP-712 compliant implementation would have the parenthesis on a separate line. While the closing parenthesis would be with the last entry

What do you mean by this? From the quoted spec it looks like the closing parentheses should be on the same line? (which is how it is currently). So I don't see what the issue is here.