Putty contest - __141345__'s results

The term strike may lead to confusion

Usually the term "strike" means the following:

A strike price is a set price at which a derivative contract can be bought or sold when it is exercised

Refer to strike price.

However, here the "strike" seems to mean the total amount when exercised.

498-501:
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

342-379:
        // filling short put: transfer strike from maker to contract
        if (!order.isLong && !order.isCall) {
            ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
            return positionId;
        }

        // filling long put: transfer strike from taker to contract
        if (order.isLong && !order.isCall) {
            // handle the case where the taker uses native ETH instead of WETH to deposit the strike
            if (weth == order.baseAsset && msg.value > 0) {
                // check enough ETH was sent to cover the strike
                require(msg.value == order.strike, "Incorrect ETH amount sent");

                // convert ETH to WETH
                // we convert the strike ETH to WETH so that the logic in exercise() works
                // - because exercise() assumes an ERC20 interface on the base asset.
                IWETH(weth).deposit{value: msg.value}();
            } else {
                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
            }

            return positionId;
        }

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

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }

Suggestion: Using a different term, like settlement, etc.

Fee is solely on the seller side

Another way might have more acceptance for users if the fee is evenly shared by the seller and buyer.

    function withdraw(Order memory order) public {
        \\ ...
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }
        \\ ...
    }        

Suggestion: Split the fee for both the buyer and seller.

No 0 check for the underlying assets

The transfer is only necessary when the order has the corresponding parts. Some orders may just have 1 underlying, not all of ERC20/ERC721/floortoken.

            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);


            _transferERC20sOut(order.erc20Assets);
            _transferERC721sOut(order.erc721Assets);
            _transferFloorsOut(order.floorTokens, positionFloorAssetTokenIds[uint256(orderHash)]);

Suggestion: Checking if the order underlying is 0 length, and only perform transfer when the underlying exists.

Duplicate underlying assets can be added

80-81:
        ERC20Asset[] erc20Assets;
        ERC721Asset[] erc721Assets;

            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
366-379:
        // filling short call: transfer assets from maker to contract
        if (!order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, order.maker);
            _transferERC721sIn(order.erc721Assets, order.maker);
            return positionId;
        }

        // filling long call: transfer assets from taker to contract
        if (order.isLong && order.isCall) {
            _transferERC20sIn(order.erc20Assets, msg.sender);
            _transferERC721sIn(order.erc721Assets, msg.sender);
            _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
            return positionId;
        }

Suggestion: Checking for the duplication of the underlying assets, and merging the same token.

batchFillOrder() requires multiple signatures, could be simplied to one

The idea behind function batchFillOrder() might be save some efforts for the user. However, it still needs to sign every single transaction. A simplier way could be signing the batch orders. And in the function fillOrder(), the verification process can be added with the batch signed flag.

2022-06-putty gas report

MAKING SOME CONSTANTS AS NON-PUBLIC TO SAVE GAS

Reducing from public to private or internal can save gas when a constant isn't used outside of its contract. The suggestion: changing the visibility from public to internal or private here:

89-101

    bytes32 public constant ERC721ASSET_TYPE_HASH =
        keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

    /**
        @dev ERC20Asset type hash used for EIP-712 encoding.
     */
    bytes32 public constant ERC20ASSET_TYPE_HASH =
        keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

    /**
        @dev ERC721Asset type hash used for EIP-712 encoding.
     */
    bytes32 public constant ORDER_TYPE_HASH =
        keccak256(

PUBLIC FUNCTIONS TO EXTERNAL

Functions only called outside the contract could be declared external. The following functions could be set external/internal/private to save gas and improve code quality. External call cost is less expensive than of public functions.

These could be set to private:

228:
    function setBaseURI(string memory _baseURI) public payable onlyOwner {

240:
    function setFee(uint256 _fee) public payable onlyOwner {

These could be set to external:

389:
    function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {

466:
    function withdraw(Order memory order) public {

546:
    function batchFillOrder(
        Order[] memory orders,
        bytes[] calldata signatures,
        uint256[][] memory floorAssetTokenIds
    ) public returns (uint256[] memory positionIds) {


573: 
    function acceptCounterOffer(
        Order memory order,
        bytes calldata signature,
        Order memory originalOrder
    ) public payable returns (uint256 positionId) {

Beside, CALLDATA could be used instead of MEMORY for these external functions.

FOR-LOOPS GAS-SAVING

There are several for loops can be improved to save gas

556:
        for (uint256 i = 0; i < orders.length; i++) {
            positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]);
        }

594:
        for (uint256 i = 0; i < assets.length; i++) {

611:
        for (uint256 i = 0; i < assets.length; i++) {

627:
        for (uint256 i = 0; i < floorTokens.length; i++) {

637:
        for (uint256 i = 0; i < assets.length; i++) {

647:
        for (uint256 i = 0; i < assets.length; i++) {

658:
        for (uint256 i = 0; i < floorTokens.length; i++) {

670:
        for (uint256 i = 0; i < whitelist.length; i++) {

728:
        for (uint256 i = 0; i < arr.length; i++) {

742:
        for (uint256 i = 0; i < arr.length; i++) {

1. LOOP LENGTH COULD BE CACHED

suggestion: cache the for loop length to memory before the loop.

2. ++I COSTS LESS GAS COMPARED TO I++ OR I += 1

suggestion: using ++i instead of i++ to increment the value of an uint variable.

3. NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

suggestion:

for (uint i = 0; i < lenth; ++i)

can be written as

for (uint i ; i < lenth; ++i)

FOR LOOP SUMMARY

The for loops can be written as follows, take one example:

uint length = assets.length;
for (uint i; i < length;) {
    // ... 
    unchecked { ++i; } 
}

> 0 can be replaced with != 0

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

293:
        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

327:
            if (weth == order.baseAsset && msg.value > 0) {

351:
            if (weth == order.baseAsset && msg.value > 0) {

427:
            if (weth == order.baseAsset && msg.value > 0) {

598-599:
            require(token.code.length > 0, "ERC20: Token is not contract");
            require(tokenAmount > 0, "ERC20: Amount too small");

DUPLICATED REQUIRE() CHECKS COULD BE REFACTORED TO A MODIFIER OR FUNCTION

296:
        order.isCall && order.isLong
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");

405:
        !order.isCall
            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");


353:
                require(msg.value == order.strike, "Incorrect ETH amount sent");

429:
                require(msg.value == order.strike, "Incorrect ETH amount sent");

Delete state variable if won't be used later

'isApprovedForAll[from][msg.sender]' can be deleted, like which is done for 'getApproved[id]'.

PyttyV2Nft.sol
     function transferFrom(
        \\ ...