Platform: Code4rena
Start Date: 11/08/2022
Pot Size: $40,000 USDC
Total HM: 8
Participants: 108
Period: 4 days
Judge: hickuphh3
Total Solo HM: 2
Id: 152
League: ETH
Rank: 57/108
Findings: 2
Award: $61.80
馃専 Selected for report: 0
馃殌 Solo Findings: 0
馃専 Selected for report: Saw-mon_and_Natalie
Also found by: 0x1f8b, 0x52, 0xDjango, 0xNazgul, 0xSmartContract, 0xSolus, 0xackermann, 0xmatt, 0xsolstars, Aymen0909, Bnke0x0, Chom, Deivitto, DevABDee, Dravee, ElKu, IllIllI, JC, Kumpa, Lambda, LeoS, MiloTruck, PwnedNoMore, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Treasure-Seeker, Vexjon, Waze, Yiko, __141345__, apostle0x01, auditor0517, berndartmueller, bin2chen, bobirichman, brgltd, bulej93, c3phas, carlitox477, cccz, cryptphi, csanuragjain, d3e4, danb, delfin454000, durianSausage, erictee, fatherOfBlocks, gogo, iamwhitelights, joestakey, jonatascm, ladboy233, mics, oyc_109, rbserver, ret2basic, robee, rokinot, rvierdiiev, shenwilly, sikorico, simon135, thank_you, wagmi, yash90, zeesaw, zkhorse
41.1998 USDC - $41.20
The code is compiled with more than one solidity version which can cause undesired behavior.
For readability purposes consider having one of the two return options (for the following functions)
#0 - HardlyDifficult
2022-08-18T20:26:28Z
[QA 00] Timelock is missing for the following functions
Invalid: this is a test file.
[QA 01] Mismatched solidity versions
Invalid - all files are compiled with 0.8.16
[QA 02] Use safeTransfer instead transfer in the following locations
Out of scope for this contest.
[QA 03] Magic number, consider using named constant instead.
FETH.sol#L209 Out of scope.
OZERC165Checker.sol#L34 OZERC165Checker.sol#L33 This file is a slightly modified clone from OpenZeppelin, we are aiming to minimize changes in this file.
FixedPriceDrop.sol#L39
We believe 24 hours is already pretty clear, a constant is not required.
Use fixed pragma
Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.
Missing indexed event parameters
I believe this is invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.
[QA 06] Remove the name from the following unused function parameters
Invalid, these blocks are required to avoid compile errors.
[QA 07] Not emitted event for state changes
FoundationTreasuryNode.sol#L47 We don't feel that events are required in a constructor.
FixedPriceDrop.sol#L27 This is a test file.
ContractFactory.sol#L30 We don't feel that events are required in a constructor.
Use named returns consistently
Agree, we have opted to use the named returns instead of return ... This is more consistent with other code in our repo and saves a bit of on the contract size. We also like named returns as a way of improving natspec, and typechain (particularly when a tuple is returned).
馃専 Selected for report: Dravee
Also found by: 0x040, 0x1f8b, 0xDjango, 0xHarry, 0xNazgul, 0xSmartContract, 0xbepresent, 0xkatana, Amithuddar, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, DevABDee, Diraco, ElKu, Fitraldys, Funen, IllIllI, JC, LeoS, Metatron, MiloTruck, Noah3o6, ReyAdmirado, Rohan16, Rolezn, Saw-mon_and_Natalie, Sm4rty, SpaceCake, TomJ, Tomio, Trabajo_de_mates, Waze, Yiko, __141345__, ajtra, apostle0x01, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, d3e4, durianSausage, erictee, fatherOfBlocks, gerdusx, gogo, hakerbaya, ignacio, jag, joestakey, ladboy233, medikko, mics, newfork01, oyc_109, pfapostol, robee, rvierdiiev, sach1r0, saian, samruna, sikorico, simon135, wagmi, zeesaw, zkhorse, zuhaibmohd
20.5997 USDC - $20.60
> 0 instead != 0 to check if an unsigned int is not 0#0 - HardlyDifficult
2022-08-17T18:55:17Z
[GAS 00] Use custom error instead string error for the following require statements
Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.
[GAS 01] Unnecessary caching of msg.sender
Invalid. Neither of the functions linked even use msg.sender
[GAS 02] abiEncodePacked() instead abiEncode() in the following locations
Invalid... while it seems like this should help, changing to .encode makes creating collections slightly more expensive. createNFTCollection increased by 9 gas and createNFTDropCollection increased by 8-10 gas
[GAS 03] Use > 0 instead != 0 to check if an unsigned int is not 0
Invalid. We tested the recommendation and got the following results:
createNFTDropCollection gas reporter results: using > 0 (current): - 319246 路 319578 路 319361 using != 0 (recommendation): - 319252 路 319584 路 319367 impact: +6 gas
[GAS 04] Cache the array size for the following loops over array
Invalid - the contract referenced here was labeled as out of scope.