Foundation Drop contest - bobirichman's results

[LOW] Add timelock for the following functions

Fair point and something we'll consider in the future.

[LOW] Payable functions that should not be payable

Invalid. These are payable by design and require it.

[LOW] Not verified input

Invalid. All those examples confirm .isContract - it's not clear what else you are suggesting here.

[LOW] Missing nonReentrancy modifier

Without a POC on a potential risk, we don't believe that a reentrancy guard is required here.

[LOW] The project is compiled with different solidity versions

Invalid, all contracts are compiled with 0.8.16

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.

[NON CRITICAL] Consider emitting an event at the following functions

I don't believe events are required for these constructors.

[NON CRITICAL] Missing function spec comments

invalid. FETH & PercentSplitETH were out of scope. The other example has a full natspec included already.

[NON CRITICAL] The following events are not indexed

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.

[NON CRITICAL] Unused function parameters should have name removed

Invalid - these params are required here due to the inheritance used.

[GAS] Do not cache msg.sender since loading msg.sender is more efficient than a local variable

Invalid. None of those links are using msg.sender.

[GAS] In the following revert statements consider using custom error instead a message

Agree but won't fix at this time. We use these in the market but not in collections. Unfortunately custom errors are still not as good of an experience for users (e.g. on etherscan). We used them in the market originally because we were nearing the max contract size limit and this was a good way to reduce the bytecode. We'll consider this in the future as tooling continues to improve.

[GAS] Mark as payable If has onlyOwner modifier

Disagree. This could save gas but 1) this seems like a poor practice to encourage and could be error prone. and 2) we don't care about optimizing admin-only actions.

[GAS] Cache array size

May be theoretically valid, but won't fix. I tested this: gas-reporter and our gas-stories suite is reporting a small regression using this technique. It also hurts readability a bit so we wouldn't want to include it unless it was a clear win.

[GAS] Use assembly opcodes iszero in the following locations

Won't fix. While this may save a tiny bit of gas, we reserve the use of assembly to areas where it would provide significant benefit or accomplish something that's otherwise not possible with Solidity alone.

[GAS] Use abiEncodePacked()

Invalid... while it seems like this should help, changing to .encode makes creating collections slightly more expensive. createNFTCollection increased by 9 gas and createNFTDropCollection increased by 8-10 gas

[GAS] Use > instead != to compare uint with 0

Invalid. We tested the recommendation and got the following results:

createNFTDropCollection gas reporter results:
  using > 0 (current):
    - 319246  ·     319578  ·      319361
  using != 0 (recommendation):
    -  319252  ·     319584  ·      319367
  impact: +6 gas