Platform: Code4rena
Start Date: 13/01/2023
Pot Size: $100,500 USDC
Total HM: 1
Participants: 23
Period: 10 days
Judge: hickuphh3
Total Solo HM: 1
Id: 201
League: ETH
Rank: 5/23
Findings: 1
Award: $1,551.45
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: horsefacts
Also found by: 0xSmartContract, ABA, Chom, IllIllI, Josiah, RaymondFam, Rickard, Rolezn, brgltd, btk, chaduke, charlesjhongc, csanuragjain, delfin454000, nadin, oyc_109
1551.4544 USDC - $1,551.45
BasicOrderFulfiller._validateAndFulfillBasicOrder()_validateAndFulfillBasicOrder() will make different transfers depending on the ItemType. For transfers where additionalRecipientsItemType is not ItemType.NATIVE (transfers between ERC20, ERC721 and ERC1155), an improvement that can be made is to check if msg.value is zero.
Otherwise, if ether is sent accidentally during these transfer, the msg.value will be locked.
Downcasting in solidity can lead to silent overflows. The ideal approach would be to use a safe wrapper that reverts if the number being casted is bigger than the expected range. Alternative, for each one of the values being downcasted, if the idea is to be as much gas efficient as possible and an overflow cannot happen, consider adding a comment for clarity.
Setting _channels[address(0)] to true can result in unexpected behavior for the project. Consider adding an address(0) check for the channel parameter.
To prevent hash collisions, abi.encode is generally preferred over abi.encodePacked . If there's a compelling reason to use abi.encodePacked, consider adding a comment.
Creating a modifier for repeated validation logic can improve code reusability and gas usage. Also, aside from reverting if the conduit doens't exist, the function ConduitController._assertConduitExists() could return true indicating when it exists.
The solidity documentation recommends adding pure functions bellow view functions.
The instances bellow highlight pure above view.
Naming all the used imports can improve the explicitness of the code
Some functions return named variables, others return explicit values.
Following function returns an explicit value.
Following function return returns a named variable.
Consider adopting the same approach throughout the codebase to improve the explicitness and readability of the code.
If possible, consider importing/reusing the same constant to improve code reusability.
The usual approach for defining constants in solidity and other programming languages is in an all uppercase format, e.g. MY_CONSTANT instead of MyConstant.
It's understandable if Seaport wants to maintain the current approach, since it's following consistently throughout all constant variables. However, please note that most common linters will complain about not defining constant in all uppercase.
Instead of declaring an empty line for each empty block for the loop inside assembly blocks, a more concise approach can be to declare the empty block without the empty line.
E.g. replacing
for { } lt(value1, value2) { } {
With the following
for {} lt(value1, value2) {} {
#0 - HickupHH3
2023-01-25T09:35:46Z
01 - Invalid. Said missing check is performed:
assembly { // route 0 and 1 are payable, otherwise route is not payable. correctPayableStatus := eq( additionalRecipientsItemType, iszero(callvalue()) ) } // Revert if msg.value has not been supplied as part of payable // routes or has been supplied as part of non-payable routes. if (!correctPayableStatus) { _revertInvalidMsgValue(msg.value); }
02 - Lacking sufficient justification on why the downcasts are unsafe. NC
 03 - NC
 04 - NC
 05 - R
 06 - NC 07 - NC 
08 - R
 
09 - NC 

10 - NC 


11 - NC
#1 - HickupHH3
2023-01-25T09:36:21Z
8 NCs + 2 Rs = 12 pts
#2 - HickupHH3
2023-01-25T09:36:43Z
Potentially grade A once moderation is applied.
#3 - c4-judge
2023-01-26T03:02:24Z
HickupHH3 marked the issue as grade-a