Deivitto - Foundation Drop contest

Findings Information

🌟 Selected for report: rbserver

Also found by: 0xc0ffEE, CodingNameKiki, Deivitto, Diraco, IllIllI, KIntern_NA, Lambda, Noah3o6, Treasure-Seeker, ignacio, oyc_109, zeesaw

Labels

bug

duplicate

2 (Med Risk)

old-submission-method

Awards

13.1705 USDC - $13.17

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L143 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L159 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L271 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L182

Vulnerability details

_safeMint() should be used rather than _mint() wherever possible

Impact

In NFTCollections.sol and NFTDropCollection, eventually it is called ERC721 _mint(). Calling _mint() this way does not ensure that the receiver of the NFT is able to accept them, making possible to lose them.

_safeMint() should be used with as it checks to see if a user can properly accept an NFT and reverts otherwise.

There is no check of the address provided by the mintNFT when creating the project that it implements ERC721Receiver.

Details

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver.

Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

References

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/d4d8d2ed9798cc3383912a23b5e8d5cb602f7d4b/contracts/token/ERC721/ERC721.sol#L271

Github Permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L143 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L159 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L271

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L182

Mitigation

Use _safeMint() as suggested by OpenZeppelin or include the check before minting.

#0 - HardlyDifficult
2022-08-18T12:18:32Z

Dupe of https://github.com/code-423n4/2022-08-foundation-findings/issues/183

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

43.102 USDC - $43.10

Labels

bug

QA (Quality Assurance)

old-submission-method

External Links

Link to GitHub issue

QA

Low

Missing checks for address(0x0) when assigning values to address state variables

Summary

Zero address should be checked for state variables and some parameters in functions like mints, withdrawals... A zero address can lead into problems.

Github Permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L87

Mitigation

Check zero address for state variables before assigning to them a value Check in token/eth/permission assigned related the 0 address

Implementation of the comment spec is confusing

Summary:

It says "for internal use only" However, external visibility is enabled. If "internal use only" means the organization, use a source of access control such as a onlyOwner modifier. If internal is for visibility, change the try catch scenario into a scenario with regular if checks so internal visibility can be used.

Github Permalinks:

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L212-L223 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L273-L283 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L225-L271

Mitigation:

Adjust the code to the specifications.

Informational

Use of magic numbers is confusing

Summary:

Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.

Details:

Values are hardcoded and would be more readable and maintainable if declared as a constant

There are 2 cases that are commented in the functions, but they would be more readable with proper constant naming (for example: SIGNATURE_LENGTH or ADDRESS_LENGTH):

Signature length is used as a hardcoded number
Address length is used as a hardcoded number

Github Permalinks:

https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/libraries/BytesLibrary.sol#L39-L44 https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/libraries/BytesLibrary.sol#L25

Mitigation:

Replace magic hardcoded numbers with declared constants.

Missing indexed event parameters

Summary:

Events without indexed event parameters make it harder and inefficient for off-chain tools to analyze them.

Details:

Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.

Github Permalinks:

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L70 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L85

Mitigation:

Consider which event parameters could be particularly useful to off-chain tools and should be indexed.

Missing Natspec

Summary:

Missing Natspec and regular comments affect readability and maintainability of a codebase.

Details:

Contracts has partial or full lack of comments

Github Permalinks:

Some Natspec @params

_assumePrimarySale https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L83

Natspec missing (@param, @return...)

Same comments but different function name

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketSharedCore.sol#L24-L36

mitigation

Add @param descriptors Add @return descriptors Add Natspec comments. Add inline comments. Add comments for what the contract does

Bad order of code

Summary

Clearness of the code is important for the readability and maintainability. As Solidity guidelines says about declaration order: 1.Type declarations 2.State variables 3.Events 4.Modifiers 5.Functions Also is good to mention that:

state variables order affects to gas in the same way as ordering structs for saving storage slots.
the more used functions in the contract can save gas being ordered the first

Details

Modifier onlyAdmin is inserted after functions

github permalink

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/AdminRole.sol#L13-L21

Mitigation

Follow solidity style guidelines https://docs.soliditylang.org/en/v0.8.15/style-guide.html

Maximum line length exceeded

Summary:

Long lines should be wrapped to conform with Solidity Style guidelines.

Details:

Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Github Permalinks:

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L88-L89 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L84 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L62 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L46 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L13

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

Mitigation:

Comments and lines of code should be wrapped to a maximum of 79 (or 99) characters to help readers easily parse the comments.

#0 - HardlyDifficult
2022-08-18T15:29:20Z

Missing checks for address(0x0)

