Foundation Drop contest - gogo's results

Foundation is a web3 destination.

General Information

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

Foundation

Findings Distribution

Researcher Performance

Rank: 44/108

Findings: 2

Award: $64.54

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Foundation

Findings Information

🌟 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

Awards

41.2059 USDC - $41.21

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

2022-08-FOUNDATION

Low Risk and Non-Critical Issues

Wrong ERC/EIP standard number

The minimal proxy pattern has been accepted as eip1167 (not 1165)

File: contracts/NFTCollectionFactory.sol

60:   * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a NFT collection contract implementation.

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#L60

Adding a `return` statement when the function defines a named return variable, is redundant

There are 1 instances of this issue:

File: contracts/NFTDropMarket.sol

108:  function _getSellerOf(address nftContract, uint256 tokenId)
118:        return payable(address(0));
125:      return super._getSellerOf(nftContract, tokenId);

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropMarket.sol

Events not emmited on important state changes

Emmiting events is recommended each time when a state variable's value is being changed or just some critical event for the contract has occurred. It also helps off-chain monitoring of the contract's state.

There are 3 instances of this issue:

File: contracts/mixins/collections/SequentialMintCollection.sol

62:   function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol

File: contracts/NFTCollectionFactory.sol

192:  function initialize(uint32 _versionNFTCollection) external initializer {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

File: contracts/NFTDropCollection.sol

120:  function initialize(

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

`public` functions not called by the contract should be declared `external` instead

There are 6+ instances of this issue:

File: contracts/mixins/roles/AdminRole.sol

47:   function isAdmin(address account) public view returns (bool approved) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol

File: contracts/mixins/shared/FoundationTreasuryNode.sol

59:   function getFoundationTreasury() public view returns (address payable treasuryAddress) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/FoundationTreasuryNode.sol

File: contracts/NFTCollection.sol

298:  function getTokenCreatorPaymentAddress(uint256 tokenId)

326:  function tokenURI(uint256 tokenId) public view override returns (string memory uri) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: contracts/NFTDropCollection.sol

252:  function getTokenCreatorPaymentAddress(

300:  function tokenURI(uint256 tokenId) public view override returns (string memory uri) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

Non-library/interface files should use fixed compiler versions, not floating ones

Change:

3:    pragma solidity ^0.8.12;

to:

3:    pragma solidity 0.8.12;

There are 18 instances of this issue:

Not used import

There are 3 instances of this issue:

File: contracts/mixins/nftDropMarket/NFTDropMarketCore.sol

5:    import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol

File: contracts/mixins/shared/ContractFactory.sol

7:    import "../../interfaces/ICollectionFactory.sol";

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/ContractFactory.sol

File: contracts/mixins/shared/MarketSharedCore.sol

5:    import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketSharedCore.sol

Not checking for address(0) as function parameter on critical places

There are 5 instances of this issue:

File: contracts/NFTCollection.sol

142:  function mintAndApprove(string calldata tokenCID, address operator) external returns (uint256 tokenId) {

174:  function mintWithCreatorPaymentAddressAndApprove(
177:    address operator
180:    setApprovalForAll(operator, true);

215:  function mintWithCreatorPaymentFactoryAndApprove(
219:    address operator
222:    setApprovalForAll(operator, true);

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: contracts/NFTCollectionFactory.sol

324:  function createNFTDropCollectionWithPaymentAddress(
332:    address payable paymentAddress
342:        paymentAddress != msg.sender ? paymentAddress : payable(0),

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

File: contracts/mixins/roles/AdminRole.sol

13:  function _initializeAdminRole(address admin) internal onlyInitializing {
14:    // Grant the role to a specified account
15:    _grantRole(DEFAULT_ADMIN_ROLE, admin);

28:  function grantAdmin(address account) external {
29:    grantRole(DEFAULT_ADMIN_ROLE, account);
30:  }

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol

File: contracts/mixins/roles/MinterRole.sol

26:  function _initializeMinterRole(address minter) internal onlyInitializing {
27:    // Grant the role to a specified account
28:    _grantRole(MINTER_ROLE, minter);

36:  function grantMinter(address account) external {
37:    grantRole(MINTER_ROLE, account);
38:  }

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol

#0 - HardlyDifficult
2022-08-18T21:00:56Z

Wrong ERC/EIP standard number

Fixed!

Use named returns consistently

Agree, we have opted to use the named returns instead of return ... This is more consistent with other code in our repo and saves a bit of on the contract size. We also like named returns as a way of improving natspec, and typechain (particularly when a tuple is returned).

Events not emmited on important state changes

For the collections, the information is emitted by the factory already. Emitting here would be redundant. For the factory, that information is available via an event when the first template is set.

public functions not called by the contract should be declared external instead

Invalid - all those are used internally or public due to inheritance requirements.

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.

Not used import

Fixed!

Not checking for address(0) as function parameter on critical places

For most of those, address(0) is not harmful. Although it may be more clear to require they use alt functions when 0 is desired.

For the role classes, we wanted to be consistent with grantRole which will also be available publicly via the OZ dependency.

Findings Information

🌟 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

Awards

23.3346 USDC - $23.33

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

2022-08-FOUNDATION

Gas Optimizations Report

The usage of `++x` will cost less gas than `x++`. The same change can be applied to `x--` as well.

This change would save up to 6 gas per instance/loop.

There are 2 instances of this issue:

File: contracts/NFTCollectionFactory.sol

207:  versionNFTCollection++;

231:  versionNFTDropCollection++;

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

State variables should be cached in stack variables rather than re-reading them.

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

There are 3 instances of this issue:

File: contracts/mixins/shared/MarketFees.sol

      /// @audit Cache `assumePrimarySale`. Used 2 times in `_getFees`
445:  if (creatorRecipients.length != 0 || assumePrimarySale) {
447:  if (assumePrimarySale) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol

File: contracts/NFTCollectionFactory.sol

      /// @audit Cache `versionNFTCollection`. Used 3 times in `adminUpdateNFTCollectionImplementation`
213:  string.concat("NFT Collection Implementation v", versionNFTCollection.toString()),
214:  string.concat("NFTv", versionNFTCollection.toString())
217:  emit ImplementationNFTCollectionUpdated(_implementation, versionNFTCollection);

      /// @audit Cache `versionNFTDropCollection`. Used 3 times in `adminUpdateNFTDropCollectionImplementation`
234:  emit ImplementationNFTDropCollectionUpdated(_implementation, versionNFTDropCollection);
239:  string.concat("NFT Drop Collection Implementation v", versionNFTDropCollection.toString()),
240:  string.concat("NFTDropV", versionNFTDropCollection.toString()),

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

Using `!= 0` on `uints` costs less gas than `> 0`.

This change saves 3 gas per instance/loop

There are 2 instances of this issue:

File: contracts/mixins/shared/MarketFees.sol

244:  if (royaltyAmount > 0) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol

File: contracts/NFTDropCollection.sol

131:  require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

It costs more gas to initialize non-`constant`/non-`immutable` variables to zero than to let the default of zero be applied

Not overwriting the default for stack variables saves 8 gas. Storage and memory variables have larger savings

There are 5 instances of this issue:

File: contracts/libraries/BytesLibrary.sol

25:   for (uint256 i = 0; i < 20; ++i) {

44:   for (uint256 i = 0; i < 4; ++i) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/BytesLibrary.sol

File: contracts/mixins/shared/MarketFees.sol

126:  for (uint256 i = 0; i < creatorRecipients.length; ++i) {

198:  for (uint256 i = 0; i < creatorShares.length; ++i) {

484:  for (uint256 i = 0; i < creatorRecipients.length; ++i) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol

Using `private` rather than `public` for constants, saves gas

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

There are 2 instances of this issue:

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

70:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

File: contracts/mixins/roles/MinterRole.sol

19:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol

Functions that are access-restricted from most users may be marked as `payable`

Marking a function as payable reduces gas cost since the compiler does not have to check whether a payment was provided or not. This change will save around 21 gas per function call.

There are 6 instances of this issue:

File: contracts/NFTCollectionFactory.sol

202:  function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {

226:  function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

File: contracts/NFTDropCollection.sol

159:  function burn(uint256 tokenId) public override onlyAdmin {

195:  function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {

209:  function selfDestruct() external onlyAdmin {

220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

`++i`/`i++` should be `unchecked{++I}`/`unchecked{I++}` in `for`-loops

When an increment or any arithmetic operation is not possible to overflow it should be placed in unchecked{} block. \This is because of the default compiler overflow and underflow safety checks since Solidity version 0.8.0. \In for-loops it saves around 30-40 gas per loop

There is 1 instance of this issue:

File: contracts/mixins/shared/MarketFees.sol

198:  for (uint256 i = 0; i < creatorShares.length; ++i) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol

Usage of `uint`s/`int`s smaller than 32 bytes (256 bits) incurs overhead

'When using elements that are smaller than 32 bytes, your contract’s 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://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html \ Use a larger size then downcast where needed

There are 19 instances of this issue:

File: contracts/mixins/collections/SequentialMintCollection.sol

27:   uint32 public latestTokenId;

34:   uint32 public maxTokenId;

40:   uint32 private burnCounter;

62:   function _initializeSequentialMintCollection(address payable _creator, uint32 _maxTokenId) internal onlyInitializing {

86:   function _updateMaxTokenId(uint32 _maxTokenId) internal {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

58:   uint16 limitPerAccount;

121:  uint16 limitPerAccount

172:  uint16 count,

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

File: contracts/NFTCollection.sol

251:  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: 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,

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

File: contracts/NFTDropCollection.sol

126:  uint32 _maxTokenId,

171:  function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {

220:  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

Use `calldata` instead of `memory` for function parameters

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

There are 9 instances of this issue:

File: contracts/libraries/AddressLibrary.sol

25:   function callAndReturnContractAddress(address externalContract, bytes memory callData)

34:   function callAndReturnContractAddress(CallWithoutValue memory call)

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/AddressLibrary.sol

File: contracts/libraries/ArrayLibrary.sol

13:   function capLength(address payable[] memory data, uint256 maxLength) internal pure {

25:   function capLength(uint256[] memory data, uint256 maxLength) internal pure {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/ArrayLibrary.sol

File: contracts/libraries/BytesLibrary.sol

16:   bytes memory data,

38:   function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/BytesLibrary.sol

File: contracts/NFTCollection.sol

107:  string memory _name,

108:  string memory _symbol

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: contracts/NFTCollectionFactory.sol

371:  CallWithoutValue memory paymentAddressFactoryCall

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

Replace `x <= y` with `x < y + 1`, and `x >= y` with `x > y - 1`

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =. Using strict comparison operators hence saves gas

There are 5 instances of this issue:

File: contracts/mixins/collections/SequentialMintCollection.sol

89:   require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

240:  if (currentBalance >= limitPerAccount) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

File: contracts/NFTCollection.sol

268:  require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: contracts/NFTDropCollection.sol

179:  require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

181:  for (uint256 i = firstTokenId; i <= latestTokenId; ) {

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

`require()/revert()` strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

There are 28 instances of this issue:

File: contracts/libraries/AddressLibrary.sol

31:   require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/AddressLibrary.sol

File: contracts/mixins/collections/SequentialMintCollection.sol

58:   require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

63:   require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

74:   require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

87:   require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

88:   require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

89:   require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol

File: contracts/mixins/roles/AdminRole.sol

19:   require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol

File: contracts/mixins/roles/MinterRole.sol

22:   require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol

File: contracts/mixins/shared/ContractFactory.sol

22:   require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

31:   require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/ContractFactory.sol

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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: 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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

Use custom errors rather than `revert()`/`require()` strings to save gas

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

There are 28 instances of this issue:

File: contracts/libraries/AddressLibrary.sol

31:   require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/libraries/AddressLibrary.sol

File: contracts/mixins/collections/SequentialMintCollection.sol

58:   require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

63:   require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

74:   require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

87:   require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

88:   require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

89:   require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/collections/SequentialMintCollection.sol

File: contracts/mixins/roles/AdminRole.sol

19:   require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/AdminRole.sol

File: contracts/mixins/roles/MinterRole.sol

22:   require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol

File: contracts/mixins/shared/ContractFactory.sol

22:   require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

31:   require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/ContractFactory.sol

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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol

File: 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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol

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");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol

Expressions for constant values such as a call to `keccak256()`, should use `immutable` rather than `constant`

It is expected that the value should be converted into a constant value at compile time. But actually the expression is re-calculated each time the constant is referenced.

There are 2 instances of this issue:

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

70:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

File: contracts/mixins/roles/MinterRole.sol

19:   bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/roles/MinterRole.sol

Using bools for storage variables incurs overhead

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past

There are 1 instances of this issue:

File: contracts/mixins/shared/MarketFees.sol

61:   bool private immutable assumePrimarySale;

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/mixins/shared/MarketFees.sol

#0 - HardlyDifficult
2022-08-17T14:58:27Z

The usage of ++x will cost less gas than x++. The same change can be applied to x-- as well.

Agree, will fix.

State variables should be cached in stack variables rather than re-reading them.

assumePrimarySale is immutable -- invalid.
version* -- may be valid, but these are admin functions which are rarely called, we are not trying to optimize.

Using != 0 on uints costs less gas than > 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

It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Invalid. This optimization technique is no longer applicable with the current version of Solidity.

Using private rather than public for constants, saves gas

Agree but won't fix. For ease of use and consistency we will continue to expose some constants publicly.

Functions that are access-restricted from most users may be marked as 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.

++i/i++ should be unchecked{++I}/unchecked{I++} in for-loops

getFeesAndRecipients is a read only function not intended to be used on-chain, but as a best practice we will add unchecked there as well.

Usage of 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 calldata instead of memory for function parameters

capLength is operating on internally sourced data so we cannot use calldata -- invalid.

Valid & will fix. This saves ~50 gas on createNFTCollection and ~60 gas on createNFTDropCollectionWithPaymentFactory

Replace x <= y with x < y + 1, and x >= y with x > y - 1

This could potentially be valid but won't fix to improve code readability and avoid potential overflow/underflow concerns.

require()/revert() strings longer than 32 bytes cost extra gas

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.

Use custom errors rather than revert()/require() strings to save gas

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.

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

While it seems this should help, changing to immutable shows a regression in our test suite. Gas costs go up by ~50 gas.

Foundation Drop contest - gogo's results

Foundation is a web3 destination.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-36] QA Report - $41.21

Findings Information

Awards

Labels

External Links

2022-08-FOUNDATION

Low Risk and Non-Critical Issues

Wrong ERC/EIP standard number

Adding a return statement when the function defines a named return variable, is redundant

Events not emmited on important state changes

public functions not called by the contract should be declared external instead

Non-library/interface files should use fixed compiler versions, not floating ones

Not used import

Not checking for address(0) as function parameter on critical places

#0 - HardlyDifficult2022-08-18T21:00:56Z

[G-35] Gas Report - $23.33

Findings Information

Awards

Labels

External Links

2022-08-FOUNDATION

Gas Optimizations Report

The usage of ++x will cost less gas than x++. The same change can be applied to x-- as well.

State variables should be cached in stack variables rather than re-reading them.

Using != 0 on uints costs less gas than > 0.

It costs more gas to initialize non-constant/non-immutable variables to zero than to let the default of zero be applied

Using private rather than public for constants, saves gas

Functions that are access-restricted from most users may be marked as payable

++i/i++ should be unchecked{++I}/unchecked{I++} in for-loops

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

Use calldata instead of memory for function parameters

Replace x <= y with x < y + 1, and x >= y with x > y - 1

require()/revert() strings longer than 32 bytes cost extra gas

Use custom errors rather than revert()/require() strings to save gas

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Using bools for storage variables incurs overhead

#0 - HardlyDifficult2022-08-17T14:58:27Z

Adding a `return` statement when the function defines a named return variable, is redundant

`public` functions not called by the contract should be declared `external` instead

#0 - HardlyDifficult
2022-08-18T21:00:56Z

The usage of `++x` will cost less gas than `x++`. The same change can be applied to `x--` as well.

Using `!= 0` on `uints` costs less gas than `> 0`.

It costs more gas to initialize non-`constant`/non-`immutable` variables to zero than to let the default of zero be applied

Using `private` rather than `public` for constants, saves gas

Functions that are access-restricted from most users may be marked as `payable`

`++i`/`i++` should be `unchecked{++I}`/`unchecked{I++}` in `for`-loops

Usage of `uint`s/`int`s smaller than 32 bytes (256 bits) incurs overhead

Use `calldata` instead of `memory` for function parameters

Replace `x <= y` with `x < y + 1`, and `x >= y` with `x > y - 1`

`require()/revert()` strings longer than 32 bytes cost extra gas

Use custom errors rather than `revert()`/`require()` strings to save gas

Expressions for constant values such as a call to `keccak256()`, should use `immutable` rather than `constant`

#0 - HardlyDifficult
2022-08-17T14:58:27Z