Putty contest - Chom's results

You must also remove _balanceOf from _burn otherwise burning may be underflow

https://github.com/Rari-Capital/solmate/blob/3c738133a0c1697096d63d28ef7a8ef298f9af6b/src/tokens/ERC721.sol#L172-L187

    function _burn(uint256 id) internal virtual {
        address owner = _ownerOf[id];

        require(owner != address(0), "NOT_MINTED");

        // Ownership check above ensures no underflow.
        unchecked {
            _balanceOf[owner]--;
        }

        delete _ownerOf[id];

        delete getApproved[id];

        emit Transfer(owner, address(0), id);
    }

Should be overridden in PuttyV2Nft.sol

    function _burn(uint256 id) internal override virtual {
        address owner = _ownerOf[id];

        require(owner != address(0), "NOT_MINTED");

        delete _ownerOf[id];

        delete getApproved[id];

        emit Transfer(owner, address(0), id);
    }

Using unlimited balance may cause metamask and etherscan to display balance in a wrong way

https://rinkeby.etherscan.io/token/0xc67dbd1f722edc4b7f409f287ed6f7d928aa730c?a=0xe979054eb69f543298406447d8ab6cbbc5791307

    // set balanceOf to max for all users
    function balanceOf(address owner) public pure override returns (uint256) {
        require(owner != address(0), "ZERO_ADDRESS");
        return type(uint256).max;
    }

Should use ownerOf(...) instead of _ownerOf for tokenURI

Currently

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

    function tokenURI(uint256 id) public view override returns (string memory) {
        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");

        return string.concat(baseURI, Strings.toString(id));
    }

_ownerOf is internal and not allowed to use in openzeppelin standard (is a private instead of internal). So, we should change to ownerOf(id) for best practice.

    function tokenURI(uint256 id) public view override returns (string memory) {
        require(ownerOf(id) != address(0), "URI query for NOT_MINTED token");

        return string.concat(baseURI, Strings.toString(id));
    }

Caching the length in for loops and increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Caching the length in for loops:

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

for loop postcondition can be made unchecked Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

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

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

Can be optimized to

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

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example,

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

Can be replaced with

// declare error before contract declaration
error IncorrectETHAmount();

if (msg.value != order.strike) revert IncorrectETHAmount();