search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

QA Report #207

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

Low Risk Issues

Issue Instances
[L‑01] Upgradeable contract not initialized 5
[L‑02] Open TODOs 1
[L‑03] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions 3

Total: 9 instances over 3 issues

Non-critical Issues

Issue Instances
[N‑01] Missing initializer modifier on constructor 4
[N‑02] constants should be defined rather than using magic numbers 5
[N‑03] Use a more recent version of solidity 9
[N‑04] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant 2
[N‑05] Constant redefined elsewhere 1
[N‑06] Non-library/interface files should use fixed compiler versions, not floating ones 4
[N‑07] NatSpec is incomplete 1
[N‑08] Event is missing indexed fields 4
[N‑09] Not using the named return variables anywhere in the function is confusing 11
[N‑10] Duplicated require()/revert() checks should be refactored to a modifier or function 1

Total: 42 instances over 10 issues

Low Risk Issues

[L‑01] Upgradeable contract not initialized

Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user

There are 5 instances of this issue:

File: contracts/NFTCollection.sol

/// @audit missing __ERC165_init()
28    contract NFTCollection is
29      INFTCollectionInitializer,
30      IGetRoyalties,
31      IGetFees,
32      IRoyaltyInfo,
33      ITokenCreator,
34      ContractFactory,
35      Initializable,
36      ERC165Upgradeable,
37      ERC721Upgradeable,
38      ERC721BurnableUpgradeable,
39      SequentialMintCollection,
40:     CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L28-L40

File: contracts/NFTDropCollection.sol

/// @audit missing __Context_init()
/// @audit missing __ERC165_init()
/// @audit missing __AccessControl_init()
28    contract NFTDropCollection is
29      INFTDropCollectionInitializer,
30      INFTDropCollectionMint,
31      IGetRoyalties,
32      IGetFees,
33      IRoyaltyInfo,
34      ITokenCreator,
35      ContractFactory,
36      Initializable,
37      ContextUpgradeable,
38      ERC165Upgradeable,
39      AccessControlUpgradeable,
40      AdminRole,
41      MinterRole,
42      ERC721Upgradeable,
43      ERC721BurnableUpgradeable,
44      SequentialMintCollection,
45:     CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L28-L45

File: contracts/NFTDropMarket.sol

/// @audit missing __ReentrancyGuard_init()
63    contract NFTDropMarket is
64      Initializable,
65      FoundationTreasuryNode,
66      FETHNode,
67      MarketSharedCore,
68      NFTDropMarketCore,
69      ReentrancyGuardUpgradeable,
70      SendValueWithFallbackWithdraw,
71      MarketFees,
72      Gap10000,
73:     NFTDropMarketFixedPriceSale

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L63-L73

[L‑02] Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There is 1 instance of this issue:

File: contracts/mixins/shared/MarketFees.sol

193:        // TODO add referral info

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L193

[L‑03] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

There are 3 instances of this issue:

File: contracts/NFTCollection.sol

28    contract NFTCollection is
29      INFTCollectionInitializer,
30      IGetRoyalties,
31      IGetFees,
32      IRoyaltyInfo,
33      ITokenCreator,
34      ContractFactory,
35      Initializable,
36      ERC165Upgradeable,
37      ERC721Upgradeable,
38      ERC721BurnableUpgradeable,
39      SequentialMintCollection,
40:     CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L28-L40

File: contracts/NFTDropCollection.sol

28    contract NFTDropCollection is
29      INFTDropCollectionInitializer,
30      INFTDropCollectionMint,
31      IGetRoyalties,
32      IGetFees,
33      IRoyaltyInfo,
34      ITokenCreator,
35      ContractFactory,
36      Initializable,
37      ContextUpgradeable,
38      ERC165Upgradeable,
39      AccessControlUpgradeable,
40      AdminRole,
41      MinterRole,
42      ERC721Upgradeable,
43      ERC721BurnableUpgradeable,
44      SequentialMintCollection,
45:     CollectionRoyalties

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L28-L45

File: contracts/NFTDropMarket.sol

63    contract NFTDropMarket is
64      Initializable,
65      FoundationTreasuryNode,
66      FETHNode,
67      MarketSharedCore,
68      NFTDropMarketCore,
69      ReentrancyGuardUpgradeable,
70      SendValueWithFallbackWithdraw,
71      MarketFees,
72      Gap10000,
73:     NFTDropMarketFixedPriceSale

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L63-L73

Non-critical Issues

[N‑01] Missing initializer modifier on constructor

OpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.

There are 4 instances of this issue:

File: contracts/NFTCollectionFactory.sol

181:    constructor(address _rolesContract) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L181

File: contracts/NFTCollection.sol

95      constructor(address _contractFactory)
96:       ContractFactory(_contractFactory) // solhint-disable-next-line no-empty-blocks

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L95-L96

