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: 83/108
Findings: 2
Award: $33.85
馃専 Selected for report: 0
馃殌 Solo Findings: 0
馃専 Selected for report: rbserver
Also found by: 0xc0ffEE, CodingNameKiki, Deivitto, Diraco, IllIllI, KIntern_NA, Lambda, Noah3o6, Treasure-Seeker, ignacio, oyc_109, zeesaw
-> _SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#:~:text=_mint(msg.sender%2C%20tokenId)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#:~:text=uint256%20tokenId)%20%7B-,tokenId%20%3D%20_mint(tokenCID)%3B,-%7D https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#:~:text=tokenId%20%3D-,_mint(tokenCID)%3B,-setApprovalForAll(operator%2C https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#:~:text=tokenId%20%3D-,_mint(tokenCID)%3B,-tokenIdToCreatorPaymentAddress%5BtokenId%5D
#0 - HardlyDifficult
2022-08-18T19:33:30Z
Use safeMint
Agree will fix - for context see our response here.
#1 - HickupHH3
2022-08-31T16:28:41Z
dup of #183
馃専 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
->X = X + Y IS CHEAPER THAN X += Y (ALSO X= X - Y IS CHEAPER THAN X -= Y)
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=not%20overflow%20here.-,creatorRev%20%2B%3D%20creatorShares%5Bi%5D%3B,-%7D https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=totalFees%20%2B%3D%20buyReferrerFee%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=%3B%20%2B%2Bi)%20%7B-,creatorRev%20%2B%3D%20creatorShares%5Bi%5D%3B,-%7D https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=totalShares%20%2B%3D%20creatorShares%5Bi%5D%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=totalRoyaltiesDistributed%20%2B%3D%20royalty%3B
-> REQUIRE()/REVERT() STRINGS LONGER THAN 32 BYTES COST EXTRA GAS
USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=%22NFTCollectionFactory%3A%20Caller%20does%20not%20have%20the%20Admin%20role%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=%22NFTCollectionFactory%3A%20RolesContract%20is%20not%20a%20contract%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=_implementation.isContract()%2C-,%22NFTCollectionFactory%3A%20Implementation%20is%20not%20a%20contract%22)%3B,-implementationNFTCollection%20%3D%20_implementation https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=%22NFTCollectionFactory%3A%20Symbol%20is%20required%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=%22NFTDropCollection%3A%20%60_baseURI%60%20must%20be%20set%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=%22NFTDropCollection%3A%20Already%20revealed%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#:~:text=%22NFTCollection%3A%20tokenCreatorPaymentAddress%20is%20required%22)%3B
-> ++i costs less gas compared to i++ or i += 1 (Also --i costs less gas compared to i--- or i -= 1)
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=versionNFTCollection%2B%2B%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#:~:text=versionNFTDropCollection%2B%2B%3B
-> USAGE OF UINTS/INTS SMALLER THAN 32 BYTES (256 BITS) INCURS OVERHEAD When using elements that are smaller than 32 bytes, your contract鈥檚 gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#:~:text=uint16%20limitPerAccount%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#:~:text=uint80%20price%2C-,uint16%20limitPerAccount,-)%20external%20%7B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#:~:text=address%20nftContract%2C-,uint16%20count%2C,-address%20payable%20buyReferrer
->IT COSTS MORE GAS TO INITIALIZE NON-CONSTANT/NON-IMMUTABLE VARIABLES TO ZERO THAN TO LET THE DEFAULT OF ZERO BE APPLIED
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=unchecked%20%7B-,for%20(uint256%20i%20%3D%200%3B%20i%20%3C%20creatorRecipients.length%3B%20%2B%2Bi)%20%7B,-_sendValueWithFallbackWithdraw( https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol#:~:text=unchecked%20%7B-,for%20(uint256%20i%20%3D%200%3B%20i%20%3C%20creatorRecipients.length%3B%20%2B%2Bi)%20%7B,-if%20(creatorShares%5Bi
->USING BOOLS FOR STORAGE INCURS OVERHEAD
->USING > 0 COSTS MORE GAS THAN != 0 WHEN USED ON A UINT IN A REQUIRE() STATEMENT
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=require(bytes,_%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=require(bytes(_symbol).length%20%3E%200%2C%20%22NFTDropCollection%3A%20%60_symbol%60%20must%20be%20set%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=require(_maxTokenId%20%3E%200%2C%20%22NFTDropCollection%3A%20%60_maxTokenId%60%20must%20be%20set%22)%3B
-> REQUIRE()/REVERT() STRINGS LONGER THAN 32 BYTES COST EXTRA GAS
USE CUSTOM ERRORS RATHER THAN REVERT()/REQUIRE() STRINGS TO SAVE GAS
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=%22NFTDropCollection%3A%20%60_baseURI%60%20must%20be%20set%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=%22NFTDropCollection%3A%20Already%20revealed%22)%3B https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#:~:text=%22NFTDropCollection%3A%20use%20%60reveal%60%20instead%22)%3B
#0 - HardlyDifficult
2022-08-17T19:50:54Z
->X = X + Y IS CHEAPER THAN X += Y
No impact. We made the recommended change and ran gas-reporter for our entire test suite. 0 impact detected. Even if there were a small benefit, we feel this hurts readability and would not be worth trivial savings.
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.
uints/ints smaller than 32 bytes (256 bits) incurs overhead.
Potentially valid but won't fix. This is uint32 to allow packing storage for a significant gas savings. Accepting the input as uint256 would incur additional overhead in checking for overflow before saving these values. Other inputs are logically limited to the size exposed, huge values cannot be used. Accepting the input as uint256 would incur additional overhead in checking for overflow before using these values.
Don't initialize variables with default values.
Invalid. This optimization technique is no longer applicable with the current version of Solidity.
Using bools for storage incurs overhead.
Valid for cidToMinted, saving ~200 gas.
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
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.