FactoryDAO contest - WatchPug's results

Lines of code

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

Vulnerability details

In FixedPricePassThruGate.sol#passThruGate(), at L48 the msg.value is checked to be >= gate.ethCost instead of == gate.ethCost, which makes it possible for the caller to send more than gate.ethCost.

However, at L53 only the amount of gate.ethCost is passed thru to gate.beneficiary instead of all the msg.value.

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

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');
    }
}

As a result, any surplus funds sent by the caller will be stuck in the contract forever, and there is no way for anyone to retrieve them.

Recommendation

Change to:

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

See also: the implementation of SpeedBumpPriceGate.sol correctly forwarded all the msg.value.

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L65-L82

function passThruGate(uint index, address) override external payable {
    uint price = getCost(index);
    require(msg.value >= price, 'Please send more ETH');

    // bump up the price
    Gate storage gate = gates[index];
    // multiply by the price increase factor
    gate.lastPrice = (price * gate.priceIncreaseFactor) / gate.priceIncreaseDenominator;
    // move up the reference
    gate.lastPurchaseBlock = block.number;

    // pass thru the 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: msg.value}("");
        require(sent, 'ETH transfer failed');
    }
}

Lines of code

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

Vulnerability details

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

    gate.priceIncreaseDenominator = priceIncreaseDenominator;

If priceIncreaseDenominator is set to 0 when addGate(), in passThruGate() the tx will revert at L72 because of div by 0.

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L71-L72

    // multiply by the price increase factor
    gate.lastPrice = (price * gate.priceIncreaseFactor) / gate.priceIncreaseDenominator;

Recommendation

Consider adding a check in addGate() to require priceIncreaseDenominator > 0.