QA Report

[code423n4]

QA

1. QA
   1. Reentrancy can change metadata
   2. Unchecked ERC20 Transfer
   3. missing zero-check
   4. Missing Event
   5. Inconsistent Solidity pragma version
   6. Typo ooner
   7. addPool require() should be on top of function
   8. MerkleResistor.initialize unnecessary variable destination
   9. Confusing message error

Most of issues caught by Slither filter by me, while keep the original format. Beware of blue formated text "<text>" as low (or med) issue should be fixed, everything else is optional as non-critical.

If Github format does not work correctly. Just use search for "<>" symbol to find issue should be fixed.

Reentrancy can change metadata

PermissionlessBasicPoolFactory.addPool reentrancy through fundPool function. Can overwritten metadata for same pool (id based on ++numPools) multiple time.

Can cause transaction emit event in wrong order with pool ID and name mismatch with stored metadata.

Only cause issue with frontend, database or tools based on transaction events instead of reading metadata from blockchain directly.

Unchecked ERC20 Transfer

<MerkleVesting.withdraw>(uint256,address) (MerkleVesting.sol#139-175) ignores return value by IERC20(tree.tokenAddress).transfer(destination,currentWithdrawal) (MerkleVesting.sol#173)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-transfer

Interface call still revert if external contract call revert, but who known what weird token user want. It is recommended to use library SafeTransfer instead of interface call.

missing zero-check

MerkleEligibility.constructor(address)._gateMaster (MerkleEligibility.sol#35) lacks a zero-check on :
  - gateMaster = _gateMaster (MerkleEligibility.sol#36)
MerkleIdentity.constructor(address)._mgmt (MerkleIdentity.sol#52) lacks a zero-check on :
  - management = _mgmt (MerkleIdentity.sol#53)
  - treeAdder = _mgmt (MerkleIdentity.sol#54)
<MerkleIdentity.setManagement>(address).newMgmt (MerkleIdentity.sol#60) lacks a zero-check on :
  - management = newMgmt (MerkleIdentity.sol#61)
MerkleIdentity.setTreeAdder(address).newAdder (MerkleIdentity.sol#67) lacks a zero-check on :
  - treeAdder = newAdder (MerkleIdentity.sol#68)
PermissionlessBasicPoolFactory.constructor(address,uint256)._globalBeneficiary (PermissionlessBasicPoolFactory.sol#77) lacks a zero-check on :
  - globalBeneficiary = _globalBeneficiary (PermissionlessBasicPoolFactory.sol#78)
VoterID.constructor(address,address,string,string).ooner (VoterID.sol#108) lacks a zero-check on :
  - _owner_ = ooner (VoterID.sol#109)
VoterID.constructor(address,address,string,string).minter (VoterID.sol#108) lacks a zero-check on :
  - _minter = minter (VoterID.sol#111)
<VoterID.setOwner>(address).newOwner (VoterID.sol#151) lacks a zero-check on :
  - _owner_ = newOwner (VoterID.sol#153)

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-zero-address-validation

Changing owner and management can have accident with javascript where the address is not properly validated (use null address).

Missing Event

<MerkleIdentity.setManagement>(address) (MerkleIdentity.sol#60-62) should emit an event for: 
 - management = newMgmt (MerkleIdentity.sol#61) 
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-access-control

<PermissionlessBasicPoolFactory.setGlobalTax>(uint256) (PermissionlessBasicPoolFactory.sol#316-320) should emit an event for: 
 - globalTaxPerCapita = newTaxPerCapita (PermissionlessBasicPoolFactory.sol#319) 
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#missing-events-arithmetic

Other external function missing events not picked up by slither include:

• MerkleIdentity.setTreeAdder(address)
• MerkleIdentity.setIpfsHash(uint,bytes32)
• MerkleIdentity.withdraw(uint,uint,string,bytes32[],bytes32[]) should at least emit final event
• FixedPricePassThruGate.PassThruGate(uint,address)
• SpeedBumpPriceGate.PassThruGate(uint,address)
• MerkleEligibility.(uint,address,bytes32[])

Inconsistent Solidity pragma version

PermissionlessBasicPoolFactory.sol use 0.8.12 while other file use 0.8.9. Current recommended version still 0.8.4 - 0.8.7

Typo ooner

should be owner

addPool require() should be on top of function

User call addPool call lots of unnecessary code before reaching the revert point. It also save gas for user too.

MerkleResistor.initialize unnecessary variable destination

MerkleResistor require destination = msg.sender on top of function. There is no interface require destination for function. Why not just use msg.sender?

Confusing message error

The message does not tell user/dev do the positive or opposite action of the message.

[illuzen]

all duplicates