code-423n4 / 2022-06-putty-findings

5 stars 0 forks source link

Gas Optimizations #292

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

List of contents:

[1] Data location of Order structure in function argument could be calldata

From the Solidity docs: Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

Function affected:

The manipulation of the order structure happen only once when calculates PuttyV2.sol#hashOppositeOrder() and this can be solved using this keyword allows us to pass memory params to a function that accepts calldata params. Gas diff table:

╭──────────────────────────────────┬─────────────────┬────────┬────────┬────────┬─────────╮
│ src/PuttyV2.sol:PuttyV2 contract ┆                 ┆        ┆        ┆        ┆         │
╞══════════════════════════════════╪═════════════════╪════════╪════════╪════════╪═════════╡
│ Deployment Cost                  ┆ Deployment Size ┆        ┆      ┆        ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ 4774098                          ┆ 25226           ┆        ┆        ┆       ┆         │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ Function Name                    ┆ min             ┆ avg    ┆ median ┆ max    ┆ # calls │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ cancel                           ┆ 3075            ┆ 26509  ┆ 34321  ┆ 34321  ┆ 4       │
│ cancel                           ┆ 703             ┆ 26005  ┆ 34440  ┆ 34440  ┆ 4       │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ exercise                         ┆ 5759            ┆ 55920  ┆ 68002 ┆ 133534 ┆ 18      │
│ exercise                         ┆ 4731            ┆ 57637  ┆ 69551 ┆ 140619 ┆ 18      │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ fillOrder                        ┆ 10014           ┆ 106845 ┆ 115883 ┆ 203777 ┆ 51      │
│ fillOrder                        ┆ 9121            ┆ 112948 ┆ 123603 ┆ 212253 ┆ 51      │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ hashOrder                        ┆ 5206            ┆ 5484   ┆ 5206   ┆ 7782   ┆ 62      │
│ hashOrder                        ┆ 4178            ┆ 4592   ┆ 4178   ┆ 7011   ┆ 114     │
├╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
│ withdraw                         ┆ 3075            ┆ 27840  ┆ 24545 ┆ 71168  ┆ 10      │
│ withdraw                         ┆ 697             ┆ 31196  ┆ 28491 ┆ 78172  ┆ 10      │
╰──────────────────────────────────┴─────────────────┴────────┴────────┴────────┴─────────╯

[2] > 0 is less efficient than != 0 for unsigned integers

If you enable the optimizer at 10k AND you're in a require statement, this will save gas. I suggest changing > 0 with != 0 here:

src/PuttyV2.sol:
292          // check base asset exists
293:         require(order.baseAsset.code.length > 0, "baseAsset is not contract");
294  

326              // handle the case where the user uses native ETH instead of WETH to pay the premium
327:             if (weth == order.baseAsset && msg.value > 0) {
328                  // check enough ETH was sent to cover the premium

350              // handle the case where the taker uses native ETH instead of WETH to deposit the strike
351:             if (weth == order.baseAsset && msg.value > 0) {
352                  // check enough ETH was sent to cover the strike

426              // handle the case where the taker uses native ETH instead of WETH to pay the strike
427:             if (weth == order.baseAsset && msg.value > 0) {
428                  // check enough ETH was sent to cover the strike

497              uint256 feeAmount = 0;
498:             if (fee > 0) {
499                  feeAmount = (order.strike * fee) / 1000;

597  
598:             require(token.code.length > 0, "ERC20: Token is not contract");
599:             require(tokenAmount > 0, "ERC20: Amount too small");
600 

[3] Setting uint256 variables to 0 is redundant

Default value is zero no need to assign 0

src/PuttyV2.sol:
496              // send the fee to the admin/DAO if fee is greater than 0%
497:             uint256 feeAmount = 0;
498              if (fee > 0) {

[4] Optimizing for-loop

  1. Using ++i consumes 5 less gas than i++
  2. Unchecked{ ++i; } consumes 49 less gas each iteration
  3. Default value is zero no need to assign i = 0
    
    src/PuttyV2.sol:
    555          
    556:         for (uint256 i = 0; i < orders.length; i++) {
    557              positionIds[i] = fillOrder(orders[i], signatures[i], floorAssetTokenIds[i]);

593 function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal { 594: for (uint256 i = 0; i < assets.length; i++) { 595 address token = assets[i].token;

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

626 ) internal { 627: for (uint256 i = 0; i < floorTokens.length; i++) { 628 ERC721(floorTokens[i]).safeTransferFrom(from, address(this), floorTokenIds[i]);

636 function _transferERC20sOut(ERC20Asset[] memory assets) internal { 637: for (uint256 i = 0; i < assets.length; i++) { 638 ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);

646 function _transferERC721sOut(ERC721Asset[] memory assets) internal { 647: for (uint256 i = 0; i < assets.length; i++) { 648 ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);

657 function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal { 658: for (uint256 i = 0; i < floorTokens.length; i++) { 659 ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);

669 function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) { 670: for (uint256 i = 0; i < whitelist.length; i++) { 671 if (target == whitelist[i]) return true;

727 function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) { 728: for (uint256 i = 0; i < arr.length; i++) { 729 encoded = abi.encodePacked(

741 function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) { 742: for (uint256 i = 0; i < arr.length; i++) { 743 encoded = abi.encodePacked(



## [5] `External` function consume less gas than `public`
As for best practices, you should use external if you expect that the function will only ever be called externally, and use public if you need to call the function internally.
Functions not used internally:
 - [`PuttyV2.sol#exercise()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L389).
 - [`PuttyV2.sol#withdraw()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L466).
 - [`PuttyV2.sol#batchFillOrder()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L546).
 - [`PuttyV2.sol#acceptCounterOffer()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L573).
 - [`PuttyV2.sol#domainSeparatorV4()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L773).
 - [`PuttyV2.sol#tokenURI()`](https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L764).