Foundation Drop contest - oyc_109's results

Judge has assessed an item in Issue #57 as Medium risk. The relevant finding follows:

[L-01] Unprotected initializer

the initializer function in NFTCollectionFactory() and NFTDropMarket.sol does not have any access control modifiers so anyone can front run and call the initializer function

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L192-L194 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L100-L102

[L-02] Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

2022-08-foundation/contracts/NFTCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTCollectionFactory.sol::42 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTDropCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTDropMarket.sol::42 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IAdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/ICollectionFactory.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IERC20Approve.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IERC20IncreaseAllowance.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IFethMarket.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IGetFees.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IGetRoyalties.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTDropCollectionMint.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IOperatorRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IOwnable.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IProxyCall.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IRoles.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IRoyaltyInfo.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/ITokenCreator.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/AddressLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/ArrayLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/BytesLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/LockedBalance.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/roles/MinterRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/Constants.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/FETHNode.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/Gap10000.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/OZERC165Checker.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::10 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/OperatorRole.sol::3 => pragma solidity ^0.8.12;

[L-03] Unused receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::29 => receive() external payable {}

[L-04] Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::193 => // TODO add referral info

[L-05] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). Unless there is a compelling reason, abi.encode should be preferred. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

2022-08-foundation/contracts/NFTCollectionFactory.sol::449 => return keccak256(abi.encodePacked(creator, nonce));

[L-06] _safeMint() should be used rather than _mint() wherever possible

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead

2022-08-foundation/contracts/NFTCollection.sol::130 => tokenId = _mint(tokenCID);
2022-08-foundation/contracts/NFTCollection.sol::143 => tokenId = _mint(tokenCID);
2022-08-foundation/contracts/NFTCollection.sol::159 => tokenId = _mint(tokenCID);
2022-08-foundation/contracts/NFTCollection.sol::262 => function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {
2022-08-foundation/contracts/NFTCollection.sol::271 => _mint(msg.sender, tokenId);
2022-08-foundation/contracts/NFTDropCollection.sol::182 => _mint(to, i);

[N-1] Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

2022-08-foundation/contracts/NFTCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTCollectionFactory.sol::42 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTDropCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/NFTDropMarket.sol::42 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IAdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/ICollectionFactory.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IERC20Approve.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IERC20IncreaseAllowance.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IFethMarket.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IGetFees.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IGetRoyalties.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/INFTDropCollectionMint.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IOperatorRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IOwnable.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IProxyCall.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IRoles.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/IRoyaltyInfo.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/interfaces/ITokenCreator.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/AddressLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/ArrayLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/BytesLibrary.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/libraries/LockedBalance.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/roles/MinterRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/Constants.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/FETHNode.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/Gap10000.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/OZERC165Checker.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::3 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::10 => pragma solidity ^0.8.12;
2022-08-foundation/contracts/mixins/treasury/OperatorRole.sol::3 => pragma solidity ^0.8.12;

[N-2] Large multiples of ten should use scientific notation

Use (e.g. 1e6) rather than decimal literals (e.g. 1000000), for better code readability

2022-08-foundation/contracts/mixins/shared/Constants.sol::10 => uint256 constant BASIS_POINTS = 10000;
2022-08-foundation/contracts/mixins/shared/Constants.sol::48 => uint256 constant SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS = 210000;

[N-3] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

2022-08-foundation/contracts/NFTCollection.sol::70 => event BaseURIUpdated(string baseURI);
2022-08-foundation/contracts/NFTDropCollection.sol::85 => event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);

[N-4] Missing SPDX license identifier

File should include SPDX-License-Identifier

