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: 63/108
Findings: 2
Award: $58.25
🌟 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
45.0803 USDC - $45.08
Overall, the code base was quite well written in my opinion and had great test coverage. The MarketFees mixin logic was a bit trickier to follow if I were to give any constructive feedback.
OpenZeppelin recommends the usage of _safeMint() instead of _mint() since safeMint() checks whether a contract can handle ERC721 tokens. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2dc086563f2e51620ebc43d2237fc10ef201c4e6/contracts/token/ERC721/ERC721.sol#L270
If the user provides an address that can't handle ERC721 tokens when calling any of the corresponding mint functions the minted token might be lost which could potentially result in the user not being able to redeem the nft anymore.
In several places:
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L143 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L159 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L271 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L182
This was previously reported and confirmed by HardlyDifficult (sponsor) and has also been reported in C4 before. https://github.com/code-423n4/2022-02-foundation-findings/issues/50 https://github.com/code-423n4/2021-11-vader-findings/issues/27
Inconsistent check on requiring a symbol to be passed when creating a collection, confirmed by Hardly Difficult (sponsor).
There is a check here https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L262 but no check here for example: https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L295. The check does not happen until inside initialize: https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L130
Would recommend consistency on the require check for better readability/developer experience.
#0 - HardlyDifficult
2022-08-18T16:32:04Z
Use safeMint
Agree will fix - for context see our response here.
Inconsistent symbol check
Agree that we were inconsistent with these checks. We have moved the NFTDropCollection requirement into the factory so that both collection types are implemented in a similar fashion, and we went with the factory instead of the collection init in order to follow the best practice of fail fast.