search

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

1 stars 0 forks source link

QA Report #142

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

QA Report

Table of Contents

summary

Only non-critical vulnerabilities were found examining the contracts.

Comment Missing function parameter or return

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ConduitController.sol

bytes32 creationCodeHash, bytes32 runtimeCodeHash

BasicOrderFulfiller.sol

bytes memory accumulator has a comment but it is missing @param accumulator

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for the parameters and returns missing.

Commented code

PROBLEM

There are portions of commented code in some files.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ConsiderationConstants.sol

uint256 constant BasicOrder_considerationIdentifier_cdPtr = 0x44\ uint256 constant BasicOrder_offerIdentifier_cdPtr = 0xe4\ uint256 constant BasicOrder_endTime_cdPtr = 0x164\ uint256 constant BasicOrder_zoneHash_cdPtr = 0x184\ uint256 constant BasicOrder_salt_cdPtr = 0x1a4\ uint256 constant BasicOrder_considerationItem_recipient_ptr = 0x140\ uint256 constant BasicOrder_offerItem_identifier_ptr = 0xe0\ uint256 constant BasicOrder_offerItem_startAmount_ptr = 0x100\ uint256 constant BasicOrder_order_zone_ptr = 0xc0\ uint256 constant BasicOrder_order_endTime_ptr = 0x160\ uint256 constant BasicOrder_order_zoneHash_ptr = 0x180\ uint256 constant BasicOrder_order_salt_ptr = 0x1a0\ uint256 constant BasicOrder_order_conduitKey_ptr = 0x1c0

TOOLS USED

Manual Analysis

MITIGATION

Remove commented code

Events emitted early

PROBLEM

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ConduitController.sol

PotentialOwnerUpdated is emitted before the new potential owner is written as a value of _conduits[conduit].potentialOwner

PotentialOwnerUpdated is emitted before the the current new potential owner from the conduit is cleared.

PotentialOwnerUpdated is emitted before the the current new potential owner from the conduit is cleared.

OwnershipTransferred is emitted before the new owner is written as a value of _conduits[conduit].owner

TOOLS USED

Manual Analysis

MITIGATION

Place the event emission in the last position in the function.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ZoneInteraction.sol

function _callIsValidOrder

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Non-library files should use fixed compiler versions

PROBLEM

contracts should be compiled using a fixed compiler version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

ZoneInteraction.sol

Conduit.sol\ ConduitController.sol

TOOLS USED

Manual Analysis

MITIGATION

Used a fixed compiler version

Non-library files should use the same compiler version

PROBLEM

contracts within the scope should be compiled using the same compiler version.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Conduit.sol and ConduitController.sol have the compiler version set to >=0.8.7, while Seaport.sol has 0.8.13

TOOLS USED

Manual Analysis

MITIGATION

Use the same compiler version throughout the contracts

HardlyDifficult commented 2 years ago

Some of this feedback does not seem valid. Most the rest is about missing comments or otherwise not helpful. Closing this as invalid.

GalloDaSballo commented 1 year ago

Some of the function comments are missing function parameters or returns

Disagree that natspec needs to be for every parameter as it's optional

There are portions of commented code in some files.

Seems like the example provided is a comment about the code to document the memory-offsets

It is not recommended to emit events before the end of the computations, as the function might revert based on conditions ahead of the event emission

Disagree as this was prob done due to how slither reports false-positives

Some functions are missing comments.

Disagree per 1)

Fixed pragmas

Disagree per discussion on #67

GalloDaSballo commented 1 year ago

1NC per updated discussion on #67