Open code423n4 opened 2 years ago
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.
Use safeMint
Agree will fix - for context see our response here.
BuyReferralPaid event should index buyReferrer
Agree but won't fix at this time. We have already started emitting this event on mainnet, to simplify integrations such as subgraph we are going to continue with this event as is. However it's a good catch and I agree that buyReferrer ideally would have been indexed here.
For CreateFixedPriceSale
I believe this is 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.
1. use of floating pragma
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
NFTCollectionFactory.sol
NFTDropCollection.sol
NFTCollection.sol
NFTDropMarket.sol
NFTDropMarketCore.sol
CollectionRoyalties.sol
NFTDropMarketFixedPriceSale.sol
AdminRole.sol
MinterRole.sol
SequentialMintCollection.sol
ContractFactory.sol
FETHNode.sol
ContractFactory.sol
FoundationTreasuryNode.sol
MarketFees.sol
MarketSharedCore.sol
2. _safemint() should be used rather than _mint() wherever possible
NFTCollection.sol#L130
NFTCollection.sol#L143
NFTCollection.sol#L159
NFTCollection.sol#L271
NFTDropCollection.sol#L182
3. event is missing indexed fields
Each event should use three indexed fields if there are three or more fields
NFTDropMarketFixedPriceSale.sol#L79
MarketFees.sol#L71