QA Report

[code423n4]

Table of Contents:

• [L-01] Missing address(0) checks
• [L-02] Comment says "Internal" instead of "Private"
• [L-03] Missing events for critical parameters that change the state
• [N-01] Unused named returns
• [N-02] The pragmas used are not the same everywhere

[L-01] Missing address(0) checks

Consider adding an address(0) check for addresses that are never changed (they should be immutable and this was mentioned in the gas report). See @audit tags:

File: MerkleEligibility.sol
35:     constructor(address _gateMaster) {
36:         gateMaster = _gateMaster; //@audit missing address(0) check
37:     }

File: PermissionlessBasicPoolFactory.sol
75:     constructor(address _globalBeneficiary, uint _globalTaxPerCapita) {
76:         globalBeneficiary = _globalBeneficiary; //@audit missing address(0) check
77:         globalTaxPerCapita = _globalTaxPerCapita;
78:     }

File: VoterID.sol
108:     constructor(address ooner, address minter, string memory nomen, string memory symbowl) {
109:         _owner_ = ooner;
110:         // we set it here with no resetting allowed so we cannot commit to NFTs and then reset
111:         _minter = minter; //@audit missing address(0) check
112:         _name = nomen;
113:         _symbol = symbowl;
114:     }

[L-02] Comment says "Internal" instead of "Private"

File: VoterID.sol
352:      * @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address. //@audit should say "Private" instead of "Internal"
...
361:     function checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory data)
362:         private returns (bool)
363:     {

[L-03] Missing events for critical parameters that change the state

This should emit an event:

File: PermissionlessBasicPoolFactory.sol
314:     function setGlobalTax(uint newTaxPerCapita) external {
315:         require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');
316:         require(newTaxPerCapita < 1000, 'Tax too high');
317:         globalTaxPerCapita = newTaxPerCapita; //@audit low: should emit an event
318:     }

[N-01] Unused named returns

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

contracts/FixedPricePassThruGate.sol:
  38:     function getCost(uint index) override external view returns (uint _ethCost) {

contracts/MerkleEligibility.sol:
  45:     function addGate(bytes32 merkleRoot, uint maxWithdrawalsAddress, uint maxWithdrawalsTotal) external returns (uint index) {
  70:     function isEligible(uint index, address recipient, bytes32[] memory proof) public override view returns (bool eligible) {

contracts/SpeedBumpPriceGate.sol:
  50:     function getCost(uint index) override public view returns (uint _ethCost) {

[N-02] The pragmas used are not the same everywhere

Consider choosing one version between solidity 0.8.9 (most contracts) and 0.8.12 (PermissionlessBasicPoolFactory.sol)

[illuzen]

all duplicates

[gititGoro]

L-01 is invalid. Constructor zero checks are not always mandatory or desirable.