QA Report

[code423n4]

QA

Unnecesary cast to uint256

The CriteriaResolution.sol lib on line 155 is making an unnecesary cast to uint256;

if (identifierOrCriteria != uint256(0)) {

I sugges to simplify it to;

if (identifierOrCriteria != 0) {

Also on Executor.sol lines 283 and 287

Consider change; uint256(1), to 1, and uint256(0), to 0

Use same style on all loops

In some loops the style is something like;

unchecked {
    for(uint256 i = 0; i < total; ++i) {
        // code
    }
}

But in other places this style appears;

for(uint256 i = 0; i < total;) {
    // code, with operations wrapped in unchecked

    // Skip overflow check as for loop is indexed starting at zero.
    unchecked {
        ++i;
    }
}

It would be nice to mantein the style pattern, for example in contracts/conduit/Conduit.sol#L65-L77 you could;

diff --git a/contracts/conduit/Conduit.sol b/contracts/conduit/Conduit.sol
index 0169c34..9cfdeba 100644
--- a/contracts/conduit/Conduit.sol
+++ b/contracts/conduit/Conduit.sol
@@ -62,17 +62,14 @@ contract Conduit is ConduitInterface, TokenTransferrer {
         // Retrieve the total number of transfers and place on the stack.
         uint256 totalStandardTransfers = transfers.length;

-        // Iterate over each transfer.
-        for (uint256 i = 0; i < totalStandardTransfers; ) {
-            // Retrieve the transfer in question.
-            ConduitTransfer calldata standardTransfer = transfers[i];
-
-            // Perform the transfer.
-            _transfer(standardTransfer);
-
-            // Skip overflow check as for loop is indexed starting at zero.
-            unchecked {
-                ++i;
+        unchecked {
+            // Iterate over each transfer.
+            for (uint256 i = 0; i < totalStandardTransfers; ++i) {
+                // Retrieve the transfer in question.
+                ConduitTransfer calldata standardTransfer = transfers[i];
+
+                // Perform the transfer.
+                _transfer(standardTransfer);
             }
         }

And also on lines contracts/conduit/Conduit.sol#L129-L141

diff --git a/contracts/conduit/Conduit.sol b/contracts/conduit/Conduit.sol
index 0169c34..30aa86f 100644
--- a/contracts/conduit/Conduit.sol
+++ b/contracts/conduit/Conduit.sol
@@ -126,17 +123,14 @@ contract Conduit is ConduitInterface, TokenTransferrer {
         // Retrieve the total number of transfers and place on the stack.
         uint256 totalStandardTransfers = standardTransfers.length;

-        // Iterate over each standard transfer.
-        for (uint256 i = 0; i < totalStandardTransfers; ) {
-            // Retrieve the transfer in question.
-            ConduitTransfer calldata standardTransfer = standardTransfers[i];
-
-            // Perform the transfer.
-            _transfer(standardTransfer);
+        unchecked {
+            // Iterate over each standard transfer.
+            for (uint256 i = 0; i < totalStandardTransfers; ++i) {
+                // Retrieve the transfer in question.
+                ConduitTransfer calldata standardTransfer = standardTransfers[i];

-            // Skip overflow check as for loop is indexed starting at zero.
-            unchecked {
-                ++i;
+                // Perform the transfer.
+                _transfer(standardTransfer);
             }
         }

On file contracts/lib/BasicOrderFulfiller.sol#L947-L978 you could wrap the loop in an unchecked block;

diff --git a/contracts/lib/BasicOrderFulfiller.sol b/contracts/lib/BasicOrderFulfiller.sol
index d59af7f..dedc0e2 100644
--- a/contracts/lib/BasicOrderFulfiller.sol
+++ b/contracts/lib/BasicOrderFulfiller.sol
@@ -944,37 +944,32 @@ contract BasicOrderFulfiller is OrderValidator {
         // Retrieve total number of additional recipients and place on stack.
         uint256 totalAdditionalRecipients = additionalRecipients.length;

-        // Iterate over each additional recipient.
-        for (uint256 i = 0; i < totalAdditionalRecipients; ) {
-            // Retrieve the additional recipient.
-            AdditionalRecipient calldata additionalRecipient = (
-                additionalRecipients[i]
-            );
+        unchecked {
+            // Iterate over each additional recipient.
+            for (uint256 i = 0; i < totalAdditionalRecipients; ++i) {
+                // Retrieve the additional recipient.
+                AdditionalRecipient calldata additionalRecipient = (
+                    additionalRecipients[i]
+                );

-            // Read ether amount to transfer to recipient and place on stack.
-            uint256 additionalRecipientAmount = additionalRecipient.amount;
+                // Read ether amount to transfer to recipient and place on stack.
+                uint256 additionalRecipientAmount = additionalRecipient.amount;

-            // Ensure that sufficient Ether is available.
-            if (additionalRecipientAmount > etherRemaining) {
-                revert InsufficientEtherSupplied();
-            }
+                // Ensure that sufficient Ether is available.
+                if (additionalRecipientAmount > etherRemaining) {
+                    revert InsufficientEtherSupplied();
+                }

-            // Transfer Ether to the additional recipient.
-            _transferEth(
-                additionalRecipient.recipient,
-                additionalRecipientAmount
-            );
+                // Transfer Ether to the additional recipient.
+                _transferEth(
+                    additionalRecipient.recipient,
+                    additionalRecipientAmount
+                );

-            // Skip underflow check as subtracted value is less than remaining.
-            unchecked {
+                // Skip underflow check as subtracted value is less than remaining.
                 // Reduce ether value available.
                 etherRemaining -= additionalRecipientAmount;
             }
-
-            // Skip overflow check as for loop is indexed starting at zero.
-            unchecked {
-                ++i;
-            }
         }

         // Ensure that sufficient Ether is still available.

On file contracts/lib/OrderCombiner.sol#L620-L651 its also possible to wrap the for loop inside a unchecked block;

diff --git a/contracts/lib/OrderCombiner.sol b/contracts/lib/OrderCombiner.sol
index babef49..34a6568 100644
--- a/contracts/lib/OrderCombiner.sol
+++ b/contracts/lib/OrderCombiner.sol
@@ -617,36 +617,31 @@ contract OrderCombiner is OrderFulfiller, FulfillmentApplier {
         // accessed and modified, however.
         bytes memory accumulator = new bytes(AccumulatorDisarmed);

-        // Iterate over each execution.
-        for (uint256 i = 0; i < executions.length; ) {
-            // Retrieve the execution and the associated received item.
-            Execution memory execution = executions[i];
-            ReceivedItem memory item = execution.item;
-
-            // If execution transfers native tokens, reduce value available.
-            if (item.itemType == ItemType.NATIVE) {
-                // Ensure that sufficient native tokens are still available.
-                if (item.amount > etherRemaining) {
-                    revert InsufficientEtherSupplied();
-                }
+        unchecked {
+            // Iterate over each execution.
+            for (uint256 i = 0; i < executions.length; ++i) {
+                // Retrieve the execution and the associated received item.
+                Execution memory execution = executions[i];
+                ReceivedItem memory item = execution.item;
+
+                // If execution transfers native tokens, reduce value available.
+                if (item.itemType == ItemType.NATIVE) {
+                    // Ensure that sufficient native tokens are still available.
+                    if (item.amount > etherRemaining) {
+                        revert InsufficientEtherSupplied();
+                    }

-                // Skip underflow check as amount is less than ether remaining.
-                unchecked {
+                    // Skip underflow check as amount is less than ether remaining.
                     etherRemaining -= item.amount;
                 }
-            }

-            // Transfer the item specified by the execution.
-            _transfer(
-                item,
-                execution.offerer,
-                execution.conduitKey,
-                accumulator
-            );
-
-            // Skip overflow check as for loop is indexed starting at zero.
-            unchecked {
-                ++i;
+                // Transfer the item specified by the execution.
+                _transfer(
+                    item,
+                    execution.offerer,
+                    execution.conduitKey,
+                    accumulator
+                );
             }
         }

diff --git a/contracts/lib/OrderFulfiller.sol b/contracts/lib/OrderFulfiller.sol
index 0060ba9..7b50ef9 100644
--- a/contracts/lib/OrderFulfiller.sol
+++ b/contracts/lib/OrderFulfiller.sol
@@ -213,8 +213,9 @@ contract OrderFulfiller is
                 }
             }

+            unchecked {
             // Iterate over each offer on the order.
-            for (uint256 i = 0; i < orderParameters.offer.length; ) {
+            for (uint256 i = 0; i < orderParameters.offer.length; ++i) {
                 // Retrieve the offer item.
                 OfferItem memory offerItem = orderParameters.offer[i];

@@ -249,9 +250,7 @@ contract OrderFulfiller is
                     }

                     // Skip underflow check as a comparison has just been made.
-                    unchecked {
-                        etherRemaining -= amount;
-                    }
+                    etherRemaining -= amount;
                 }

                 // Transfer the item from the offerer to the caller.
@@ -261,11 +260,7 @@ contract OrderFulfiller is
                     offererConduitKey,
                     accumulator
                 );
-
-                // Skip overflow check as for loop is indexed starting at zero.
-                unchecked {
-                    ++i;
-                }
+            }
             }
         }

@@ -302,8 +297,9 @@ contract OrderFulfiller is
                 }
             }

+            unchecked {
             // Iterate over each consideration item on the order.
-            for (uint256 i = 0; i < orderParameters.consideration.length; ) {
+            for (uint256 i = 0; i < orderParameters.consideration.length; ++i) {
                 // Retrieve the consideration item.
                 ConsiderationItem memory considerationItem = (
                     orderParameters.consideration[i]
@@ -349,9 +345,7 @@ contract OrderFulfiller is
                     }

                     // Skip underflow check as a comparison has just been made.
-                    unchecked {
-                        etherRemaining -= amount;
-                    }
+                    etherRemaining -= amount;
                 }

                 // Transfer item from caller to recipient specified by the item.
@@ -361,11 +355,7 @@ contract OrderFulfiller is
                     fulfillerConduitKey,
                     accumulator
                 );
-
-                // Skip overflow check as for loop is indexed starting at zero.
-                unchecked {
-                    ++i;
-                }
+            }
             }
         }

Add message in test reverts

In EIP1271Wallet.sol, inside test folder, in the isValidSignature(bytes32,bytes) the reverts don't have message Add message to help the tester

[HardlyDifficult]

Unnecesary cast to uint256

This is just a style preference, changing would not impact the bytecode produced.

Use same style on all loops

Agree, it's a fair point that they could be more consistent internally. Some of the suggested changes here could have minor gas savings as well. But for the most part these suggestions appear to be just minor style preferences.

Add message in test reverts

Revert messages can be helpful, but since these are test files it's not important to the protocol itself.

[GalloDaSballo]

Casting -> The entire codebase always casts to explicit type, this is a style-convention that is not broken inside the reference and implementation contract (only tests break this), for that reason I don't think the finding is valid

Loops Would be a valid gas report

Revert Test file so agree with out of scope

Agree with the judges decision

[HardlyDifficult]

The bar for QA reports in this contest is at least 2 valid non-critical findings or at least one valid low risk finding. Per the comments above, this submission is below that bar -- closing as invalid.