Open code423n4 opened 2 years ago
Unresolved TODO comments
Agree, will fix.
Use safeMint
Agree will fix - for context see our response here.
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.
.MOVE REQUIRE CHECK TO TOP OF FUNCTION
Invalid(?) All the links are to requires at the top of functions. Not clear what's being suggested here.
1.OPEN TODOS
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment There is 1 instance of this issue
// Link to githubfile
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L193
2.USE _SAFEMINT() INSTEAD OF _MINT()
_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
Instances
//Links to github file https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L271 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L182
3.. USE OF FLOATING PRAGMA
Recommend using fixed solidity version.This is not a critical issue but this creates unsatbility to the program so having a fixed version reduces the unstability.
Instances
// Links to github file
https://github.com/code-423n4/2022-08-foundation All the conracts in scope are valid for this issue.
4.MOVE REQUIRE CHECK TO TOP OF FUNCTION
The require check in comes after several state changes. Consider moving it to the top of the function to follow the checks-effects-interactions pattern.
Instances
//Link to github file https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L262 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L158 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L238