code423n4 commented 2 years ago

[QA-1][AmountDeriver.sol] The order of checking `if (!exact)` does not map 1:1 betwen reference and optimized one

In the optimized contract, the logic order of _getFraction is as follows:

[QA-2][ZoneInteraction.sol] Missing comment at `_callIsValidOrder` function

Throughout the codebase, function has comments for their arguments and the note for dev. However, _callIsValidOrder function does not have comments explaining their arguments or its role. https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/ZoneInteraction.sol#L56-L61

AdvancedOrder[] memory advancedOrders is defined at the function.

However, it also call return advancedOrders at the end of the function. It simply can remove return advancedOrders;. https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/OrderFulfiller.sol#L477-L478

These for loops are wrapped by unchecked.

However, ++i exists at the end of function.

// Increment counter inside body of loop for gas efficiency.
++i;

It simply can be located at the for loop definition instead.

for (uint256 i = 0; i < totalOrders; ++i) {

0xleastwood commented 2 years ago

This is not really an issue. The functions are equivalently the same, except the primary contract is optimised.

The rest are minor issues.

GalloDaSballo commented 2 years ago

It would still revert so disagree with the finding

Disagree that natspec has to be 100% for all params, all of natspec is optional

Disagree because the codebase is consistently using named returns with explicit returns

Disagree as the suggestion consumes more gas / misses the point

Agree with Invalid