Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #104

Open
code423n4 opened this issue Jun 3, 2022 · 1 comment
Open

QA Report #104

code423n4 opened this issue Jun 3, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax

Comments

@code423n4
Copy link
Contributor

Seaport QA report (Low/Non-critical)

Summary:
The code base is well structured and well documented. Although assembly is heavily utilized to optimize the gas spending, it also contains the reference files to compare and test on.
One concern going forward is to modify and update the code. One should have a deep understanding to be able to safely change things. After apply any change, it should be thoroughly reviewed and tested.

Low

lack of owner address check in the createConduit

  • contracts/conduit/ConduitController.sol:94 github link
  • function createConduit
        _conduits[conduit].owner = initialOwner;

Upon create conduit, createConduit function does not ensure whether the owner address is valid and accessible, such as zero address check. Presumably setting an address one does not have an access is not intended use, since transferring ownership is a two-step process of nominating and accepting. Upon nominating ownership with transferOwnership, zero address is checked for the potential owner (in ConduitController.sol:197)
When the user creates a conduit with either zero address or a random address as the owner of the conduit by accident, the conduit is basically useless. The conduit cannot be updated to add channels, nor the ownership can be transferred.

Non-critical

misleading comment in GettersAndDerivers.sol

  • contracts/lib/GettersAndDerivers.sol:131 github link
  • function _deriveOrderHash
            // Iterate over the offer items (not including tips).
            // prettier-ignore
            for { let i := 0 } lt(i, originalConsiderationLength) {
                i := add(i, 1)
            } {

This is iterating over the consideration, but the comment says Iterate over the offer items

misleading comment in FulfillmentApplier.sol

  • contracts/lib/FulfillmentApplier.sol:188 github link
  • function _aggregateValidFulfillmentOfferItems
     * @dev Internal pure function to aggregate a group of offer items using
     *      supplied directives on which component items are candidates for
     *      aggregation, skipping items on orders that are not available.
     *
     * @param advancedOrders  The orders to aggregate offer items from.
     * @param offerComponents An array of FulfillmentComponent structs
     *                        indicating the order index and item index of each
     *                        candidate offer item for aggregation.
     * @param execution       The execution to apply the aggregation to.
     */
    function _aggregateValidFulfillmentOfferItems(
        AdvancedOrder[] memory advancedOrders,
        FulfillmentComponent[] memory offerComponents,
        Execution memory execution
    ) internal view {

The comment in the line 188 Internal pure function does not match with the function's actual modifier internal view (line 202).
The function _aggregateValidFulfillmentOfferItems is not pure as it uses caller() in the line 297.

misleading comment in ReferenceFulfillmentApplier.sol

  • reference/lib/ReferenceFulfillmentApplier.sol:252 github link
  • function _aggregateValidFulfillmentOfferItems
     * @dev Internal pure function to aggregate a group of offer items using
     *      supplied directives on which component items are candidates for
     *      aggregation, skipping items on orders that are not available.
     *
     * @param ordersToExecute The orders to aggregate offer items from.
     * @param offerComponents An array of FulfillmentComponent structs
     *                        indicating the order index and item index of each
     *                        candidate offer item for aggregation.
     * @param startIndex      The initial order index to begin iteration on when
     *                        searching for offer items to aggregate.
     *
     * @return execution The aggregated offer items.
     */
    function _aggregateValidFulfillmentOfferItems(
        OrderToExecute[] memory ordersToExecute,
        FulfillmentComponent[] memory offerComponents,
        uint256 startIndex
    ) internal view returns (Execution memory execution) {

The comment in the line 252 Internal pure function does not match the function's actual modifier internal view in the line 269.
The function _aggregateValidFulfillmentOfferItems is not pure as it uses msg.sender in the line 296.

missing Natspec in function _callIsValideOrder

  • contracts/lib/ZoneInteraction.sol:56 github link
  • function _callIsValidOrder is missing Natspec

typo in Assertions.sol

     * @dev Internal view function to to ensure that the supplied consideration

to is duplicated.
function to to ensure -> function to ensure

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@0xleastwood 0xleastwood added the invalid This doesn't seem right label Jun 24, 2022
@HardlyDifficult HardlyDifficult removed the invalid This doesn't seem right label Jul 6, 2022
@code-423n4 code-423n4 deleted a comment from 0xleastwood Jul 6, 2022
@GalloDaSballo
Copy link

lack of owner address check in the createConduit

Valid Low per #56

Typo /Comments

1 NC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
Projects
None yet
Development

No branches or pull requests

4 participants