QA Report

[code423n4]

1. Missing zero address checks

Risk

Low

Impact

FactoryDAO's contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality

Proof of Concept

MerkleDropFactory.sol:

• Missing check for tokenAddress address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L49
• Missing check for destination address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L88

MerkleResistor.sol:

• Missing check for tokenAddress address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L80

MerkleVesting.sol:

• Missing check for tokenAddress address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L62

MerkleIdentity.sol:

• Missing check for _mgmt address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L52
• Missing check for newMgmt address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L60
• Missing check for newAdder address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L67
• Missing checks for nftAddress, priceGateAddress, eligibilityAddress addresses - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L92-L94

MerkleEligibility.sol:

• Missing check _gateMaster address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L35

FixedPricePassThruGate.sol:

• Missing check for _beneficiary address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L28 SpeedBumpPriceGate.sol:
• Missing check beneficiary address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L37

VoterID.sol:

• Missing check for ooner and minter addresses - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L108
• Missing check for thisOwner address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L122
• Missing check for newOwner address - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L151

PermissionlessBasicPoolFactory.sol:

• Missing check for _globalBeneficiary address - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L75
• Missing check for depositTokenAddress address - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L97
• Missing check for excessBeneficiary address - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L98
• Missing check for rewardTokenAddresses addresses - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L116

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add zero address checks for listed parameters.

2. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

MerkleIdentity.sol:

• Missing setManagement event - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L60-L62
• Missing setTreeAdder event - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L67
• Missing setIpfsHash event - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L75
• Missing withdraw event - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L116-L135

MerkleEligibility.sol:

• Missing event for addGate - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L45-L51

FixedPricePassThruGate.sol:

• Missing event for addGate - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L28-L33

SpeedBumpPriceGate.sol:

• Missing event for addGate - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L37-L45

PermissionlessBasicPoolFactory.sol:

• Missing event for withdrawTaxes - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L261
• Missing event for setGlobalTax - https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L314

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

3. Use SafeERC20 safeTransfer

Risk

Low

Impact

The IERC20.transfer() function returns a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Furthermore, tokens that do not correctly implement the EIP20 standard, like USDT which does not return a success boolean, will revert.

Proof of Concept

MerkleVesting.sol:

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L173

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use OpenZeppelin's SafeERC20 with the safeTransfer function that handles the return value check as well as non-standard-compliant tokens.

4. Not following checks-effects-interactions pattern

Risk

Low

Impact

Function PermissionlessBasicPoolFactory.fundPool does not follow checks-effects-interactions pattern that might lead to reentrancy attacks.

    success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
    // bookkeeping to make sure pools don't share tokens
    pool.rewardFunding[i] += amount;

Proof of Concept

PermissionlessBasicPoolFactory.sol

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L144-L146

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to first set the effects and then perform interactions such as external calls.

5. Owner - critical address change

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

MerkleIdentity.sol:

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L60-L62

VoterID.sol:

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L151-L155

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for passing ownership for HolyPaladinToken.sol and PaladinRewardReserve.sol contracts.

6. Missing validation

Risk

Low

Impact

Missing check for _globalTaxPerCapita that should not exceeds 1000.

Proof of Concept

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L75-L78

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add check for _globalTaxPerCapita to make sure that the value cannot be bigger than 1000.

7. VoterID tokenURI does not revert for non existent tokens

Risk

Non-Critical

Impact

Function VoterID.tokenURI that implements ERC721 standard should revert in case of non existent tokens.

Proof of Concept

• https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L411-L413

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to revert for non existent tokens.

8. Contracts use different compiler versions

Risk

Non-Critical

Impact

Using different compiler versions across contracts of the same project might lead to confusion and accidental errors.

Proof of Concept

Examples:

• 0.8.12 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L3
• 0.8.9 - https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L3

Tools Used

Manual Review / VS Code

Recommended Mitigation Steps

Consider using a single compiler version for compiling both contracts, for example 0.8.12.

[illuzen]

all duplicates