2022-08-foundation/contracts/NFTCollectionFactory.sol::1 => /*
2022-08-foundation/contracts/NFTDropMarket.sol::1 => /*

[N-5] Missing NatSpec

Code should include NatSpec

2022-08-foundation/contracts/interfaces/ICollectionFactory.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0
2022-08-foundation/contracts/interfaces/IERC20Approve.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0
2022-08-foundation/contracts/interfaces/IERC20IncreaseAllowance.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0
2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0
2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0
2022-08-foundation/contracts/interfaces/IProxyCall.sol::1 => // SPDX-License-Identifier: MIT OR Apache-2.0

[N-6] Constants should be defined rather than using magic numbers

It is bad practice to use numbers directly in code without explanation

2022-08-foundation/contracts/libraries/BytesLibrary.sol::25 => for (uint256 i = 0; i < 20; ++i) {

[N-7] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

2022-08-foundation/contracts/NFTCollection.sol::326 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
2022-08-foundation/contracts/NFTDropCollection.sol::300 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::47 => function isAdmin(address account) public view returns (bool approved) {
2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::59 => function getFoundationTreasury() public view returns (address payable treasuryAddress) {
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::36 => * function foo() public {

[N-8] Adding a return statement when the function defines a named return variable, is redundant

It is not necessary to have both a named return and a return statement.

2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::59 => function getFoundationTreasury() public view returns (address payable treasuryAddress) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::208 => function getRoyaltyRegistry() external view returns (address registry) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::234 => returns (address payable[] memory recipients, uint256[] memory splitPerRecipientInBasisPoints)
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::283 => ) external view returns (address payable[] memory recipients, uint256[] memory splitPerRecipientInBasisPoints) {
2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol::20 => function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {

[N-9] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

instances:

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::70 => bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
2022-08-foundation/contracts/mixins/roles/MinterRole.sol::19 => bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::29 => * bytes32 public constant MY_ROLE = keccak256("MY_ROLE");
2022-08-foundation/contracts/mixins/treasury/OperatorRole.sol::14 => bytes32 private constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");

[G-01] Don't Initialize Variables with Default Value

Uninitialized variables are assigned with the types default value. Explicitly initializing a variable with it's default value costs unnecesary gas.

2022-08-foundation/contracts/libraries/BytesLibrary.sol::25 => for (uint256 i = 0; i < 20; ++i) {
2022-08-foundation/contracts/libraries/BytesLibrary.sol::44 => for (uint256 i = 0; i < 4; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-02] Cache Array Length Outside of Loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::503 => for (uint256 i = 1; i < creatorRecipients.length; ) {

[G-03] Using > 0 costs more gas than != 0 when used on a uint in a require() statement

When dealing with unsigned integer types, comparisons with != 0 are cheaper then with > 0. This change saves 6 gas per instance

2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

[G-04] Long Revert Strings

Shortening revert strings to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

2022-08-foundation/contracts/NFTCollection.sol::158 => require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
2022-08-foundation/contracts/NFTCollection.sol::263 => require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
2022-08-foundation/contracts/NFTCollection.sol::264 => require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
2022-08-foundation/contracts/NFTCollection.sol::268 => require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
2022-08-foundation/contracts/NFTCollection.sol::327 => require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
2022-08-foundation/contracts/NFTCollectionFactory.sol::173 => require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
2022-08-foundation/contracts/NFTCollectionFactory.sol::182 => require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::203 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::227 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::262 => require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::93 => require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::172 => require(count != 0, "NFTDropCollection: `count` must be greater than 0");
2022-08-foundation/contracts/NFTDropCollection.sol::179 => require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
2022-08-foundation/contracts/NFTDropCollection.sol::238 => require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
2022-08-foundation/contracts/libraries/AddressLibrary.sol::31 => require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::58 => require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::63 => require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::74 => require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::87 => require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::88 => require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::89 => require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::19 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
2022-08-foundation/contracts/mixins/roles/MinterRole.sol::22 => require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::22 => require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::31 => require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::20 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::137 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::152 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

[G-05] Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

2022-08-foundation/contracts/libraries/LockedBalance.sol::100 => uint256 lockupMetadata = lockups.lockups[index / 2];

[G-06] Use calldata instead of memory

Use calldata instead of memory for function parameters saves gas if the function argument is only read.

2022-08-foundation/contracts/libraries/BytesLibrary.sol::38 => function startsWith(bytes memory callData, bytes4 functionSig) internal pure returns (bool) {

[G-07] Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

2022-08-foundation/contracts/NFTCollectionFactory.sol::104 => IRoles public immutable rolesContract;
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::19 => address public immutable contractFactory;

[G-08] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

2022-08-foundation/contracts/NFTCollection.sol::119 => function burn(uint256 tokenId) public override onlyCreator {
2022-08-foundation/contracts/NFTCollection.sol::230 => function selfDestruct() external onlyCreator {
2022-08-foundation/contracts/NFTCollection.sol::238 => function updateBaseURI(string calldata baseURIOverride) external onlyCreator {
2022-08-foundation/contracts/NFTCollection.sol::251 => function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {
2022-08-foundation/contracts/NFTCollectionFactory.sol::202 => function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {
2022-08-foundation/contracts/NFTCollectionFactory.sol::226 => function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {
2022-08-foundation/contracts/NFTDropCollection.sol::159 => function burn(uint256 tokenId) public override onlyAdmin {
2022-08-foundation/contracts/NFTDropCollection.sol::171 => function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {
2022-08-foundation/contracts/NFTDropCollection.sol::195 => function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {
2022-08-foundation/contracts/NFTDropCollection.sol::209 => function selfDestruct() external onlyAdmin {
2022-08-foundation/contracts/NFTDropCollection.sol::220 => function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

[G-09] Empty blocks should be removed or emit something

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

2022-08-foundation/contracts/NFTCollection.sol::97 => {}
2022-08-foundation/contracts/NFTDropCollection.sol::103 => {}
2022-08-foundation/contracts/NFTDropMarket.sol::94 => {}
2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::29 => receive() external payable {}

[G-10] Usage of uints/ints 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.

2022-08-foundation/contracts/NFTCollectionFactory.sol::80 => uint32 public versionNFTCollection;
2022-08-foundation/contracts/NFTCollectionFactory.sol::95 => uint32 public versionNFTDropCollection;
2022-08-foundation/contracts/libraries/LockedBalance.sol::11 => uint32 expiration;
2022-08-foundation/contracts/libraries/LockedBalance.sol::104 => uint128 lockedBalanceBits;
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::27 => uint32 public latestTokenId;
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::34 => uint32 public maxTokenId;
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::40 => uint32 private burnCounter;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::54 => uint80 price;
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol::58 => uint16 limitPerAccount;

[G-11] Using bools for storage incurs overhead

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. Use uint256(1) and uint256(2) for true/false instead

2022-08-foundation/contracts/NFTCollection.sol::53 => mapping(string => bool) private cidToMinted;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::61 => bool private immutable assumePrimarySale;

[G-12] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

2022-08-foundation/contracts/NFTCollection.sol::267 => tokenId = ++latestTokenId;
2022-08-foundation/contracts/libraries/BytesLibrary.sol::25 => for (uint256 i = 0; i < 20; ++i) {
2022-08-foundation/contracts/libraries/BytesLibrary.sol::44 => for (uint256 i = 0; i < 4; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::126 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::198 => for (uint256 i = 0; i < creatorShares.length; ++i) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::484 => for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-13] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

use <x> = <x> + <y> or <x> = <x> - <y> instead to save gas

2022-08-foundation/contracts/mixins/shared/MarketFees.sol::134 => creatorRev += creatorShares[i];
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::150 => totalFees += buyReferrerFee;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::199 => creatorRev += creatorShares[i];
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::490 => totalShares += creatorShares[i];
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::505 => totalRoyaltiesDistributed += royalty;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::527 => totalFees -= buyReferrerFee;

[G-14] 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

2022-08-foundation/contracts/NFTCollection.sol::158 => require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
2022-08-foundation/contracts/NFTCollection.sol::263 => require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");
2022-08-foundation/contracts/NFTCollection.sol::264 => require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");
2022-08-foundation/contracts/NFTCollection.sol::268 => require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");
2022-08-foundation/contracts/NFTCollection.sol::327 => require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");
2022-08-foundation/contracts/NFTCollectionFactory.sol::173 => require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");
2022-08-foundation/contracts/NFTCollectionFactory.sol::182 => require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::203 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::227 => require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");
2022-08-foundation/contracts/NFTCollectionFactory.sol::262 => require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");
2022-08-foundation/contracts/NFTDropCollection.sol::88 => require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::93 => require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");
2022-08-foundation/contracts/NFTDropCollection.sol::130 => require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::131 => require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");
2022-08-foundation/contracts/NFTDropCollection.sol::172 => require(count != 0, "NFTDropCollection: `count` must be greater than 0");
2022-08-foundation/contracts/NFTDropCollection.sol::179 => require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");
2022-08-foundation/contracts/NFTDropCollection.sol::238 => require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");
2022-08-foundation/contracts/libraries/AddressLibrary.sol::31 => require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::58 => require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::63 => require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::74 => require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::87 => require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::88 => require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::89 => require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::19 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
2022-08-foundation/contracts/mixins/roles/MinterRole.sol::22 => require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::22 => require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");
2022-08-foundation/contracts/mixins/shared/ContractFactory.sol::31 => require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");
2022-08-foundation/contracts/mixins/treasury/AdminRole.sol::20 => require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::137 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol::152 => require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

[G-15] Do not calculate constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas. Consequences: each usage of a constant costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library)

2022-08-foundation/contracts/mixins/shared/Constants.sol::26 => uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;
2022-08-foundation/contracts/mixins/shared/Constants.sol::43 => uint256 constant ROYALTY_RATIO = BASIS_POINTS / ROYALTY_IN_BASIS_POINTS;
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::45 => uint256 private constant CREATOR_ROYALTY_DENOMINATOR = BASIS_POINTS / 1000; // 10%
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::47 => uint256 private constant PROTOCOL_FEE_DENOMINATOR = BASIS_POINTS / 500; // 5%

[G-16] Public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.

2022-08-foundation/contracts/NFTCollection.sol::326 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
2022-08-foundation/contracts/NFTDropCollection.sol::300 => function tokenURI(uint256 tokenId) public view override returns (string memory uri) {
2022-08-foundation/contracts/mixins/roles/AdminRole.sol::47 => function isAdmin(address account) public view returns (bool approved) {
2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::59 => function getFoundationTreasury() public view returns (address payable treasuryAddress) {

[G-17] Not using the named return variables when a function returns, wastes deployment gas

It is not necessary to have both a named return and a return statement.

2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol::59 => function getFoundationTreasury() public view returns (address payable treasuryAddress) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::208 => function getRoyaltyRegistry() external view returns (address registry) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::234 => returns (address payable[] memory recipients, uint256[] memory splitPerRecipientInBasisPoints)
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::283 => ) external view returns (address payable[] memory recipients, uint256[] memory splitPerRecipientInBasisPoints) {
2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol::20 => function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {

[G-18] Use assembly to check for address(0)

Saves 6 gas per instance if using assembly to check for address(0)

e.g.

assembly {
 if iszero(_addr) {
  mstore(0x00, "zero address")
  revert(0x00, 0x20)
 }
}

instances:

2022-08-foundation/contracts/NFTCollection.sol::158 => require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");
2022-08-foundation/contracts/NFTDropCollection.sol::138 => if (_paymentAddress != address(0)) {
2022-08-foundation/contracts/NFTDropCollection.sol::149 => if (_approvedMinter != address(0)) {
2022-08-foundation/contracts/NFTDropMarket.sol::116 => if (owner != address(0)) {
2022-08-foundation/contracts/NFTDropMarket.sol::140 => if (owner != address(0)) {
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol::63 => require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::358 => if (creator != address(0)) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::369 => if (owner != address(0)) {
2022-08-foundation/contracts/mixins/shared/MarketFees.sol::522 => if (buyReferrer != address(0) && buyReferrer != msg.sender && buyReferrer != seller && buyReferrer != creator) {

[G-19] Use selfbalance()

Use selfbalance() instead of address(this).balance when getting your contract's balance of ETH to save gas.

2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol::38 => amount = address(this).balance;