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: 37/108
Findings: 3
Award: $74.99
馃専 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.1993 USDC - $41.20
It's a good practice to avoid the use of floating pragma. Code must be compiled with the same version it as been tested the most. It also avoids the use of any nightly builds which can have unexpected and unknown behaviors
4 instances:
Consider replacing ^0.8.12 by 0.8.12
Low risk because the tremendous majority of the time there is any risk.
_mint() is discouragedThe use of _safeMind() instead of _mint() can prevent tokens from being lost and is from a documentation point of view a better practice.
2 instances:
Consider replacing _mint() by _safemind().
to sent -> to send seems more right.
2 instances:
#0 - HardlyDifficult
2022-08-18T19:59:06Z
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.
Use safeMint
Agree will fix - for context see our response here.
[N-01 ] Typo
Agree, fixed.
馃専 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.6172 USDC - $20.62
!= 0 instead of > 0 for unsigned integers.length return a uint and _maxToken is a uint.
A uint can't be below zero, so != 0 is sufficient and is gas more efficient.
3 instances:
Consider changing > 0 to != 0 in these lines.
This optimization only work for solidity <0.8.13 which can be the case in ^0.8.12
These change also add consistency to the code since this optimization is already done on most require().
save 3 gas each
++x instead of x++The use of ++x is cheaper than x++ because the compiler does not have to create a temporary variable.
2 instances:
Consider changing
versionNFTCollection++ and versionNFTDropCollection++
to
++versionNFTCollection and ++versionNFTDropCollection
save 5 gas each
require() checks should be refactored to a modifier3 instances:
Consider adding a new modifier to check if an address is a contract. The returned string will be less explicit, which is not a problem since these functions are all external and payable with only one input. There is so no enough space for a wrong interpretation.
save deployment cost
1679538 -> 1663179 (-16359) Avg gas for NFTCollectionFactory deployment with the add of something like:
modifier isAContract(address _contract) { require(_contract.isContract(), "Is not a contract"); _; }
calldata instead of memory for read only argument in external functionIf a function parameter is read only, it is cheaper in gas to use calldata instead of memory.
2 instances:
Consider changing memory to calldata
Reverted string wich are longuer than 32 bytes require at least one additional mstore and so consume more gas than a shorter.
17 instances:
Consider shortening the revert strings to fit within 32 bytes, or using custom errors. Shortening the contract name and removing the auxiliary verb in the reverted string is most of the time sufficient to fit within 32 bytes.
Save deployment cost or runtime cost when the condition is met Deployment cost by replacing long revert string with a random 31 byte revert string (gas avg):
NFTCollection: 2860674 -> 2820633 (-40041)
NFTCollectionFactory: 1679538 -> 1638497 (-41041)
NFTDropCollection: 3372722 -> 3315827 (-56895)
#0 - HardlyDifficult
2022-08-17T19:03:39Z
[G-01] Use != 0 instead of > 0 for unsigned integers
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
[G-02] Use ++x instead of x++
Valid, will fix.
[G-03] Duplicated require() checks should be refactored to a modifier
Valid, however not planning on making this change at this time. We are not concerned with deployment cost or contract size here -- and it's rare enough that readability would not be impacted much.
[G-04] Using calldata instead of memory for read only argument in external function
Valid & will fix. This saves ~50 gas on createNFTCollection.
[G-05] Short reverted string can save gas
Agree but won't fix. We use up to 64 bytes, aiming to respect the incremental cost but 32 bytes is a bit too short to provide descriptive error messages for our users.