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: 85/108
Findings: 2
Award: $33.77
🌟 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
13.1705 USDC - $13.17
contracts/NFTCollectionFactory.sol 193: versionNFTCollection = _versionNFTCollection;
_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function
contracts/NFTDropCollection.sol 182: _mint(to, i);
#0 - HardlyDifficult
2022-08-18T21:30:03Z
Missing checks for address(0x0) when assigning values to address state variables
Invalid. That's a number not an address, and 0 is an acceptable value there.
Use safeMint
Agree will fix - for context see our response here.
#1 - HickupHH3
2022-08-31T11:05:44Z
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.6049 USDC - $20.60
contracts/NFTCollectionFactory.sol 173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
contracts/NFTDropCollection.sol
88: require(bytes(_baseURI).length > 0, "NFTDropCollection: _baseURI must be set");
93: require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
contracts/NFTCollectionFactory.sol 80: uint32 public versionNFTCollection; 95: uint32 public versionNFTDropCollection; 192: function initialize(uint32 _versionNFTCollection) external initializer { 291: uint32 maxTokenId, 329: uint32 maxTokenId, 368: uint32 maxTokenId, 391: uint32 maxTokenId,
contracts/NFTDropCollection.sol 126: uint32 _maxTokenId, 220: function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
contracts/NFTCollectionFactory.sol 173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); 182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); 203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
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");
1.Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. 2.Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc. 3.In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:
contracts/NFTCollectionFactory.sol 173: require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role"); 182: require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract"); 203: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 227: require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract"); 262: require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
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");
From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks. In some instances, an overflow/underflow is impossible, gas can be saved by using an unchecked block to remove the implicit checks:
contracts/NFTCollectionFactory.sol (Before) 207: versionNFTCollection++; (After) 207: unchecked { versionNFTCollection++ };
(Before) 231: versionNFTDropCollection++; (After) 231: unchecked { versionNFTDropCollection++ }:
If data can fit into 32 bytes, then you should use bytes32 datatype rather than bytes or strings as it is cheaper in solidity. Basically, any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract. instances of string can be replaced by bytes(1..32):
contracts/NFTCollectionFactory.sol 240: string.concat("NFTDropV", versionNFTDropCollection.toString()), 214: string.concat("NFTv", versionNFTCollection.toString())
contracts/NFTDropCollection.sol 303: return string.concat(baseURI, tokenId.toString(), ".json");
contracts/NFTCollectionFactory.sol 172: modifier onlyAdmin() { 202: function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin { 226: function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
#0 - HardlyDifficult
2022-08-17T09:17:17Z
- 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.
- Usage of uints smaller than 32 bytes (256 bits) incurs overhead
contracts/NFTCollectionFactory.sol 80: uint32 public versionNFTCollection; 95: uint32 public versionNFTDropCollection; 192: function initialize(uint32 _versionNFTCollection) external initializer {
Invalid. The benefit from packing storage outweighs the casting overhead. Compressing these allows for a warm sload instead of 2 cold sloads while deploying new collections.
291: uint32 maxTokenId, 329: uint32 maxTokenId, 368: uint32 maxTokenId, 391: uint32 maxTokenId, contracts/NFTDropCollection.sol 126: uint32 _maxTokenId, 220: function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {
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.
3 & 4. 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.
- Unchecking arithmetic operations that cannot underflow/overflow
Invalid. The examples provided are already unchecked.
- Use bytes32 instead of string
Invalid. The baseURI cannot be stored in bytes32 because it may be longer than 32 bytes. It's possible this technique could work for name or symbol, however in order to stay flexible in case a creator would like to use a long value here -- we will keep these as strings.
- Functions guaranteed to revert when called by normal users can be marked payable
Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.