Putty contest - MiloTruck's results

Non-Critical Issues

keccak256() can be called directly on single strings without abi.encodePacked()

To improve code clarity, consider changing the following lines:

contracts/src/PuttyV2.sol:
  89:        bytes32 public constant ERC721ASSET_TYPE_HASH =
  90:            keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

  95:        bytes32 public constant ERC20ASSET_TYPE_HASH =
  96:            keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

to remove the unnecessary abi.encodePacked() call:

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

bytes32 public constant ERC20ASSET_TYPE_HASH =
        keccak256("ERC20Asset(address token,uint256 tokenAmount)");

Multiple positionId mappings can be combined into a single mapping of a positionId to a struct

The following three mappings use positionId as a key:

contracts/src/PuttyV2.sol:
 149:        mapping(uint256 => uint256) public positionExpirations;
 155:        mapping(uint256 => bool) public exercisedPositions;
 163:        mapping(uint256 => uint256[]) public positionFloorAssetTokenIds;

If appropriate, consider using a single mapping, where the key is positionId, and the value is a struct as such:

struct positionInfo {
    uint256 expiration;
    bool exercised;
    uint256[] floorAssetTokenIds;
}

mapping(uint256 => positionInfo) public positionInfos;

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/src/PuttyV2.sol:
 290:        require(block.timestamp < order.expiration, "Order has expired");
 316:        positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration;
 401:        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
 481:        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/src/PuttyV2.sol:
 185:        event FilledOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);
 193:        event ExercisedOrder(bytes32 indexed orderHash, uint256[] floorAssetTokenIds, Order order);

