Putty contest - gogo's results

Redundant import

Proof on concept

Redundant import of "openzeppelin/utils/String.sol". Library not used in the context of the file/contract.

Recommendation

Remove the import statement at line 5 of the PuttyV2Nft.sol

Scope

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2Nft.sol#L5

Gas optimisations

For loop gas optimisations

Array.length should not be looked up in every loop of a for-loop

Proof of concept

One of most the expensive opcodes is SLOAD.
It is better to cache arr.length in a stack variable like uint256 arrLength in order to save gas upon each lookup of the for-loops.

Recommendation

Cache arrays' length in memory in order to save gas from SLOAD on every iteration.

Use ++i instead of i++, especially when it’s used in for-loops (--i/i-- too)

Proof of concept

++i costs less gas than i++

Recommendation

Replace each i++ with ++i, especially in for-loops

Do not initialize i counter in for-loops

Proof of concept

Uint256's default value is zero so it is useless and a waste of gas to initialize it to 0

Recommendation

Use uint256 i; instead of uint256 i = 0;

Example

- 497   uint256 feeAmount = 0;
+ 497   uint256 feeAmount;

Put ++i in unchecked {}

Proof of concept

Since Solidity version 0.8 overflow and underflow checks are implemented in Solidity compiler. \ So it is applied by default on every arithmetic operation and costs additional gas.
In the case of for-loop i will always be less than the target (e.g. arr.length) so those checks are unnecessary.

Recommendation

Put each ++i in unchecked {} in for-loops

Example

- 556   for (uint256 i = 0; i < orders.length; ++i) {

+ 556   for (uint256 i = 0; i < orders.length;) {
+ 557      ...
+ 558      unchecked {
+ 559         ++i
+ 560      }
+ 561   }

Scope (for-loop gas optimisations)

In https://github.com/code-423n4/2022-06-putty/blob/main/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 < orders.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++)

Use custom errors instead of require with string expressions

Proof of concept

Custom errors introduced in Solidity version 0.8.4 cost less than require()/revert() because of the missing string messages.

Recommendation

Use if checks with descriptive custom errors instead of require expressions with string parameters.

Example

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

- 551   require(orders.length == signatures.length, "Length mismatch in input");
+ 551   if(orders.length != signatures.length) revert LengthMismatch();

Scope

https://github.com/code-423n4/2022-06-putty/blob/main/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");

Redundant return statements

Proof of concept

Use else if instead of return expressions

Scope

In function fillOrder https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol

  345   return positionId;

  363   return positionId;

  370   return positionId;

  378   return positionId;

Use return expression instead of naming a return variable

Proof of concept

There is no reason to use a named return varible instead of directly returned value in this cases:

Scope

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

Use calldata instead of memory location for reference type variable

Proof of concept

Storing function parameters is cheaper if they are located in calldata because copying them in memory cost gas

Recommendation

Store function parameters in calldata where possible and be careful

Scope

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L269
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L389
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L466
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L526
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L547
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L574
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L576
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L593
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L610
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L623
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L636
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L646
https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L657 floorTokens only!<br> https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L669 <br> https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L683

Use external instead of public function visibility modifiers

Proof of concept

Functions that are not being called from inside the contract through its lifecycle should be marked as external. External function parameters are located in calldata by default which saves gas comparing to the memory location in public functions.

Scope

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