QA Report

[code423n4]

1. Excess ether sent to FixedPricePassThruGate is lost (low)

passThruGate() redirects to a beneficiary only gate.ethCost, requiring that msg.value >= gate.ethCost. As there are no other ways to access native tokens held by this contract, the cumulative excess value, a sum of msg.value - gate.ethCost, will be permanently frozen within the contract.

Proof of Concept

Only gate.ethCost is now forwarded:

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L53-L53

(bool sent, bytes memory data) = gate.beneficiary.call{value: gate.ethCost}("");

Recommended Mitigation Steps

Pass-though the whole msg.value as the excess is not used:

(bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}("");

2. One step owner change (non-critical)

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

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

    /// @notice Changing the owner key
    /// @dev Only current owner may do this
    /// @param newOwner the new address that will be owner, old address is no longer owner
    function setOwner(address newOwner) external ownerOnly {
        address oldOwner = _owner_;
        _owner_ = newOwner;
        emit OwnerUpdated(oldOwner, newOwner);
    }

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

    /// @notice Change the management key
    /// @dev Only the current management key can change this
    /// @param newMgmt the new management key
    function setManagement(address newMgmt) external managementOnly {
        management = newMgmt;
    }

Recommended Mitigation Steps

Consider utilizing two-step owner rights transferring process (proposition and acceptance in the separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system.

3. No zero address checks in constructors (non-critical)

Functionality of the system will be unavailable if core config addresses be set to zero.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L50-L55

    /// @notice Whoever deploys the contract sets the two privileged keys
    /// @param _mgmt key that will initially be both management and treeAdder
    constructor(address _mgmt) {
        management = _mgmt;
        treeAdder = _mgmt;
    }

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L33-L37

    /// @notice Deployer connects it to MerkleIdentity
    /// @param _gateMaster address of MerkleIdentity contract, which has exclusive right to call passThruGate
    constructor(address _gateMaster) {
        gateMaster = _gateMaster;
    }

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L102-L114

    /// @notice Whoever deploys the contract determines the name, symbol and owner. Minter should be MerkleIdentity contract
    /// @dev names are misspelled on purpose because we already have owners and _owner_ and _name and...
    /// @param ooner the owner of this contract
    /// @param minter address (MerkleIdentity contract) that can mint NFTs in this series
    /// @param nomen name of the NFT series
    /// @param symbowl symbol for the NFT series
    constructor(address ooner, address minter, string memory nomen, string memory symbowl) {
        _owner_ = ooner;
        // we set it here with no resetting allowed so we cannot commit to NFTs and then reset
        _minter = minter;
        _name = nomen;
        _symbol = symbowl;
    }

Recommended Mitigation Steps

Consider adding zero address checks as operation mistakes mitigation tends to out-weight gas optimization in such core config one time setups.

[illuzen]

all duplicates

[KenzoAgada]

Issue 1 should be upgraded to high, duplicate of H-01 #48.

[gititGoro]

Thanks, @KenzoAgada. Good catch.