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: 39/108
Findings: 3
Award: $74.97
🌟 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.2005 USDC - $41.20
OpenZeppelin recommendation is to use _safeMint() instead of mint() for ERC721Upgradeable.
_safeMint() will ensure that the recipient implements IERC721Receiver or is an EOA.
File: contracts/NFTDropCollection.sol 182: _mint(to, i);
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol
OpenZeppelin recommends adding an empty constructor with the initializer modifier in order to avoid potential griefs, social engineering and other exploits.
File: contracts/NFTDropCollection.sol 62: contract NFTCollectionFactory is ICollectionFactory, Initializable, Gap10000 {
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol
File: contracts/NFTDropCollection.sol 28: contract NFTDropCollection is
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol
File: contracts/NFTDropMarket.sol 63: contract NFTDropMarket is
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropMarket.sol
File: contracts/mixins/collections/SequentialMintCollection.sol 13: abstract contract SequentialMintCollection is ITokenCreator, Initializable, ERC721BurnableUpgradeable {
File: contracts/mixins/roles/AdminRole.sol 12: abstract contract AdminRole is Initializable, AccessControlUpgradeable {
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol
File: contracts/mixins/treasury/AdminRole.sol 13: abstract contract AdminRole is Initializable, OZAccessControlUpgradeable {
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/treasury/AdminRole.sol
File: contracts/mixins/roles/MinterRole.sol 14: abstract contract MinterRole is Initializable, AccessControlUpgradeable, AdminRole {
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol
Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
Even if the idea is to make use of the latests versions of solidity, I would still recommend to lock the pragma for contracts that will be deployed to the mainnet and are not intended to be consumed by other developers.
All the 20 contracts in scope are rounding the pragma version.
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropMarket.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/AddressLibrary.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/ArrayLibrary.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/BytesLibrary.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/CollectionRoyalties.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/Constants.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/ContractFactory.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/FETHNode.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/FoundationTreasuryNode.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/Gap10000.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketSharedCore.sol https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/treasury/AdminRole.sol
File: contracts/NFTDropCollection.sol 159: function burn(uint256 tokenId) public override onlyAdmin {
https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol
File: contracts/mixins/shared/FoundationTreasuryNode.sol 59: function getFoundationTreasury() public view returns (address payable treasuryAddress) {
#0 - HardlyDifficult
2022-08-18T16:10:00Z
Use safeMint
Agree will fix - for context see our response here.
Use constructor to initialize templates
Agree this is a good best practice to add. Will fix.
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.
[NC-02] Replace public with external
Invalid. burn is an override so it must remain public. getFoundationTreasury is used internally by MarketFees.
🌟 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
Replacing require() strings with custom error would yield the following improvements on gas deployments (diff formatted for readability).
$ git diff --no-index gas-stories-original.txt gas-stories-with-custom-errors.txt diff --git a/gas-stories-original.txt b/gas-stories-with-custom-errors.txt index e2b107d..73442b8 100644 --- a/gas-stories-original.txt +++ b/gas-stories-with-custom-errors.txt @@ -1,6 +1,6 @@ Deployments - Avg -NFTCollection 2899976 -NFTDropCollection 3409650 + Avg +NFTCollection 2704389 +NFTDropCollection 3056772
There are 12 instances of this issue
File: contracts/NFTCollection.sol 158: require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required"); 263: require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); 264: require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted"); 268: require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted"); 327: require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
File: contracts/NFTDropCollection.sol 88: require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set"); 93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed"); 130: require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set"); 131: require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set"); 172: require(count != 0, "NFTDropCollection: `count` must be greater than 0"); 179: require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId"); 238: require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
#0 - HardlyDifficult
2022-08-19T15:32:38Z
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.