OpenSea Seaport contest - shung's results

QA Report

An order can be cancelled more than once

OrderValidator._cancel does not check if an order is already cancelled. This can cause OrderCancelled event to emit more than once for the same order. Consider either throwing when _cancel is called on an already cancelled order, or wrapping the event emission with an if statement.

Typos

• OrderCombiner.sol:L365: revertOnValid → revertOnInvalid
• ConsiderationStructs.sol:L68: an → and

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.