Invalid. If the address provided is address(0) then the if directly above this will revert.

Implementation of the comment spec is confusing

Invalid. There is a natspec comment already included that explains these odd functions and why it was done this way.

Use of magic numbers is confusing

Valid NC feedback, will fix. Both examples did have a comment explaining the number but a constant may be more clear.

Missing indexed event parameters

Invalid. index should be reserved for params that are likely to be requested as a filter. In these examples those params are data not really filter candidates. And for the string specifically, indexed would prevent the full information from being communicated, requiring a second unindexed version which is a bit redundant and increases gas costs.

Missing natspec comments

Fair feedback -- for natspec we aim for complete coverage of the public interfaces but for internal/private/libraries we have some gaps in order to reduce redundancy, for those we aim to include comments where things are unclear or not obvious from the function and variable names.

@inheritdoc is a natspec supported standard that effectively inlines comments from another file -- for those examples we should have complete coverage already.

Bad order of code

Agree, will move that modifier up.

Maximum line length exceeded

Fair however we use a limit of 120 instead. I believe this is more readable since it prevents excessive wrapping, particularly in comment. This is enforced by our linter.

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

61.2074 USDC - $61.21

Labels

bug

G (Gas Optimization)

old-submission-method

External Links

Link to GitHub issue

GAS

Constants expressions are expressions, not constants.

summary

Constant expressions are left as expressions, not constants.

details

Reference to this kind of issue: https://github.com/ethereum/solidity/issues/9232

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L70 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/MinterRole.sol#L19

mitigation

Use immutable for this expressions and set it on constructor

use of custom errors rather than revert() / require() error message

summary

Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.

details

Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L158 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L263 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L264 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L268 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L327

https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L173 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L182 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L203 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L227 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L262

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L93 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L131 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L172 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L179 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L238

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

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L58 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L63 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L74 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L87 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L89

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/AdminRole.sol#L19

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/MinterRole.sol#L22

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/ContractFactory.sol#L22 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/ContractFactory.sol#L31

mitigation

replace each error message in a require by a custom error

duplicated require() check should be refactored

summary

duplicated require() / revert() checks should be refactored to a modifier or function to save gas

details

Event appears twice and can be reduced

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L203 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L227

mitigation

refactor this checks to different functions to save gas

use != rather than >0 for unsigned integers in require() statements

Summary

When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When Using != , the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L131

mitigation

Use != 0 rather than > 0 for unsigned integers in require() statements.

Reduce the size of error messages (Long revert Strings)

summary

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L158 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L263 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L264 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L268 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L327

https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L173 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L182 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L203 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L227 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L262

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L93 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L131 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L172 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L179 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L238

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

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L58 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L63 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L74 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L87 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/collections/SequentialMintCollection.sol#L89

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/AdminRole.sol#L19

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/MinterRole.sol#L22

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/ContractFactory.sol#L22 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/ContractFactory.sol#L31

mitigation

Consider shortening the revert strings to fit in 32 bytes

Using bools for storage incurs overhead {

summary

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.

details

Here is one example of OpenZeppelin about this optimization 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) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L53 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L61

mitigation

Consider using uint256 with values 0 and 1 rather than bool

Store using Struct over multiple mappings

summary

All these variables could be combine in a Struct in order to reduce the gas cost.

details

As noticed in: https://gist.github.com/alexon1234/b101e3ac51bea3cbd9cf06f80eaa5bc2 When multiple mappings that access the same addresses, uints, etc, all of them can be mixed into an struct and then that data accessed like: mapping(datatype => newStructCreated) newStructMap; Also, you have this post where it explains the benefits of using Structs over mappings https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L55-L64

mitigation

Consider mixing different mappings into an struct when able in order to save gas.

Unused named returns

summary

Using both named returns and a return statement isn’t necessary. Removing one of those can improve code clarity

details

Also as returns variable is ignored, it wastes extra gas

github permalinks

mitigation

Remove return or returns when both used

Use calldata instead of memory for function parameters

Summary

It is generally cheaper to load variables directly from calldata, rather than copying them to memory.

Details

Only use memory if the variable needs to be modified

Github Permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTCollection.sol#L107-L108 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L371

Mitigation

Use calldata rather than memory in external functions where the parameter is not modified but only read

Using private rather than public for constants saves gas

summary

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

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L70 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/roles/MinterRole.sol#L19

mitigation

Consider replacing public for private in constants for gas saving.

Explicit initialization

summary

It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.

details

If a variable is not set/initialized, it is assumed to have the default value ( 0 for uint, false for bool, address(0) for address…).

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/mixins/shared/Constants.sol#L15

mitigation

