Gas Optimizations

[code423n4]

(G1) Array length is read multiple times

In multiple instances, the same array length is read multiple time from memory/calldata. It would be better to save the value on the stack once and for all. Example:

    function batchFillOrder(
        Order[] memory orders,
        bytes[] calldata signatures,
        uint256[][] memory floorAssetTokenIds
    ) public returns (uint256[] memory positionIds) {
        require(orders.length == signatures.length, "Length mismatch in input");
        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");

        positionIds = new uint256[](orders.length);

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

It can be rewritten like:

    function batchFillOrder(
        Order[] memory orders,
        bytes[] calldata signatures,
        uint256[][] memory floorAssetTokenIds
    ) public returns (uint256[] memory positionIds) {
        uint256 _length = orders.length;
        require(_length == signatures.length, "Length mismatch in input");
        require(_length == floorAssetTokenIds.length, "Length mismatch in input");

        positionIds = new uint256[](_length);

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

Lines

grep -r '\.length'

(G2) Error codes instead of strings

Recent versions of solidity support custom errors. They are more gas efficient than error strings when deploying the contract and when the error is raised. For example:

require(msg.sender == order.maker, "Not your order");

It can be rewritten like:

error NotYourOrder(); // this in the global scope
...
if(msg.sender != order.maker) revert NotYourOrder();

Reference

Lines

grep -r 'require'

(G3) Transferring ERC721 IN doesn't need to be "safe"

function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {
    for (uint256 i = 0; i < assets.length; i++) {
        ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);
    }
}

Here safeTransferFrom performs a onERC721Received call to address(this), which isn't needed (we know it does nothing).

Suggested using the simpler transferFrom.

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

(G4) Opposite order creates unnecessary memory

    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
        // use decode/encode to get a copy instead of reference
        Order memory oppositeOrder = abi.decode(abi.encode(order), (Order));

        // get the opposite side of the order (short/long)
        oppositeOrder.isLong = !order.isLong;
        orderHash = hashOrder(oppositeOrder);
    }

Here oppositeOrder is created by copying order to new memory. It would be better working with the same memory reference. We can make it safe by recasting order.isLong at the end.

    function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
        bool _isLong = order.isLong;   // save on stack

        order.isLong = !_isLong;
        orderHash = hashOrder(order);

        order.isLong = _isLong;
    }

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

(G5) for-loop i++

In general, a for-loop like this can be made more gas efficient.

for (uint256 i = 0; i < length; i++) {
    ...
}

Consider rewriting it like this:

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Lines

grep -r 'i++'