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: 54/108
Findings: 2
Award: $61.89
🌟 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.2059 USDC - $41.21
#1 Missing natspec comment supply
A function has a natspec comment to explain utility about function or parameter but natspec comment supply is missing. So i suggest to add natspec comment for parameter supply.
#2 Missing natspec comment treasury
A function has a natspec comment to explain utility about function or parameter but natspec comment treasury is missing. So i suggest to add natspec comment for parameter treasury.
#3 Missing natspec comment all param in distributeFunds()
A function has a natspec comment to explain utility about function or parameter but all natspec comment params is missing. So i suggest to add natspec comment for all parameter.
#4 Missing natspec comment all param in getFees()
A function has a natspec comment to explain utility about function or parameter but all natspec comment params is missing. So i suggest to add natspec comment for all parameter.
#5 Missing indexed field
Each event should use three indexed fields if there are three or more fields. add indexed in buyReferrer..
#6 Similiar name function()
there have two same function with similiar param, so we suggest to choose one of the function anda remove unused one.
#7 Remove unused code
remove all unused code in contract before deploy the contract to increase readibility and saving gas fee.
#8 Unnecessary unchecked for 0.8.0 above
According to Solidity documentation: Prior to Solidity 0.8.0, arithmetic operations would always wrap in case of under- or overflow leading to widespread use of libraries that introduce additional checks. Since Solidity 0.8.0, all arithmetic operations revert on over- and underflow by default, thus making the use of these libraries unnecessary.
#0 - HardlyDifficult
2022-08-18T17:43:45Z
Missing natspecs
Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.
BuyReferralPaid event should index buyReferrer
Agree but won't fix at this time. We have already started emitting this event on mainnet, to simplify integrations such as subgraph we are going to continue with this event as is. However it's a good catch and I agree that buyReferrer ideally would have been indexed here.
Similiar name function()
Fair feedback but going to leave as is. These are library functions which do the same thing, just operating on different types.
Remove unused code
Invalid - these instances have those params due to inheritance requirements.
Unnecessary unchecked for 0.8.0 above
Invalid. Unchecked is a gas optimization that can be used when math is known to be safe.
🌟 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.6764 USDC - $20.68
#1 Visibility
Reducing from public to private or internal can save gas when a constant isn’t used outside of its contract. I suggest changing the visibility from public to internal or private.
#2 Looping
default uint is 0 so remove unnecassary explicit can reduce gas. caching the array length can reduce gas it caused access to a local variable is more cheap than query storage / calldata / memory in solidity.
#3 use calldata instead of memory
When arguments are read-only on external functions, the function() has an inputed parameter that using memory, if this function didnt change the parameter, its cheaper to use calldata then memory. so we suggest to change it.
#4 Use storage instead memory
Use storage instead of memory to reduce the gas fee. i suggest to change this.
#5 Cache recipients.length
cache the recipients.length to the local too for saving the gas fee. because mload is cheaper than sload.
#6 Cache recipientBasisPoints.length
cache the recipientBasisPoints.length to the local too for saving the gas fee. because mload is cheaper than sload.
#7 Cache creatorRecipients.length https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L432-L465
cache the creatorRecipients.length to the local too for saving the gas fee. because mload is cheaper than sload.
#8 Reduce string
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
#9 Use custom error instead revert string to save gas https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L173
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
#10 Increment
pre increment e.g ++i more cheaper gas than post increment e.g i++. i suggest to use pre increment.
#11 Use != 0 instead >0
!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)
#0 - HardlyDifficult
2022-08-19T16:12:07Z
Using private rather than public for constants to saves gas.
Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.
Don't initialize variables with default values.
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
calldata
Valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactory
Updated startsWith, NFTCollection, and the AddressLibrary & call
Invalid for capLength due to how that data is sourced. Same for replaceAtIf.
Use storage instead memory
For the encode examples this is invalid, storage cannot be used. For mint, we tested and found this caused a small regression - maybe because all fields are being used here. For the return values this is invalid, storage cannot be used.
Cache Array Length Outside of Loop
May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.
Cache recipientBasisPoints.length
Cache creatorRecipients.length
Little to no benefit. And the comment is invalid, these are not sloads.
Use short error messages
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.
Custom errors
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.
++i costs less than i++
Agree and will fix.
Use != 0 instead of > 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