QA Report

[code423n4]

QA Report (low and non-critical bugs)

Multiple cancelation of an order

A user can cancel an order multiple times (it will emit the OrderCancelled event even though it was already cancelled). Maybe it should first check if the order is already cancelled.

Unhandled TODO comments

You left some TODO comments in the tests file (index.js) and in the BaseOrderTest test contract, which may contain stuff you need to do and haven't done.

Mistake in the comment

The comment in line 180 in the FulfillmentApplier contract says "Set the offerer as the recipient if execution amount is nonzero", however the code sets the offerer as the recipient if the execution amount is zero.

Wrong reference implementation

The ReferenceFulfillmentApplier contract doesn't revert if a non-matching item is being fulfilled, wether it is an offer or a consideration item.

In the _aggregateValidFulfillmentOfferItems function we can see the assignment of the return value of the _checkMatchingOffer function to the invalidFulfillment variable, but right after it the value in the invalidFulfillment variable is being overwritten in the beginning of the next iteration of the loop, without checking it is valid.

Similarly, in the _aggregateValidFulfillmentConsiderationItems function there's an assignment to the potentialCandidate.invalidFulfillment variable and right after it its value is overwritten by other value without checking that it is valid.

A fix to that issue will be to check if it is valid before overwriting it (and reverting if not), or adding a check that the value is valid to the loop condition, so it'll break if it is invalid and revert right after the loop.

[HardlyDifficult]

Merging with https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/114

[HardlyDifficult]

Multiple cancelation of an order

It does not seem harmful to allow orders to be canceled multiple times. However it may be worth considering to avoid confusion. It's pretty common for users to fire the same transaction multiple times, not realizing that there is a delay before the first transaction is mined.

Unhandled TODO comments

Since this is feedback on a test file, it does not seem important to address.

Mistake in the comment

Agree the comment should be fixed here.

Wrong reference implementation

The reference code linked does appear to be missing expected breaks (as used in other parts of the code).

Basic order fulfillment can get into a re-enterable state

See comments in https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/114