QA Report

[code423n4]

See the markdown file with the details of this report here.

[c4-judge]

Picodes marked the issue as grade-a

[0xhiroshi]

[L-01] The length of the array arguments in the execute function is not checked Invalid. As long as the ERC20 token transfers amounts match the total required ERC20 token amounts for trades we are good. Each TokenTransfer is an aggregate.

[L-02] Every token accidentally sent to the Contract cannot be recovered Invalid (technically not our responsibility if tokens are sent accidentally).

[L-03] Remove unused code Invalid (general lib).

[L-04] Some ERC20 tokens should need to approve(0) first Invalid, we will just approve(0) first.

[L-05] Insufficient coverage V0 is out of scope. TokenReceiver onERC1155BatchReceived cannot be covered until we support a marketplace that supports multiple ERC1155 orders.

[L-06] _transferETH function create dirty bits Invalid, CALL discards the upper 96 bits.

[L-07] Critical Address Changes Should Use Two-step Procedure Invalid, worst case we just deploy another contract for ERC20EnabledLooksRareAggregator / call setFee again.

[L-08] Owner can renounce Ownership Invalid, not really an issue.

[L-09] Need Fuzzing test Invalid, they are all just ++i/++j. There won't pose real issues.

[L-10] Lock pragmas to specific compiler version Invalid, those files are shared between different projects.

[L-11] Loss of precision due to rounding Invalid, realistically the minimum NFT value will be 0.001 ETH and that's 1e15. For USDC even if it's only worth 1 USDC that's 1e6.

[L-12] The transferERC1155 function has a reentrancy vulnerability Invalid, it is an owner only function. We will only withdraw to addresses we know.

[NC-01] Add I prefix to interface contracts Will not fix. We copy Seaport files as is.

[NC-02] Not using the latest version of solmate from dependencies I tried and it seems like the latest from npm is 6.6.1

? Please choose a version of "solmate" from this list: ❯ 6.6.1 6.6.0 6.5.0

[NC-03] 0 address check Invalid. We have total control over the addresses.

[NC-04] Omissions in Events Acknowledged but we do not think it is necessary in this case, also we are dropping fees completely.

[NC-05] Add parameter to Event-Emit ERC20EnabledLooksRareAggregatorSet removed. We simply do not need it.

[NC-06] Include return parameters in NatSpec comments Valid.

[NC-07] Use a more recent version of Solidity @0xJurassicPunk maybe we should do ^0.8.17 for contracts-libs?

[NC-08] Solidity compiler optimizations can be problematic Acknowledged.

[NC-09] NatSpec is missing Not planning to have Natspec for internal/private functions. For LooksRareAggregator, added. For TokenReceiver, will add.

[NC-10] Unnecessary receive() Invalid, we need it to receive refunds.

[NC-11] Signature Malleability of EVM's ecrecover() 1 @0xJurassicPunk leave this to you.

[NC-12] Lines are too long Fixed.

[NC-13] Stop using v != 27 && v != 28 or v == 27 v == 28 @0xJurassicPunk leave this to you.

[NC-14] Use a more recent version of Seaport dependencies Actually we don't need Seaport.js anymore, uninstalled.

[NC-15] Empty blocks should be removed or Emit something Removed receive for SeaportProxy but for LooksRareAggregator it is still needed.

[NC-16] Use Underscores for Number Literals Valid.

[S-01] Generate perfect code headers every time Thanks, will consider.

[c4-sponsor]

0xhiroshi requested judge review