HardlyDifficult
commented
2 years ago Missing checks for address(0x0)
Invalid. The isContract check directly before this line will revert on address(0).
Use named returns consistently
Agree, we have opted to use the named returns instead of return ... This is more consistent with other code in our repo and saves a bit of on the contract size. We also like named returns as a way of improving natspec, and typechain (particularly when a tuple is returned).
Typos
Agree, thanks. Will fix.
Unresolved TODO comments
Agree, will fix.
Missing events on initialize
Disagree - but it's a valid point. For the collection creations that information is already emitted by the factory, including it here would be redundant. For versionNFTCollection that value will be emitted when the first template is assigned. I don't believe emitting for the ReentrancyGuard is helpful or necessary.
Emitting BuyReferralPaid after changes
Disagree, but it's subjective. Logically the buy referrer payment is _sendValue and then emit. The next line updating totalFees is for the larger function and not specific to the buy referrer payment action.
public functions should be external
Invalid. getTokenCreatorPaymentAddress, isAdmin and getFoundationTreasury are used internally. tokenURI was declared by an OZ dependency.
See the markdown file with the details of this report here.