Don't initialize variables to default value

Index initialized in for loop

summary

In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/libraries/BytesLibrary.sol#L25 https://github.com/code-423n4/2022-08-foundation/blob/b4e1dc6a9a434d576cb8f74676e7dbb65dacc98d/contracts/libraries/BytesLibrary.sol#L44 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L126 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L198 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L484

mitigation

Don't initialize variables to default value

++i costs less gas compared to i++

summary

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

details

i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

github permalinks

var++ https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L207 https://github.com/code-423n4/2022-08-foundation/blob/254d384116d22266b12916f1aeb46b9ef0143f1f/contracts/NFTCollectionFactory.sol#L231

mitigation

Replace to ++i.

increments can be unchecked in loops

summary

Unchecked operations as the ++i on for loops are cheaper than checked one.

details

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..

The code would go from:

for (uint256 i; i < numIterations; i++) {
// ...
}

to

for (uint256 i; i < numIterations;) {
  // ...
  unchecked { ++i; }
}
The risk of overflow is inexistent for a uint256 here.

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L198-L200

mitigation

Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header

<array>.length should no be looked up in every loop of a for-loop

summary

In loops not assigning the length to a variable so memory accessed a lot (caching local variables)

details

The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas)

github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L126 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L198 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L484 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L503

mitigation

Assign the length of the array.length to a local variable in loops for gas savings

Variables should be cached when used several times

summary

Variables read more than once improves gas usage when cached into local variable

details

NOTE: In loops or state variables, this is even more gas saving

github permalinks

latestTokenId https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L181

creatorShares[i] https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L129-L134 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L485-L490

mitigation

Cache variables used more than one into a local variable.

>= cheaper than >

Summary

Strict inequalities ( > ) are more expensive than non-strict ones ( >= ). This is due to some supplementary checks (ISZERO, 3 gas)

Github permalinks

https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L88 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L130 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/NFTDropCollection.sol#L131 https://github.com/code-423n4/2022-08-foundation/blob/7d6392498e8f3b8cdc22beb582188ffb3ed25790/contracts/mixins/shared/MarketFees.sol#L244

mitigation

Consider using >= 1 instead of > 0 to avoid some opcodes

#0 - HardlyDifficult
2022-08-19T15:31:07Z

constants expressions are expressions, not constants

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

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.

duplicated require() check should be refactored

Agree, will consider changing here.

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

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.

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.

Store using Struct over multiple mappings

Potentially valid but these variables support different use cases, since they are not read together it may not be beneficial.

Not using the named return variables when a function returns, wastes deployment gas

Agree for code consistency with other parts of our code. Saves 0.013 bytes on the bytecode size.

calldata

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

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.

Don't initialize variables with default values.

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

++i costs less than i++

Agree and will fix.

unchecked loop in getFeesAndRecipients

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.

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.

Variables should be cached when used several times

Agree, will fix.

= cheaper than >

Invalid. This rec changes logic and violates requirements.

Foundation Drop contest - Deivitto's results

Foundation is a web3 destination.

General Information

Findings Distribution

Researcher Performance

External Links

[M-07] _safeMint() should be used rather than _mint() wherever possible - $13.17

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

_safeMint() should be used rather than _mint() wherever possible

Impact

Details

References

Github Permalinks

Mitigation

#0 - HardlyDifficult2022-08-18T12:18:32Z

[Q-28] QA Report - $43.10

Findings Information

Awards

Labels

External Links

QA

Low

Missing checks for address(0x0) when assigning values to address state variables

Summary

Github Permalinks

Mitigation

Implementation of the comment spec is confusing

Summary:

Github Permalinks:

Mitigation:

Informational

Use of magic numbers is confusing

Summary:

Details:

Github Permalinks:

Mitigation:

Missing indexed event parameters

Summary:

Details:

Github Permalinks:

Mitigation:

Missing Natspec

Summary:

Details:

Github Permalinks:

Some Natspec @params

Natspec missing (@param, @return...)

More documentation needed

Same comments but different function name

mitigation

Bad order of code

Summary

Details

github permalink

Mitigation

Maximum line length exceeded

Summary:

Details:

Github Permalinks:

Mitigation:

#0 - HardlyDifficult2022-08-18T15:29:20Z

[G-24] Gas Report - $61.21

Findings Information

Awards

Labels

External Links

GAS

Constants expressions are expressions, not constants.

summary

details

github permalinks

mitigation

use of custom errors rather than revert() / require() error message

summary

details

#0 - HardlyDifficult
2022-08-18T12:18:32Z

#0 - HardlyDifficult
2022-08-18T15:29:20Z