search

code-423n4 / 2022-05-opensea-seaport-findings

1 stars 0 forks source link

QA Report #169

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Table of Contents:

[L-01] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Same finding from other contests:

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

contracts/conduit/ConduitController.sol:
   72                      keccak256(
   73:                         abi.encodePacked(
   74                              bytes1(0xff),
   75                              address(this),
   76                              conduitKey,
   77                              _CONDUIT_CREATION_CODE_HASH
   78                          )
   79                      )

  324                      keccak256(
  325:                         abi.encodePacked(
  326                              bytes1(0xff),
  327                              address(this),
  328                              conduitKey,
  329                              _CONDUIT_CREATION_CODE_HASH
  330                          )
  331                      )

contracts/lib/ConsiderationBase.sol:
  193          eip712DomainTypehash = keccak256(
  194:             abi.encodePacked(
  195                  "EIP712Domain(",
  196                      "string name,",
  197                      "string version,",
  198                      "uint256 chainId,",
  199                      "address verifyingContract",
  200                  ")"
  201              )
  202          );

  211          orderTypehash = keccak256(
  212:             abi.encodePacked(
  213                  orderComponentsPartialTypeString,
  214                  considerationItemTypeString,
  215                  offerItemTypeString
  216              )
  217          );

[N-01] Use at least Solidity 0.8.12 to get string.concat()

Use a solidity version of at least 0.8.12 to be able to use string.concat() instead of abi.encodePacked():

contracts/conduit/ConduitController.sol:
    2: pragma solidity >=0.8.7;
   73:                         abi.encodePacked(
  325:                         abi.encodePacked(

Additionally, consider using string.concat() instead of abi.encodePacked() here:

contracts/lib/ConsiderationBase.sol:
    2: pragma solidity 0.8.13;
  150:         bytes memory offerItemTypeString = abi.encodePacked(
  162:         bytes memory considerationItemTypeString = abi.encodePacked(
  175:         bytes memory orderComponentsPartialTypeString = abi.encodePacked(
  194:             abi.encodePacked(
  212:             abi.encodePacked(

[N-02] Unused named returns

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

215:     function fulfillAvailableOrders(
...
225:         returns (bool[] memory availableOrders, Execution[] memory executions)
226:     {
...
228:         return
229:             _fulfillAvailableAdvancedOrders(
230:                 _convertOrdersToAdvanced(orders), // Convert to advanced orders.
231:                 new CriteriaResolver[](0), // No criteria resolvers supplied.
232:                 offerFulfillments,
233:                 considerationFulfillments,
234:                 fulfillerConduitKey,
235:                 maximumFulfilled
236:             );
237:     }
300:     function fulfillAvailableAdvancedOrders(
...
311:         returns (bool[] memory availableOrders, Execution[] memory executions)
312:     {
...
314:         return
315:             _fulfillAvailableAdvancedOrders(
316:                 advancedOrders,
317:                 criteriaResolvers,
318:                 offerFulfillments,
319:                 considerationFulfillments,
320:                 fulfillerConduitKey,
321:                 maximumFulfilled
322:             );
323:     }
349:     function matchOrders(
...
352:     ) external payable override returns (Execution[] memory executions) {
...
354:         return
355:             _matchAdvancedOrders(
356:                 _convertOrdersToAdvanced(orders),
357:                 new CriteriaResolver[](0), // No criteria resolvers supplied.
358:                 fulfillments
359:             );
360:     }
398:     function matchAdvancedOrders(
...
402:     ) external payable override returns (Execution[] memory executions) {
...
404:         return
405:             _matchAdvancedOrders(
406:                 advancedOrders,
407:                 criteriaResolvers,
408:                 fulfillments
409:             );
410:     }
107:     function _fulfillAvailableAdvancedOrders(
...
116:         returns (bool[] memory availableOrders, Execution[] memory executions)
117:     {
...
135:         return (availableOrders, executions);
445:     function _executeAvailableFulfillments(
...
452:         returns (bool[] memory availableOrders, Execution[] memory executions)
453:     {
...
547:         return (availableOrders, executions);
548:     }
704:     function _matchAdvancedOrders(
...
708:     ) internal returns (Execution[] memory executions) {
...
718:         return _fulfillAdvancedOrders(advancedOrders, fulfillments);
719:     }
738:     function _fulfillAdvancedOrders(
...
741:     ) internal returns (Execution[] memory executions) {
...
791:         return (executions);
792:     }
457:     function _convertOrdersToAdvanced(Order[] calldata orders)
...
460:         returns (AdvancedOrder[] memory advancedOrders)
...
478:         return advancedOrders;
479:     }

[N-03] It's better to emit after all processing is done

contracts/conduit/ConduitController.sol (1):

  201:         // Emit an event indicating that the potential owner has been updated.
  202:         emit PotentialOwnerUpdated(conduit, newPotentialOwner);
  203  
  204          // Set the new potential owner as the potential owner of the conduit.
  205          _conduits[conduit].potentialOwner = newPotentialOwner;

contracts/conduit/ConduitController.sol (2):

  218:         // Emit an event indicating that the potential owner has been cleared.
  219:         emit PotentialOwnerUpdated(conduit, address(0));
  220  
  221          // Clear the current new potential owner from the conduit.
  222          delete _conduits[conduit].potentialOwner;

contracts/conduit/ConduitController.sol (3 & 4):

  242:         // Emit an event indicating that the potential owner has been cleared.
  243:         emit PotentialOwnerUpdated(conduit, address(0));
  244  
  245          // Clear the current new potential owner from the conduit.
  246          delete _conduits[conduit].potentialOwner;
  247  
  248:         // Emit an event indicating conduit ownership has been transferred.
  249:         emit OwnershipTransferred(
  250              conduit,
  251              _conduits[conduit].owner,
  252              msg.sender
  253          );
  254  
  255          // Set the caller as the owner of the conduit.
  256          _conduits[conduit].owner = msg.sender;

[N-04] Avoid floating pragmas: the version should be locked

contracts/conduit/Conduit.sol:
  2: pragma solidity >=0.8.7;

contracts/conduit/ConduitController.sol:
  2: pragma solidity >=0.8.7;

contracts/conduit/lib/ConduitEnums.sol:
  2: pragma solidity >=0.8.7;

contracts/conduit/lib/ConduitStructs.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/AbridgedTokenInterfaces.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/AmountDerivationErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ConduitControllerInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ConduitInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ConsiderationEventsAndErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ConsiderationInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/CriteriaResolutionErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/EIP1271Interface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/FulfillmentApplicationErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ImmutableCreate2FactoryInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ReentrancyErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/SeaportInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/SignatureVerificationErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/TokenTransferrerErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ZoneInteractionErrors.sol:
  2: pragma solidity >=0.8.7;

contracts/interfaces/ZoneInterface.sol:
  2: pragma solidity >=0.8.7;

contracts/lib/ConsiderationConstants.sol:
  2: pragma solidity >=0.8.7;

contracts/lib/ConsiderationEnums.sol:
  2: pragma solidity >=0.8.7;

contracts/lib/ConsiderationStructs.sol:
  2: pragma solidity >=0.8.7;

contracts/lib/TokenTransferrer.sol:
  2: pragma solidity >=0.8.7;

contracts/lib/TokenTransferrerConstants.sol:
  2: pragma solidity >=0.8.7;

[N-05] Missing NatSpec comments

The following comments are missing (see @audit tags):

  139      /**
  140       * @notice Initiate conduit ownership transfer by assigning a new potential
  141       *         owner for the given conduit. Once set, the new potential owner
  142       *         may call `acceptOwnership` to claim ownership of the conduit.
  143       *         Only the owner of the conduit in question may call this function.
  144       *
  145:      * @param conduit The conduit for which to initiate ownership transfer. //@audit missing @param newPotentialOwner
  146       */
  147      function transferOwnership(address conduit, address newPotentialOwner)
  148          external;
  123      /**
  124       * @dev Internal pure function to apply a fraction to a consideration
  125       * or offer item.
  126       *
  127       * @param startAmount     The starting amount of the item.
  128       * @param endAmount       The ending amount of the item.
  129       * @param numerator       A value indicating the portion of the order that
  130       *                        should be filled.
  131       * @param denominator     A value indicating the total size of the order.
  132       * @param elapsed         The time elapsed since the order's start time.
  133       * @param remaining       The time left until the order's end time.
  134:      * @param duration        The total duration of the order. //@audit missing @param roundUp
  135       *
  136       * @return amount The received item to transfer with the final amount.
  137       */
  138      function _applyFraction(
  139          uint256 startAmount,
  140          uint256 endAmount,
  141          uint256 numerator,
  142          uint256 denominator,
  143          uint256 elapsed,
  144          uint256 remaining,
  145          uint256 duration,
  146          bool roundUp
  147      ) internal pure returns (uint256 amount) {
  1001      /**
  1002       * @dev Internal function to transfer ERC20 tokens to a given recipient as
  1003       *      part of basic order fulfillment.
  1004       *
  1005       * @param from                 The originator of the ERC20 token transfer.
  1006       * @param to                   The recipient of the ERC20 token transfer.
  1007       * @param erc20Token           The ERC20 token to transfer.
  1008       * @param amount               The amount of ERC20 tokens to transfer.
  1009       * @param additionalRecipients The additional recipients of the order.
  1010       * @param fromOfferer          A boolean indicating whether to decrement
  1011:      *                             amount from the offered amount.  //@audit missing @param accumulator
  1012       */
  1013      function _transferERC20AndFinalize(
  1014          address from,
  1015          address to,
  1016          address erc20Token,
  1017          uint256 amount,
  1018          AdditionalRecipient[] calldata additionalRecipients,
  1019          bool fromOfferer,
  1020          bytes memory accumulator
  1021      ) internal {
GalloDaSballo commented 2 years ago

[L-01] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Don't believe any meaningful risk was shown, you'd want to avoid using encodePacked to avoid hash-clashes, however none were demonstrated

[N-01] Use at least Solidity 0.8.12 to get string.concat()

This would be just syntactic sugar, don't think it's valid

[N-02] Unused named returns

The codebase always returns the named variable explicitly, which is done consistently. Hence the named return is used.

[N-03] It's better to emit after all processing is done

Given the specific context, I assume the code was re-written to avoid false positives from Slither (state change before "external calls". I believe the events are emitted properly, and consistently

[N-04] Avoid floating pragmas: the version should be locked

Disagree per discussion on #67

[N-05] Missing NatSpec comments

I don't believe this to be valid, natspec is always optional

I think the judges were correct in marking this report invalid

IllIllI000 commented 2 years ago

NatSpec is optional, but if it is added, it's inconsistent to only list some of the params

IllIllI000 commented 2 years ago

@GalloDaSballo and the sponsor ended up making the change: https://github.com/ProjectOpenSea/seaport/blob/3b064ba270760eeda58f60d7d8dc7e14bd39bbaf/contracts/interfaces/ConduitControllerInterface.sol#L162

GalloDaSballo commented 2 years ago

Still disagree but giving a valid NC

HardlyDifficult commented 2 years ago

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.