Gas Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

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

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/src/PuttyV2.sol:
 556:        for (uint256 i = 0; i < orders.length; 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++) {

For-Loops: Cache array length outside of loops

Reading an array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

For example:

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

can be changed to:

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

Consider making the following change to these lines:

contracts/src/PuttyV2.sol:
 556:        for (uint256 i = 0; i < orders.length; 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++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

for (uint256 i; i < numIterations; ++i) {  
    // ...  
}  

can be changed to:

for (uint256 i; i < numIterations;) {  
    // ...  
    unchecked { ++i; }  
}  

Consider making the following change to these lines:

contracts/src/PuttyV2.sol:
 556:        for (uint256 i = 0; i < orders.length; 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++) {

Arithmetics: ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2, thus it costs more gas.

The same logic applies for --i and i--.

Consider using ++i instead of i++ or i += 1 in the following instances:

contracts/src/PuttyV2.sol:
 556:        for (uint256 i = 0; i < orders.length; 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++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/src/PuttyV2.sol:
 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) {
 498:        if (fee > 0) {
 598:        require(token.code.length > 0, "ERC20: Token is not contract");
 599:        require(tokenAmount > 0, "ERC20: Amount too small");

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/src/PuttyV2.sol:
  89:        bytes32 public constant ERC721ASSET_TYPE_HASH =
  90:            keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

  95:        bytes32 public constant ERC20ASSET_TYPE_HASH =
  96:            keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

 101:        bytes32 public constant ORDER_TYPE_HASH =
 102:            keccak256(
 103:                abi.encodePacked(
 104:                    "Order(",
 105:                    "address maker,",
 106:                    "bool isCall,",
 107:                    "bool isLong,",
 108:                    "address baseAsset,",
 109:                    "uint256 strike,",
 110:                    "uint256 premium,",
 111:                    "uint256 duration,",
 112:                    "uint256 expiration,",
 113:                    "uint256 nonce,",
 114:                    "address[] whitelist,",
 115:                    "address[] floorTokens,",
 116:                    "ERC20Asset[] erc20Assets,",
 117:                    "ERC721Asset[] erc721Assets",
 118:                    ")",
 119:                    "ERC20Asset(address token,uint256 tokenAmount)",
 120:                    "ERC721Asset(address token,uint256 tokenId)"
 121:                )
 122:            );

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/src/PuttyV2.sol:
 389:        function exercise(Order memory order, uint256[] calldata floorAssetTokenIds) public payable {

 466:        function withdraw(Order memory order) public {

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

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

 669:        function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) {

 753:        function domainSeparatorV4() public view returns (bytes32) {

Errors: Use custom errors instead of revert strings

Since Solidity v0.8.4, custom errors should be used instead of revert strings due to:

• Cheaper deployment cost
• Lower runtime cost upon revert

Taken from Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors can be defined using of the error statement, both inside or outside of contracts.

Instances where custom errors can be used instead:

contracts/src/PuttyV2.sol:
 214:        require(_weth != address(0), "Unset weth address");
 241:        require(_fee < 30, "fee must be less than 3%");
 278:        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");
 281:        require(!cancelledOrders[orderHash], "Order has been cancelled");
 284:        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
 287:        require(order.duration < 10_000 days, "Duration too long");
 290:        require(block.timestamp < order.expiration, "Order has expired");
 293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
 297:        ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
 298:        : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");
 329:        require(msg.value == order.premium, "Incorrect ETH amount sent");
 353:        require(msg.value == order.strike, "Incorrect ETH amount sent");
 395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
 398:        require(order.isLong, "Can only exercise long positions");
 401:        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
 405:        ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
 406:        : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
 429:        require(msg.value == order.strike, "Incorrect ETH amount sent");
 470:        require(!order.isLong, "Must be short position");
 475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
 481:        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
 527:        require(msg.sender == order.maker, "Not your order");
 551:        require(orders.length == signatures.length, "Length mismatch in input");
 552:        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");
 598:        require(token.code.length > 0, "ERC20: Token is not contract");
 599:        require(tokenAmount > 0, "ERC20: Amount too small");
 765:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");

contracts/src/PuttyV2Nft.sol:
  12:        require(to != address(0), "INVALID_RECIPIENT");
  13:        require(_ownerOf[id] == address(0), "ALREADY_MINTED");
  26:        require(from == _ownerOf[id], "WRONG_FROM");
  27:        require(to != address(0), "INVALID_RECIPIENT");
  41:        require(owner != address(0), "ZERO_ADDRESS");

Unnecessary initialization of variables with default values

Uninitialized variables are assigned with a default value depending on its type:

• uint: 0
• bool: false
• address: address(0)

Thus, explicitly initializing a variable with its default value costs unnecesary gas. For example, the following code:

bool b = false;
address c = address(0);
uint256 a = 0;

can be changed to:

uint256 a;
bool b;
address c;

Consider declaring the following lines without explicitly setting a value:

contracts/src/PuttyV2.sol:
 497:        uint256 feeAmount = 0;

Unnecessary definition of variables

Some variables are defined even though they are only used once in their respective functions. Not defining these variables and directly using the expressions on the right can help to reduce gas cost and contract size.

Instances include:

contracts/src/PuttyV2.sol:
 515:        uint256 floorPositionId = order.isCall ? longPositionId : uint256(orderHash);

Use calldata instead of memory for read-only function parameters

If a reference type function parameter, such as arrays, is read-only, it is cheaper to use calldata instead of memory. This would help to save gas as values are read directly from calldata using calldataload and avoids additional intermediate memory operations.

Consider changing memory to calldata in the following functions:

contracts/src/PuttyV2.sol:
 593:        function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
 610:        function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal {

 622:        function _transferFloorsIn(
 623:            address[] memory floorTokens,
 624:            uint256[] memory floorTokenIds,
 625:            address from
 626:        ) internal {

 636:        function _transferERC20sOut(ERC20Asset[] memory assets) internal {
 646:        function _transferERC721sOut(ERC721Asset[] memory assets) internal {
 657:        function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/src/PuttyV2.sol:
  89:        bytes32 public constant ERC721ASSET_TYPE_HASH =
  90:            keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));

  95:        bytes32 public constant ERC20ASSET_TYPE_HASH =
  96:            keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));

 101:        bytes32 public constant ORDER_TYPE_HASH =
 102:            keccak256(
 103:                abi.encodePacked(
 104:                    "Order(",
 105:                    "address maker,",
 106:                    "bool isCall,",
 107:                    "bool isLong,",
 108:                    "address baseAsset,",
 109:                    "uint256 strike,",
 110:                    "uint256 premium,",
 111:                    "uint256 duration,",
 112:                    "uint256 expiration,",
 113:                    "uint256 nonce,",
 114:                    "address[] whitelist,",
 115:                    "address[] floorTokens,",
 116:                    "ERC20Asset[] erc20Assets,",
 117:                    "ERC721Asset[] erc721Assets",
 118:                    ")",
 119:                    "ERC20Asset(address token,uint256 tokenAmount)",
 120:                    "ERC721Asset(address token,uint256 tokenId)"
 121:                )
 122:            );

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.