Gas Optimizations

[code423n4]

Summary

Gas Optimizations

	Issue	Instances
1	Using EIP-712 hashes for positionIds wastes gas	1
2	Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate	1
3	Using calldata instead of memory for read-only arguments in external functions saves gas	7
4	State variables should be cached in stack variables rather than re-reading them from storage	1
5	Multiple accesses of a mapping/array should use a local variable cache	6
6	<array>.length should not be looked up in every loop of a for-loop	10
7	++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops	10
8	Optimize names to save gas	1
9	Using bools for storage incurs overhead	2
10	Using > 0 costs more gas than != 0 when used on a uint in a require() statement	1
11	It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied	11
12	++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)	10
13	Using private rather than public for constants, saves gas	3
14	Inverting the condition of an if-else-statement wastes gas	1
15	Use custom errors rather than revert()/require() strings to save gas	33

Total: 98 instances over 15 issues

Gas Optimizations

1. Using EIP-712 hashes for positionIds wastes gas

EIP-712 introduces a lot of metadata into the data being hashed, in order for it to be able to be read before the data is signed. When the signature isn't being checked, it's inefficient to use these hashes over normal keccak256() hashes. This gist shows that the most simple order (all zero-byte values, no internal arrays) has an overhead of 658 gas as compared to normal hashing. More complicated orders, especially with internal arrays, will save even more. Message signing should be separate from positionId calculation, which will save gas. These hashes are used in all of the major functions of the project, so savings over time will be significant

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

305          // create opposite long/short position for taker
306          bytes32 oppositeOrderHash = hashOppositeOrder(order);
307          positionId = uint256(oppositeOrderHash);
308:         _mint(msg.sender, positionId);

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

2. Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

149       mapping(uint256 => uint256) public positionExpirations;
150   
151       /**
152           @notice Whether or not a position has been exercised. Maps 
153                   from positionId to isExercised.
154       */
155       mapping(uint256 => bool) public exercisedPositions;
156   
157       /**
158           @notice The floor asset token ids for a position. Maps from 
159                   positionId to floor asset token ids. This should only 
160                   be set for a long call position in `fillOrder`, or for 
161                   a short put position in `exercise`.
162       */
163:      mapping(uint256 => uint256[]) public positionFloorAssetTokenIds;

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

3. Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

There are 7 instances of this issue:

File: contracts/src/PuttyV2.sol

/// @audit _baseURI
209       constructor(
210           string memory _baseURI,
211           uint256 _fee,
212:          address _weth

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

/// @audit order
466:      function withdraw(Order memory order) public {

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

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

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

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

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

4. State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

/// @audit fee on line 498
499:                  feeAmount = (order.strike * fee) / 1000;

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

5. Multiple accesses of a mapping/array should use a local variable cache

The instances below point to the second+ access of a value inside a mapping/array, within a function. Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times, saves ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations. Caching an array's struct avoids recalculating the array offsets into memory/calldata

There are 6 instances of this issue:

File: contracts/src/PuttyV2.sol

/// @audit assets[i] on line 595
596:              uint256 tokenAmount = assets[i].tokenAmount;

/// @audit assets[i] on line 612
612:              ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);

/// @audit assets[i] on line 638
638:              ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);

/// @audit assets[i] on line 648
648:              ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);

/// @audit arr[i] on line 731
731:                  keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))

/// @audit arr[i] on line 745
745:                  keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))

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

6. <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

There are 10 instances of this issue:

File: 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++) {

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

7. ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

There are 10 instances of this issue:

File: 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++) {

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

8. Optimize names to save gas

public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

/// @audit setBaseURI(), setFee(), fillOrder(), exercise(), withdraw(), cancel(), batchFillOrder(), acceptCounterOffer(), isWhitelisted(), hashOppositeOrder(), hashOrder(), encodeERC20Assets(), encodeERC721Assets(), domainSeparatorV4()
53:   contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Ownable {

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

9. Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 2 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

143:      mapping(bytes32 => bool) public cancelledOrders;

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

File: contracts/src/PuttyV2.sol   #2

155:      mapping(uint256 => bool) public exercisedPositions;

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

10. Using > 0 costs more gas than != 0 when used on a uint in a require() statement

This change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

599:              require(tokenAmount > 0, "ERC20: Amount too small");

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

11. It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 11 instances of this issue:

File: contracts/src/PuttyV2.sol

497:              uint256 feeAmount = 0;

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++) {

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

12. ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

There are 10 instances of this issue:

File: 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++) {

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

13. Using private rather than public for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/src/PuttyV2.sol   #1

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

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

File: contracts/src/PuttyV2.sol   #2

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

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

File: contracts/src/PuttyV2.sol   #3

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:          );

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

14. Inverting the condition of an if-else-statement wastes gas

Flipping the true and false blocks instead saves 3 gas

There is 1 instance of this issue:

File: contracts/src/PuttyV2.sol   #1

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

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

15. Use custom errors rather than revert()/require() strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 33 instances of this issue:

File: 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");

28            require(
29                msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
30                "NOT_AUTHORIZED"
31:           );

41:           require(owner != address(0), "ZERO_ADDRESS");

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

File: 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");

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