if user call addMerkleTree() of MerkleIdentity with priceIndex==0 by mistake or other uninitialized gates, then attacker can steal all the NFTs with 0 payment

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L28-L33 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L37-L45 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L89-L107

Vulnerability details

Impact

If treeAdder calls addMerkleTree() with priceIndex==0 by mistake, then attackers can buy NFT's with price of 0 because gate Indexs in FixedPricePassThruGate or SpeedBumpPriceGate start from 1 and gates[0] will be uninitialized and getCost(0) will return 0. This will also happen if treeAdder use uninitialized priceIndex in calling addMerkleTree(), becase getCost(uninitialized index) will return 0 too and FixedPricePassThruGate and SpeedBumpPriceGate don't check that getCost(index) is initilized.

Proof of Concept

This is the code of getCost(index) in FixedPricePassThruGate or SpeedBumpPriceGate:

    function getCost(uint index) override public view returns (uint _ethCost) {
        Gate memory gate = gates[index];
        // compute the linear decay
        uint decay = gate.decayFactor * (block.number - gate.lastPurchaseBlock);
        // gate.lastPrice - decay < gate.priceFloor (left side could underflow)
        if (gate.lastPrice < decay + gate.priceFloor) {
            return gate.priceFloor;
        } else {
            return gate.lastPrice - decay;
        }
    }

    function getCost(uint index) override external view returns (uint _ethCost) {
        Gate memory gate = gates[index];
        return gate.ethCost;
    }

which is obvious that they both return 0 for uninitialized gates[index]. so if treeAdder make mistake in the value of priceIndex, then all the users can buy NFTs with 0 price and fund will be lost. This is the code of passThruGate() which is called in FixedPricePassThruGate:

    function passThruGate(uint index, address) override external payable {
        Gate memory gate = gates[index];
        require(msg.value >= gate.ethCost, 'Please send more ETH');

        // pass thru ether
        if (msg.value > 0) {
            // use .call so we can send to contracts, for example gnosis safe, re-entrance is not a threat here
            (bool sent, bytes memory data) = gate.beneficiary.call{value: gate.ethCost}("");
            require(sent, 'ETH transfer failed');
        }
    }

There is no check here for index and the code will not revert in case gates[index] is not initialized. (the passThruGate() in SpeedBumpPriceGate will revert because of division by zero)

Tools Used

VIM

Recommended Mitigation Steps

In addMerkleTree() check that priceIndex is initialized in Gate.

[illuzen]

Invalid, different indices are used for each gate and merkle identity, and in any case, argument entry error is not in scope, as it is a frontend issue

[gititGoro]

Marking as invalid.