Gas Optimizations

[code423n4]

Gas Report

Inefficient struct utilization

Functions in OrderValidator retreive _orderStatus[] using a memory location. This necessiates inefficient mapping lookup operations when writing to the storage. For example, the following block performs the same offset calculation four times.

lib/OrderValidator.sol:70:       _orderStatus[orderHash].isValidated = true;
lib/OrderValidator.sol:71:       _orderStatus[orderHash].isCancelled = false;
lib/OrderValidator.sol:72:       _orderStatus[orderHash].numerator = 1;
lib/OrderValidator.sol:73:       _orderStatus[orderHash].denominator = 1;

I suggest using a storage pointer instead of memory location when retreiving _orderStatus[].

Reference diff

Below is a diff file that applies the suggested change. This is for reference only.

diff --git a/contracts/lib/OrderValidator.sol b/contracts/lib/OrderValidator.sol
index 09f10cb..7160ce0 100644
--- a/contracts/lib/OrderValidator.sol
+++ b/contracts/lib/OrderValidator.sol
@@ -51,7 +51,7 @@ contract OrderValidator is Executor, ZoneInteraction {
         bytes memory signature
     ) internal {
         // Retrieve the order status for the given order hash.
-        OrderStatus memory orderStatus = _orderStatus[orderHash];
+        OrderStatus storage orderStatus = _orderStatus[orderHash];

         // Ensure order is fillable and is not cancelled.
         _verifyOrderStatus(
@@ -67,10 +67,10 @@ contract OrderValidator is Executor, ZoneInteraction {
         }

         // Update order status as fully filled, packing struct values.
-        _orderStatus[orderHash].isValidated = true;
-        _orderStatus[orderHash].isCancelled = false;
-        _orderStatus[orderHash].numerator = 1;
-        _orderStatus[orderHash].denominator = 1;
+        orderStatus.isValidated = true;
+        orderStatus.isCancelled = false;
+        orderStatus.numerator = 1;
+        orderStatus.denominator = 1;
     }

     /**
@@ -165,7 +165,7 @@ contract OrderValidator is Executor, ZoneInteraction {
         );

         // Retrieve the order status using the derived order hash.
-        OrderStatus memory orderStatus = _orderStatus[orderHash];
+        OrderStatus storage orderStatus = _orderStatus[orderHash];

         // Ensure order is fillable and is not cancelled.
         if (
@@ -223,19 +223,19 @@ contract OrderValidator is Executor, ZoneInteraction {
             // Skip overflow check: checked above unless numerator is reduced.
             unchecked {
                 // Update order status and fill amount, packing struct values.
-                _orderStatus[orderHash].isValidated = true;
-                _orderStatus[orderHash].isCancelled = false;
-                _orderStatus[orderHash].numerator = uint120(
+                orderStatus.isValidated = true;
+                orderStatus.isCancelled = false;
+                orderStatus.numerator = uint120(
                     filledNumerator + numerator
                 );
-                _orderStatus[orderHash].denominator = uint120(denominator);
+                orderStatus.denominator = uint120(denominator);
             }
         } else {
             // Update order status and fill amount, packing struct values.
-            _orderStatus[orderHash].isValidated = true;
-            _orderStatus[orderHash].isCancelled = false;
-            _orderStatus[orderHash].numerator = uint120(numerator);
-            _orderStatus[orderHash].denominator = uint120(denominator);
+            orderStatus.isValidated = true;
+            orderStatus.isCancelled = false;
+            orderStatus.numerator = uint120(numerator);
+            orderStatus.denominator = uint120(denominator);
         }

         // Return order hash, a modified numerator, and a modified denominator.
@@ -300,8 +300,9 @@ contract OrderValidator is Executor, ZoneInteraction {
                 );

                 // Update the order status as not valid and cancelled.
-                _orderStatus[orderHash].isValidated = false;
-                _orderStatus[orderHash].isCancelled = true;
+                OrderStatus storage orderStatus = _orderStatus[orderHash];
+                orderStatus.isValidated = false;
+                orderStatus.isCancelled = true;

                 // Emit an event signifying that the order has been cancelled.
                 emit OrderCancelled(orderHash, offerer, zone);
@@ -363,7 +364,7 @@ contract OrderValidator is Executor, ZoneInteraction {
                 );

                 // Retrieve the order status using the derived order hash.
-                OrderStatus memory orderStatus = _orderStatus[orderHash];
+                OrderStatus storage orderStatus = _orderStatus[orderHash];

                 // Ensure order is fillable and retrieve the filled amount.
                 _verifyOrderStatus(
@@ -379,7 +380,7 @@ contract OrderValidator is Executor, ZoneInteraction {
                     _verifySignature(offerer, orderHash, order.signature);

                     // Update order status to mark the order as valid.
-                    _orderStatus[orderHash].isValidated = true;
+                    orderStatus.isValidated = true;

                     // Emit an event signifying the order has been validated.
                     emit OrderValidated(
diff --git a/contracts/lib/Verifiers.sol b/contracts/lib/Verifiers.sol
index b0cf467..483bf08 100644
--- a/contracts/lib/Verifiers.sol
+++ b/contracts/lib/Verifiers.sol
@@ -85,7 +85,7 @@ contract Verifiers is Assertions, SignatureVerification {
     }

     /**
-     * @dev Internal pure function to validate that a given order is fillable
+     * @dev Internal view function to validate that a given order is fillable
      *      and not cancelled based on the order status.
      *
      * @param orderHash       The order hash.
@@ -101,10 +101,10 @@ contract Verifiers is Assertions, SignatureVerification {
      */
     function _verifyOrderStatus(
         bytes32 orderHash,
-        OrderStatus memory orderStatus,
+        OrderStatus storage orderStatus,
         bool onlyAllowUnused,
         bool revertOnInvalid
-    ) internal pure returns (bool valid) {
+    ) internal view returns (bool valid) {
         // Ensure that the order has not been cancelled.
         if (orderStatus.isCancelled) {
             // Only revert if revertOnInvalid has been supplied as true.

Gas savings table

I have ran the provided yarn profile script to compare the gas costs before and after applying the reference diff. All external mutative Seaport functions showed considerable gas savings, except incrementNonce(), which has nothing to do with order validation. Below is a table comparing minimum and maximum gas costs of all external mutative Seaport functions, except incrementNonce(), before and after applying the diff.

Function	Old Cost	New Cost
cancel	44124–61296	43831–61003
fulfillAdvancedOrder	104014–209645	103219–208885
fulfillAvailableAdvancedOrders	173305–229465	172553–227921
fulfillAvailableOrders	172879–229282	172088–227698
fulfillBasicOrder	93563–1624267	92188–1622918
fulfillOrder	102728–213402	101946–212633
matchAdvancedOrders	206604–272820	205006–271277
matchOrders	166719–366925	165177–365347
validate	58025–69440	57765–69232

[HardlyDifficult]

This is a straightforward recommendation that has a non-trivial impact on the core functions. It's also well explained. I ran the recommended change and confirmed decent gas savings across the board. This seems to be a worthwhile change to consider including.

[IllIllI000]

This gas report is about a single issue and is ranked 85. This same exact issue, as well as many more instances the same issue are flagged in https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/144 as the third item "Multiple accesses of a mapping/array should use a local variable cache" which has a proof of concept showing the gas savings in a concise example, along with many other gas savings but is only ranked 60. @HardlyDifficult @0xleastwood can you provide more clarity on how the ranking has been performed?

[HardlyDifficult]

This gas report is about a single issue and is ranked 85. This same exact issue, as well as many more instances the same issue are flagged in #144 as the third item "Multiple accesses of a mapping/array should use a local variable cache" which has a proof of concept showing the gas savings in a concise example, along with many other gas savings but is only ranked 60. @HardlyDifficult @0xleastwood can you provide more clarity on how the ranking has been performed?

They are similar, but this report is more useful. In 144 the report is focused on the access costs to save 42 gas per instance and dumps every instance where that may apply. This report zoomed into a specific change that results in a sizeable gas savings in total for critical functions in a way that is easy to confirm.

This report scores higher for focusing on just the most impactful changes, providing an easy to follow diff of the recommended changes, and highlighting the expected total benefit. The approach used here makes it easy to conclude the recommendation provided is worth close consideration. The sponsor's time should be respected so we scored reports which read like static analysis reports lower (and if they offered no targeted insights they were closed as invalid).