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: 88/108
Findings: 1
Award: $21.30
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
21.2979 USDC - $21.30
Storage variables that are re-read in the same function can be cached to avoid expensive SLOAD and save gas
versionNFTCollection in
emit ImplementationNFTCollectionUpdated(_implementation, versionNFTCollection);
latestTokenId in
unchecked { // If +1 overflows then +count would also overflow, unless count==0 in which case the loop would exceed gas limits firstTokenId = latestTokenId + 1; } latestTokenId = latestTokenId + count; require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
Functions that are called multiple times in the same function can be cached and re-used to save gas
versionNFTCollection.toString() in
string.concat("NFT Collection Implementation v", versionNFTCollection.toString()), string.concat("NFTv", versionNFTCollection.toString())
versionNFTDropCollection.toString() in
string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()), string.concat("NFTDropV", versionNFTDropCollection.toString()),
In functions use arguments and local variables instead of storage variables in emitting events to avoid expensive storage read operations
baseURI,postRevealBaseURIHash can be replaced by function arguments
emit URIUpdated(baseURI, postRevealBaseURIHash);
nftContract, saleConfig.seller, saleConfig.price, saleConfig.limitPerAccount in
emit CreateFixedPriceSale(nftContract, saleConfig.seller, saleConfig.price, saleConfig.limitPerAccount);
Custom errors from solidity 0.8.4 are more efficient than revert strings with cheaper deployment and runtime time costs when revert condition is met
Refer: https://blog.soliditylang.org/2021/04/21/custom-errors/](https://blog.soliditylang.org/2021/04/21/custom-errors/
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
As EVM operates on 256 bits at a time, using a variable less than 256 bits costs more as additional operation has to be performed to reduce variable to the desired size
uint32 public versionNFTCollection;
uint32 public versionNFTDropCollection;
!= 0 instead of != 0 for unsigned integersUnsigned integers will never have value less than 0, so checking != 0 than > 0 costs less gas
require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
Array length can be cached and used in the loop condition instead of reading it in every iteration and save gas
for (uint256 i = 0; i < creatorRecipients.length; ++i)
for (uint256 i = 0; i < creatorShares.length; ++i) {
for (uint256 i = 0; i < creatorRecipients.length; ++i)
for (uint256 i = 1; i < creatorRecipients.length; )
for (uint256 i = firstTokenId; i <= latestTokenId; )
Constant variables can be declared internal to save gas as it can avoid creating a non-payable getter function and not adding another entry to the method ID table
bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
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.
require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required"); require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set"); require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
require(count != 0, "NFTDropCollection: `count` must be greater than 0");
require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared"); require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase"); require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
#0 - HardlyDifficult
2022-08-19T00:37:24Z
Cache storage values
Agree, we'll look at making changes here.
Cache function calls
Agree that could help, but we are not concerned with optimizing admin-only functions.
Use function arguments instead of storage variables in events
Agree, fixed
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.
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.
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
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.
constants expressions are expressions, not constants
While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 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.