File: contracts/NFTDropCollection.sol

101     constructor(address _contractFactory)
102:      ContractFactory(_contractFactory) // solhint-disable-next-line no-empty-blocks

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L101-L102

File: contracts/NFTDropMarket.sol

82      constructor(
83        address payable _treasury,
84        address _feth,
85        address _royaltyRegistry
86      )
87        FoundationTreasuryNode(_treasury)
88        FETHNode(_feth)
89        MarketFees(
90          _royaltyRegistry,
91          /*assumePrimarySale=*/
92          true
93:       ) // solhint-disable-next-line no-empty-blocks

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L82-L93

[N‑02] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 5 instances of this issue:

File: contracts/libraries/BytesLibrary.sol

/// @audit 20
25:         for (uint256 i = 0; i < 20; ++i) {

/// @audit 4
40:       if (callData.length < 4) {

/// @audit 4
44:         for (uint256 i = 0; i < 4; ++i) {

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

File: contracts/mixins/shared/Constants.sol

/// @audit 1000
26:   uint256 constant MIN_PERCENT_INCREMENT_DENOMINATOR = BASIS_POINTS / 1000;

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

File: contracts/NFTCollectionFactory.sol

/// @audit 0x1337000000000000000000000000000000000000000000000000000000001337
242:        0x1337000000000000000000000000000000000000000000000000000000001337,

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L242

[N‑03] Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

There are 9 instances of this issue:

File: contracts/libraries/AddressLibrary.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/libraries/AddressLibrary.sol#L3

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L3

File: contracts/mixins/shared/ContractFactory.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/ContractFactory.sol#L3

File: contracts/mixins/shared/FETHNode.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FETHNode.sol#L3

File: contracts/mixins/shared/FoundationTreasuryNode.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FoundationTreasuryNode.sol#L3

File: contracts/mixins/shared/MarketFees.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L3

File: contracts/NFTCollectionFactory.sol

42:   pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L42

File: contracts/NFTCollection.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L3

File: contracts/NFTDropCollection.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L3

[N‑04] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

While it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

There are 2 instances of this issue:

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

70:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L70

File: contracts/mixins/roles/MinterRole.sol

19:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/MinterRole.sol#L19

[N‑05] Constant redefined elsewhere

Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract's value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don't get out of sync.

There is 1 instance of this issue:

File: contracts/mixins/roles/MinterRole.sol

/// @audit seen in contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol 
19:     bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/roles/MinterRole.sol#L19

[N‑06] Non-library/interface files should use fixed compiler versions, not floating ones

There are 4 instances of this issue:

File: contracts/NFTCollectionFactory.sol

42:   pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L42

File: contracts/NFTCollection.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L3

File: contracts/NFTDropCollection.sol

3:    pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L3

File: contracts/NFTDropMarket.sol

42:   pragma solidity ^0.8.12;

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L42

[N‑07] NatSpec is incomplete

There is 1 instance of this issue:

File: contracts/mixins/shared/MarketFees.sol

/// @audit Missing: '@param _assumePrimarySale'
79      /**
80       * @notice Configures the registry allowing for royalty overrides to be defined.
81       * @param _royaltyRegistry The registry to use for royalty overrides.
82       */
83:     constructor(address _royaltyRegistry, bool _assumePrimarySale) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L79-L83

[N‑08] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

There are 4 instances of this issue:

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

79      event CreateFixedPriceSale(
80        address indexed nftContract,
81        address indexed seller,
82        uint256 price,
83        uint256 limitPerAccount
84:     );

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L79-L84

File: contracts/mixins/shared/MarketFees.sol

71      event BuyReferralPaid(
72        address indexed nftContract,
73        uint256 indexed tokenId,
74        address buyReferrer,
75        uint256 buyReferrerFee,
76        uint256 buyReferrerSellerFee
77:     );

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L71-L77

File: contracts/NFTCollection.sol

70:     event BaseURIUpdated(string baseURI);

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L70

File: contracts/NFTDropCollection.sol

85:     event URIUpdated(string baseURI, bytes32 postRevealBaseURIHash);

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L85

[N‑09] Not using the named return variables anywhere in the function is confusing

Consider changing the variable to be an unnamed one

There are 11 instances of this issue:

File: contracts/libraries/AddressLibrary.sol

/// @audit contractAddress
34      function callAndReturnContractAddress(CallWithoutValue memory call)
35        internal
36:       returns (address payable contractAddress)

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/libraries/AddressLibrary.sol#L34-L36

File: contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol

/// @audit numberThatCanBeMinted
227     function getAvailableCountFromFixedPriceSale(address nftContract, address user)
228       external
229       view
230:      returns (uint256 numberThatCanBeMinted)

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L227-L230

File: contracts/mixins/shared/FoundationTreasuryNode.sol

/// @audit treasuryAddress
59:     function getFoundationTreasury() public view returns (address payable treasuryAddress) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/FoundationTreasuryNode.sol#L59

File: contracts/mixins/shared/MarketFees.sol

/// @audit registry
208:    function getRoyaltyRegistry() external view returns (address registry) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L208

File: contracts/mixins/shared/MarketSharedCore.sol

/// @audit seller
20:     function getSellerOf(address nftContract, uint256 tokenId) external view returns (address payable seller) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketSharedCore.sol#L20

File: contracts/NFTCollectionFactory.sol

/// @audit collection
286     function createNFTDropCollection(
287       string calldata name,
288       string calldata symbol,
289       string calldata baseURI,
290       bytes32 postRevealBaseURIHash,
291       uint32 maxTokenId,
292       address approvedMinter,
293       uint256 nonce
294:    ) external returns (address collection) {

/// @audit collection
324     function createNFTDropCollectionWithPaymentAddress(
325       string calldata name,
326       string calldata symbol,
327       string calldata baseURI,
328       bytes32 postRevealBaseURIHash,
329       uint32 maxTokenId,
330       address approvedMinter,
331       uint256 nonce,
332       address payable paymentAddress
333:    ) external returns (address collection) {

/// @audit collection
363     function createNFTDropCollectionWithPaymentFactory(
364       string calldata name,
365       string calldata symbol,
366       string calldata baseURI,
367       bytes32 postRevealBaseURIHash,
368       uint32 maxTokenId,
369       address approvedMinter,
370       uint256 nonce,
371       CallWithoutValue memory paymentAddressFactoryCall
372:    ) external returns (address collection) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L286-L294

File: contracts/NFTDropCollection.sol

/// @audit uri
300:    function tokenURI(uint256 tokenId) public view override returns (string memory uri) {

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L300

File: contracts/NFTDropMarket.sol

/// @audit seller
108     function _getSellerOf(address nftContract, uint256 tokenId)
109       internal
110       view
111       override(MarketSharedCore, NFTDropMarketFixedPriceSale)
112:      returns (address payable seller)

/// @audit sellerOrOwner
132     function _getSellerOrOwnerOf(address nftContract, uint256 tokenId)
133       internal
134       view
135       override
136:      returns (address payable sellerOrOwner)

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropMarket.sol#L108-L112

[N‑10] Duplicated require()/revert() checks should be refactored to a modifier or function

The compiler will inline the function, which will avoid JUMP instructions usually associated with functions

There is 1 instance of this issue:

File: contracts/NFTCollectionFactory.sol

227:      require(_implementation.isContract(), "NFTCollectionFactory: Implementation is not a contract");

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollectionFactory.sol#L227

HardlyDifficult commented 2 years ago

Very detailed report - thank you.

Upgradeable contract not initialized

Fair feedback but I don't believe changes are required. In the first two contracts the examples listed are no-ops, but it could be argued calling them is good practice anyways. For the last one, the suggested call is already included.

Unresolved TODO comments

Agree, will fix.

[L‑03] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Invalid. The instances here are top level contracts, which do not require an explicit gap since their slots come after all inherited contracts. Also 2 of them, the collections, are proxies but not upgradable so gaps are not required at all (but some were included to encourage code reuse in our repo).

Use constructor to initialize templates

Agree this is a good best practice to add. Will fix.

Use of magic numbers is confusing

Valid NC feedback, will fix. The first two examples did have a comment explaining the number but a constant may be more clear. For MIN_PERCENT_INCREMENT_DENOMINATOR I think the existing comment is sufficient there. The last one is a special case where it really is just a magic / arbitrary value.

[N‑03] Use a more recent version of solidity

Invalid - this is a good consideration though. I confirmed our contracts compile with 0.8.12. The using for change in 0.8.13 was about global usings which do not apply to this repo.

[N‑04] Expressions for constant values such as a call to

Interesting point. I think we'll leave this as is since it does not save gas as you noted, and the OZ deployment helper throws an error with this change -- it may be nice to not suppress that just in case it provides valid feedback for a different issue later on.

[N‑05] Constant redefined elsewhere

Fair feedback but don't think we'll change here at this time. We inherit from OZ which has a public variable for DEFAULT_ADMIN_ROLE. For consistency we want MINTER_ROLE to be a getter as well, which would be lost if we use a library as suggested. An alt may be to have a shared constant and then include an explicit external getter separately..

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.

[N‑07] NatSpec is incomplete

Agree, fixed.

[N‑08] Event is missing indexed fields

BuyReferrerPaid: 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 the others 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.

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).

Use modifier for duplicated require()/revert()

Agree, will fix.