QA Report

[code423n4]

Summary of Findings for Low / Non-Critical issues

LOW

• L-01 : Transaction reverted without a reason string, if invalid OrderType given

Non-Critical

• NC-01 : wrong dev comments in function _assertRestrictedAdvancedOrderValidity
• NC-02 : ConduitController can transfer ownership to the same owner

Details L-01

Title : Transaction reverted without a reason string, if invalid OrderType given

In the event the OrderType field is wrongly set to an invalid value like more than 3 , then the transaction will revert without a reason string.
Sample transaction output with a value set to 8

Transaction sent: 0xb0bf0b246a12051f24bc6413d7c6258c823d3a0bbeddf95febc0b126f9c148e6
Max fee: 2.076398196 gwei Priority fee: 2.0 gwei Gas limit: 30000000 Nonce: 4
Transaction confirmed (Transaction reverted without a reason string) Block: 27 Gas used: 31603 (0.11%) Gas price: 2.033439549 gwei

Impact

Debuggability will be an issue if no reason string is received

Recommended Mitigation Steps

Add a new Error String for invalid OrderType field , and check for the OrderType

Details NC-01

Title : wrong dev comments in function _assertRestrictedAdvancedOrderValidity

Proof of Concept

Contract : ZoneInteraction.sol
Line : 120

            // If no extraData or criteria resolvers are supplied...
            if (
                advancedOrder.extraData.length == 0 &&
                criteriaResolvers.length == 0
            ) {

Recommended Mitigation Steps

The comments for this 'if block' should be 'and' instead of 'or' as below

// If no extraData and criteria resolvers are supplied...

or can be still better worded as

// If both extraData and criteria resolvers are not supplied...

Details NC-02

Title : ConduitController can transfer ownership to the same owner

Impact

Unnecessary transaction

Proof of Concept

Contract : ConduitController
Function : transferOwnership
https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/conduit/ConduitController.sol#L189

Recommended Mitigation Steps

    error AlreadyOwner(address conduit);

    if (newPotentialOwner == _conduits[conduit].owner) {
        revert AlreadyOwner(conduit);
    }

[GalloDaSballo]

Details NC-01

Valid NC

Rest I disagree with

L-01 -> No Code, No POC
NC-02 -> That increases gas cost to no benefit

[KenzoAgada]

NC-02 "ConduitController can transfer ownership to the same owner" was considered valid low on #90

[GalloDaSballo]

Re-valued to NC

NC-02 -> Valid NC

2NC