Back to erictee's contests

Foundation Drop contest - erictee'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: 32/108

Findings: 3

Award: $76.74

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryFoundation

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.2563 USDC - $41.26

Labels

bug
QA (Quality Assurance)
edited-by-warden

External Links

Link to GitHub issue

[L-01] Do not use Deprecated Library Functions.

Impact

The usage of deprecated library functions should be discouraged. _setupRole() is deprecated in favor of _grantRole().

Findings:
2022-08-foundation/contracts/mixins/treasury/AdminRole.sol:L16    _setupRole(DEFAULT_ADMIN_ROLE, admin);

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L173  function _setupRole(bytes32 role, address account) internal {

[L-02] Open TODOs

Impact

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

Findings:
2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L193      // TODO add referral info

[L-03] Avoid floating pragmas for non-library contracts.

Impact

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.

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTDropMarket.sol:L42     pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTCollection.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/NFTCollectionFactory.sol:L42     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IOperatorRole.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IFethMarket.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTDropCollectionMint.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IGetRoyalties.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IRoyaltyInfo.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTCollectionInitializer.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/ITokenCreator.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IERC20Approve.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IGetFees.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IRoles.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IOwnable.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/INFTDropCollectionInitializer.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IProxyCall.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IAdminRole.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/ICollectionFactory.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/interfaces/IERC20IncreaseAllowance.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/MarketSharedCore.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/FETHNode.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/FoundationTreasuryNode.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/Gap10000.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/Constants.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/OZERC165Checker.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketCore.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/collections/CollectionRoyalties.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/OperatorRole.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/CollateralManagement.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L10     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/roles/MinterRole.sol:L3     pragma solidity ^0.8.12;

2022-08-foundation/contracts/mixins/roles/AdminRole.sol:L3     pragma solidity ^0.8.12;

[L-04] require()/revert() statements should have descriptive strings.

Impact

Consider adding descriptive strings in require()/revert().

Findings:
2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L37 *     require(hasRole(MY_ROLE, msg.sender));

[L-05] Missing checks for address(0x0) when assigning values to address state variables

Findings:

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollection.sol#L96 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropCollection.sol#L102 https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTDropMarket.sol#L87-L90

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

Impact

_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

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L182      _mint(to, i);

2022-08-foundation/contracts/NFTCollection.sol:L130    tokenId = _mint(tokenCID);

2022-08-foundation/contracts/NFTCollection.sol:L143    tokenId = _mint(tokenCID);

2022-08-foundation/contracts/NFTCollection.sol:L159    tokenId = _mint(tokenCID);

2022-08-foundation/contracts/NFTCollection.sol:L262  function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {

2022-08-foundation/contracts/NFTCollection.sol:L271      _mint(msg.sender, tokenId);

#0 - HardlyDifficult

2022-08-18T21:29:15Z

discouraged. _setupRole()

Agree, fixed.

Unresolved TODO comments

Agree, will fix.

Use fixed pragma

Disagree. We intentionally use a floating pragma in order to make integrating with contracts easier. Other contract developers are looking to interact with our contracts and they may be on a different version than we use. The pragma selected for our contracts is the minimum required in order to correctly compile and function. This way integration is easier if they lag a few versions behind, or if they use the latest but we don't bump our packages frequently enough, and when we do upgrade versions unless there was a breaking solidity change -- it should just swap in by incrementing our npm package version.

[L-04] require()/revert() statements should have descriptive strings.

This is a clone of the OZ written file with minimal modifications. We want to make as few edits as possible so will leave this as they implemented it.

[L-05] Missing checks for address(0x0) when assigning values to address state variables

Invalid. All those will revert if address(0) is provided with the .isContract checks used.

Use safeMint

Agree will fix - for context see our response here.

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

22.3055 USDC - $22.31

Labels

bug
G (Gas Optimization)
edited-by-warden

External Links

Link to GitHub issue

[G-01] Use assembly to check for zero address.

Impact

Save 6 gas when assembly is used to check for zero address. e.g:

assembly {
            if iszero(_addr) {
                mstore(0x00, "zero address")
                revert(0x00, 0x20)
            }
Findings:
2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L63    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

[G-02] Use custom errors rather than revert()/require() string to save gas

Impact

Custom errors are available from solidity version 0.8.4, it saves around 50 gas each time they are hit by avoiding having to allocate and store the revert string.

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L88    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L93    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

2022-08-foundation/contracts/NFTDropCollection.sol:L130    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L131    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L172    require(count != 0, "NFTDropCollection: `count` must be greater than 0");

2022-08-foundation/contracts/NFTDropCollection.sol:L179    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

2022-08-foundation/contracts/NFTDropCollection.sol:L238    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

2022-08-foundation/contracts/NFTCollection.sol:L158    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

2022-08-foundation/contracts/NFTCollection.sol:L263    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");

2022-08-foundation/contracts/NFTCollection.sol:L264    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

2022-08-foundation/contracts/NFTCollection.sol:L268      require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

2022-08-foundation/contracts/NFTCollection.sol:L327    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L173    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L182    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L203    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L227    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L262    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

2022-08-foundation/contracts/libraries/AddressLibrary.sol:L31    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:L22    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:L31    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L58    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L63    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L74    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L87    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L88    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L89    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol:L20    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L137    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L152    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol:L22    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

2022-08-foundation/contracts/mixins/roles/AdminRole.sol:L19    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

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

Impact

When working with unsigned integer types, comparisons with != 0 are cheaper than with > 0 . This changes saves 6 gas per instance.

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L88    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L130    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L131    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

[G-04] Functions guaranteed to revert when called by normal users can be declared as payable.

Impact

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.

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L159  function burn(uint256 tokenId) public override onlyAdmin {

2022-08-foundation/contracts/NFTDropCollection.sol:L171  function mintCountTo(uint16 count, address to) external onlyMinterOrAdmin returns (uint256 firstTokenId) {

2022-08-foundation/contracts/NFTDropCollection.sol:L195  function reveal(string calldata _baseURI) external onlyAdmin validBaseURI(_baseURI) onlyWhileUnrevealed {

2022-08-foundation/contracts/NFTDropCollection.sol:L209  function selfDestruct() external onlyAdmin {

2022-08-foundation/contracts/NFTDropCollection.sol:L220  function updateMaxTokenId(uint32 _maxTokenId) external onlyAdmin {

2022-08-foundation/contracts/NFTCollection.sol:L119  function burn(uint256 tokenId) public override onlyCreator {

2022-08-foundation/contracts/NFTCollection.sol:L230  function selfDestruct() external onlyCreator {

2022-08-foundation/contracts/NFTCollection.sol:L238  function updateBaseURI(string calldata baseURIOverride) external onlyCreator {

2022-08-foundation/contracts/NFTCollection.sol:L251  function updateMaxTokenId(uint32 _maxTokenId) external onlyCreator {

2022-08-foundation/contracts/NFTCollection.sol:L262  function _mint(string calldata tokenCID) private onlyCreator returns (uint256 tokenId) {

2022-08-foundation/contracts/NFTCollectionFactory.sol:L202  function adminUpdateNFTCollectionImplementation(address _implementation) external onlyAdmin {

2022-08-foundation/contracts/NFTCollectionFactory.sol:L226  function adminUpdateNFTDropCollectionImplementation(address _implementation) external onlyAdmin {

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol:L14  function _initializeAdminRole(address admin) internal onlyInitializing {

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L166   * This function should only be called from the constructor when setting

2022-08-foundation/contracts/mixins/roles/MinterRole.sol:L26  function _initializeMinterRole(address minter) internal onlyInitializing {

2022-08-foundation/contracts/mixins/roles/AdminRole.sol:L13  function _initializeAdminRole(address admin) internal onlyInitializing {

[G-05] ++i costs less gas than i++, especially when it's used in for loops.

Impact

Saves 6 gas per loop.

Findings:
2022-08-foundation/contracts/NFTCollectionFactory.sol:L207      versionNFTCollection++;

2022-08-foundation/contracts/NFTCollectionFactory.sol:L231      versionNFTDropCollection++;

[G-06] Revert message greater than 32 bytes

Impact

Keep revert message lower than or equal to 32 bytes to save gas.

Findings:
2022-08-foundation/contracts/NFTDropCollection.sol:L88    require(bytes(_baseURI).length > 0, "NFTDropCollection: `_baseURI` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L93    require(postRevealBaseURIHash != bytes32(0), "NFTDropCollection: Already revealed");

2022-08-foundation/contracts/NFTDropCollection.sol:L130    require(bytes(_symbol).length > 0, "NFTDropCollection: `_symbol` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L131    require(_maxTokenId > 0, "NFTDropCollection: `_maxTokenId` must be set");

2022-08-foundation/contracts/NFTDropCollection.sol:L172    require(count != 0, "NFTDropCollection: `count` must be greater than 0");

2022-08-foundation/contracts/NFTDropCollection.sol:L179    require(latestTokenId <= maxTokenId, "NFTDropCollection: Exceeds max tokenId");

2022-08-foundation/contracts/NFTDropCollection.sol:L238    require(_postRevealBaseURIHash != bytes32(0), "NFTDropCollection: use `reveal` instead");

2022-08-foundation/contracts/NFTCollection.sol:L158    require(tokenCreatorPaymentAddress != address(0), "NFTCollection: tokenCreatorPaymentAddress is required");

2022-08-foundation/contracts/NFTCollection.sol:L263    require(bytes(tokenCID).length != 0, "NFTCollection: tokenCID is required");

2022-08-foundation/contracts/NFTCollection.sol:L264    require(!cidToMinted[tokenCID], "NFTCollection: NFT was already minted");

2022-08-foundation/contracts/NFTCollection.sol:L268      require(maxTokenId == 0 || tokenId <= maxTokenId, "NFTCollection: Max token count has already been minted");

2022-08-foundation/contracts/NFTCollection.sol:L327    require(_exists(tokenId), "NFTCollection: URI query for nonexistent token");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L173    require(rolesContract.isAdmin(msg.sender), "NFTCollectionFactory: Caller does not have the Admin role");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L182    require(_rolesContract.isContract(), "NFTCollectionFactory: RolesContract is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L203    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L227    require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

2022-08-foundation/contracts/NFTCollectionFactory.sol:L262    require(bytes(symbol).length != 0, "NFTCollectionFactory: Symbol is required");

2022-08-foundation/contracts/libraries/AddressLibrary.sol:L31    require(contractAddress.isContract(), "InternalProxyCall: did not return a contract");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:L22    require(msg.sender == contractFactory, "ContractFactory: Caller is not the factory");

2022-08-foundation/contracts/mixins/shared/ContractFactory.sol:L31    require(_contractFactory.isContract(), "ContractFactory: Factory is not a contract");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L58    require(msg.sender == owner, "SequentialMintCollection: Caller is not creator");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L63    require(_creator != address(0), "SequentialMintCollection: Creator cannot be the zero address");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L74    require(totalSupply() == 0, "SequentialMintCollection: Any NFTs minted must be burned first");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L87    require(_maxTokenId != 0, "SequentialMintCollection: Max token ID may not be cleared");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L88    require(maxTokenId == 0 || _maxTokenId < maxTokenId, "SequentialMintCollection: Max token ID may not increase");

2022-08-foundation/contracts/mixins/collections/SequentialMintCollection.sol:L89    require(latestTokenId <= _maxTokenId, "SequentialMintCollection: Max token ID must be >= last mint");

2022-08-foundation/contracts/mixins/treasury/AdminRole.sol:L20    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L137    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to grant");

2022-08-foundation/contracts/mixins/treasury/OZAccessControlUpgradeable.sol:L152    require(hasRole(_roles[role].adminRole, _msgSender()), "AccessControl: sender must be an admin to revoke");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol:L22    require(isMinter(msg.sender) || isAdmin(msg.sender), "MinterRole: Must have the minter or admin role");

2022-08-foundation/contracts/mixins/roles/AdminRole.sol:L19    require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "AdminRole: caller does not have the Admin role");

[G-07] Explicit initialization with zero not required

Impact

Explicit initialization with zero is not required for variable declaration because uints are 0 by default. Removing this will reduce contract size and save a bit of gas.

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

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

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L126      for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L198    for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L484          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-08] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops.

Impact

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.

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

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

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L126      for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L198    for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L484          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

[G-09] Cache Array Length Outside of Loop

Impact

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.

Findings:
2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L126      for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L198    for (uint256 i = 0; i < creatorShares.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L484          for (uint256 i = 0; i < creatorRecipients.length; ++i) {

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L503      for (uint256 i = 1; i < creatorRecipients.length; ) {

[G-10] Using private rather than public for constants to saves gas.

Impact

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.

Findings:
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:L70  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

2022-08-foundation/contracts/mixins/roles/MinterRole.sol:L19  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

[G-11] X += Y costs more gas than X = X + Y for state variables.

Impact

Consider changing X += Y to X = X + Y to save gas.

Findings:
2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L134        creatorRev += creatorShares[i];

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L150        totalFees += buyReferrerFee;

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L199      creatorRev += creatorShares[i];

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L490            totalShares += creatorShares[i];

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L505        totalRoyaltiesDistributed += royalty;

[G-12] Using bools for storage incurs overhead.

Impact
    // 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.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

Findings:
2022-08-foundation/contracts/NFTCollection.sol:L53  mapping(string => bool) private cidToMinted;

2022-08-foundation/contracts/mixins/shared/MarketFees.sol:L61  bool private immutable assumePrimarySale;

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead.

Impact

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.11/internals/layout_in_storage.html Use a larger size then downcast where needed.

Findings:
2022-08-foundation/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol:L172    uint16 count,

#0 - HardlyDifficult

2022-08-17T10:47:33Z

  1. Use assembly to check for zero address.

Won't fix. While this may save a tiny bit of gas, we reserve the use of assembly to areas where it would provide significant benefit or accomplish something that's otherwise not possible with Solidity alone.

  1. 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.

  1. 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
  1. 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.

  1. ++i costs less than i++

Agree and will fix.

  1. 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.

  1. Don't initialize variables with default values.

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

[G-08] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops.

5 examples provided, 4 are already unchecked - so those are invalid. 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.

  1. 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.

  1. Using private rather than public for constants to saves gas.

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

  1. += COSTS MORE GAS THAN = + FOR STATE VARIABLES

No impact. We made the recommended change and ran gas-reporter for our entire test suite. 0 impact detected. Even if there were a small benefit, we feel this hurts readability and would not be worth trivial savings.

  1. Using bools for storage incurs overhead.

Valid for cidToMinted, saving ~200 gas. Not seeing any benefit for assumePrimarySale, potentially because it's an immutable variable.

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead.

Potentially valid but won't fix. The input here is 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.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter