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
Rank: 72/108
Findings: 1
Award: $41.26
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
41.2563 USDC - $41.26
* @title A library for address helpers not already covered by the OZ library library.
Remove repeated word library
In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable, there are cases where very long comments interfere with readability. Below are several instances of long comments that could be improved by wrapping, as shown:
unchecked { // Add the referrer fee back into the total fees so that all 3 return fields sum to the total price for events totalFees += buyReferrerFee; }
Recommendation:
unchecked { // Add the referrer fee back into the total fees so that all 3 return fields // sum to the total price for events. totalFees += buyReferrerFee; }
Similarly for the following:
NFTCollectionFactory.sol: L35-36
/** * @notice Create a new collection contract. * @dev The nonce must be unique for the msg.sender + implementation version, otherwise this call will revert. * @param name The collection's `name`. * @param symbol The collection's `symbol`. * @param nonce An arbitrary value used to allow a creator to mint multiple collections with a counterfactual address. * @return collection The address of the newly created collection contract. */
Suggestion:
/** * @notice Create a new collection contract. * @dev The nonce must be unique for the msg.sender + implementation version, * otherwise this call will revert. * @param name The collection's `name`. * @param symbol The collection's `symbol`. * @param nonce An arbitrary value used to allow a creator to mint * multiple collections with a counterfactual address. * @return collection The address of the newly created collection contract. */
Similarly for the following:
NFTCollectionFactory.sol: L272-285
NFTCollectionFactory.sol: L308-323
NFTCollectionFactory.sol: L347-362
NFTCollectionFactory.sol: L425-431
NFTCollectionFactory.sol: L436-443
NFTCollectionFactory.sol: L57-61
/** * @title A factory to create NFT collections. * @notice Call this factory to create NFT collections. * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a NFT collection contract implementation. */
Suggestion:
/** * @title A factory to create NFT collections. * @notice Call this factory to create NFT collections. * @dev This creates and initializes an ERC-1165 minimal proxy pointing to a * NFT collection contract implementation. */
Comments that contain TODOs or other language indicating open items should be addressed and modified or removed. Below are two instances:
// TODO add referral info
/* Overrides must support ERC-165 when registered, except for overrides defined by the registry owner. If that results in an override w/o 165 we may need to upgrade the market to support or ignore that override. */
@param
statement/** * @notice Configures the registry allowing for royalty overrides to be defined. * @param _royaltyRegistry The registry to use for royalty overrides. */ constructor(address _royaltyRegistry, bool _assumePrimarySale) { if (!_royaltyRegistry.supportsInterface(type(IRoyaltyRegistry).interfaceId)) { revert NFTMarketFees_Address_Does_Not_Support_IRoyaltyRegistry(); } royaltyRegistry = IRoyaltyRegistry(_royaltyRegistry); assumePrimarySale = _assumePrimarySale; // In the constructor, `this` refers to the implementation address. Everywhere else it'll be the proxy. implementationAddress = this; }
@param
statement is missing for _assumePrimarySale
#0 - HardlyDifficult
2022-08-18T17:07:01Z
Typo
Agree, will fix.
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.
Unresolved TODO comments
Agree, will fix.
Missing param
Agree, will fix.