OpenSea Seaport contest - csanuragjain's results

Incorrect documentation can cause confusion

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/interfaces/ConduitControllerInterface.sol#L105

Issue: Interface mentions that the last twenty bytes of the supplied conduit key must match the caller whereas implementation contract mentions that the first twenty bytes of the supplied conduit key must match the caller

Recommendation: Change ConduitControllerInterface.sol#L105 to mention first twenty bytes of the supplied conduit key must match the caller

Not verifying if create2 deployment failed

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/ConduitController.sol#L91

Issue: if deployment fails conduit will be address 0 and since codehash will not match, contract will assume that conduit has been created and will update _conduits[0x00]

Recommendation: Add below check to createConduit

address conduitAddr=new Conduit{ salt: conduitKey }();
require(conduitAddr!=address(0), "Conduit creation failed");

offerer/zone should not be address(0)

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/reference/lib/ReferenceOrderFulfiller.sol#L151

Issue: While fulfilling orders there is no check to verify zero address check for offerer/zone (In case order type is open zero address for both would be accepted)

Recommendation: Add below check to _applyFractionsAndTransferEach

require(offerer!=address(0) && zone!=address(0),"Incorrect offerer or zone");

Conduit address can be computed using predefined function

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/ConduitController.sol#L314

Issue: Create2 exposes a computeAddress function which can be used to calculate the address of Conduit but Conduit instead uses a homebrew calculation in getConduit.

Recommendation: Make use of Create2.computeAddress function to determine conduit address in getConduit function

Offer & Consideration length should be greater than 0 while fulfilling orders

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderFulfiller.sol#L75 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/reference/lib/ReferenceOrderFulfiller.sol#L80

Issue: If Offer & Consideration length is zero then Order fulfillment does not need to do anything and should return immediately to prevent Gas

Recommendation: Have below check added in _validateAndFulfillAdvancedOrder function

require(orderParameters.offer.length>0 && orderParameters.consideration.length>0 , "Incorrect paramters");

Criteria resolver should be skipped if order is not fulfilled

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/reference/lib/ReferenceCriteriaResolution.sol#L210

Issue: If order is not fulfilled then there is no need to run criteria resolver

Recommendation: Add below check in _applyCriteriaResolversAdvanced

if (advancedOrder.numerator == 0) {
                    return;
                }

Existing Conduit check can be done with lesser gas

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/ConduitController.sol#L85

Issue: conduit.codehash == _CONDUIT_RUNTIME_CODE_HASH is currently used to know if conduit is already existing or not. The same could be done in gas saving way using bytes32(0) check

Recommendation: Replace check with below in createConduit:

if(!_conduits[conduit].key == bytes32(0)){
revert ConduitAlreadyExists(conduit);
}

Insert is not required when Conduit is not used

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Executor.sol#L260

Issue: If conduitKey == bytes32(0) then there is no need to check _triggerIfArmedAndNotAccumulatable since there will not be any new transfer insert.

Recommendation: The recommendation should be applied for all transfers. Change the function implementation like below:

    function _transferERC20(
        address token,
        address from,
        address to,
        uint256 amount,
        bytes32 conduitKey,
        bytes memory accumulator
    ) internal {

        _assertNonZeroAmount(amount);

        if (conduitKey == bytes32(0)) {

            _performERC20Transfer(token, from, to, amount);
        } else {

// Add trigger here
_triggerIfArmedAndNotAccumulatable(accumulator, conduitKey);

            _insert(
                conduitKey,
                accumulator,
                uint256(1),
                token,
                from,
                to,
                uint256(0),
                amount
            );
        }
    }

Transfer not required if amount is 0

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L174

Issue: If amount is not correct then there is no need for transfer to proceed

Recommendation: Add below check in _transfer function:

require(item.amount!=0, "Invalid amount")

No need to update channel if same state

Contract: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/conduit/Conduit.sol#L163

Issue: Add a check to see if channel updating is actually not required as this will return ConduitController.sol#L118 before it does unrequired updation and will save gas

Recommendation: Add below check if updating is not required:

require(_channels[channel]!=isOpen, "Already in same state");