InitialETHCrowdfund instances are vulnerable to reorgs

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/CrowdfundFactory.sol#L198-L198
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/crowdfund/InitialETHCrowdfund.sol#L115
https://github.com/code-423n4/2023-10-party/blob/b23c65d62a20921c709582b0b76b387f2bb9ebb5/contracts/party/PartyFactory.sol#L51

Vulnerability details

Impact

Funds that have been sent for a crowdfund might be used by someone else

Proof of Concept

InitialETHCrowdfund instances are created by the CrowdfundFactory contract using the Clones library from OpenZeppelin.

As we can see here, these InitialETHCrowdfund proxies are created with the clone() function of the library.

    inst = InitialETHCrowdfund(address(crowdfundImpl).clone());

Now, let's check the clone() function in the OpenZeppelin Clones library.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/74016c376ad7c1a902a7bef6803090f68f2433fb/contracts/proxy/Clones.sol#L36

    function clone(address implementation) internal returns (address instance) {
        /// @solidity memory-safe-assembly
        assembly {
            // Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
            // of the `implementation` address with the bytecode before the address.
            mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
            // Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
            mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
-->         instance := create(0, 0x09, 0x37)
        }
        if (instance == address(0)) {
            revert ERC1167FailedCreateClone();
        }
    }

As we can see here, it uses create opcode, which means the address of the clone only depends on the nonce of the CrowdfundFactory contract. It doesn't take a salt value as create2 (it is used in cloneDeterministic function).

This is a problem because it makes these clones vulnerable to reorgs.
Reorgs might happen on any EVM chain, and you can see the latest reorgs in the Ethereum chain here in this etherscan link.
Most of these reorgs are only with a depth of 1 block. More deep reorgs are not that common but they can still occur. The biggest one was a little more than 1 year ago with 7 block depth. (https://cointelegraph.com/news/ethereum-beacon-chain-experiences-7-block-reorg-what-s-going-on)

1. Alice creates a crowdfund

2. Alice sends ETH to the crowdfund address

3. Bob sees that there is a network reorg and makes a call to create a crowdfund with different authority parameters.

4. The crowdfund instance with Bob's authority will be created at the same address

5. Bob's crowdfund will have the Alice's ETH.

Similar submission related to reorgs due to using create can be seen here: code-423n4/2023-04-frankencoin-findings#155

Tools Used

Manual review

Recommended Mitigation Steps

I would recommend deploying cloned instances with create2 with a specific salt that includes the msg.sender

Assessed type

Error

[ydspa]

QA: L

[c4-pre-sort]

ydspa marked the issue as insufficient quality report

[gzeon-c4]

The seems valid to me, pool snipping bot can also be frontran with a malicious crowdfund deployment. @0xble

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-sponsor]

0xble (sponsor) acknowledged

[stalinMacias]

@gzeon-c4 Can I ask why is this issue considered a medium when in the previous party contest a similar issue about reorgs was judged as a QA?

[osmanozdemir1]

Hi @stalinMacias, thanks for the comment and thanks @gzeon-c4 for judging this contest.

I would like to share some historical data related to reorg issues.

August 2023 - PoolTogether: code-423n4/2023-08-pooltogether-findings#31
July 2023 - PoolTogether: code-423n4/2023-07-pooltogether-findings#416
May 2023 - Maia: code-423n4/2023-05-maia-findings#861
April 2023 - Frankencoin: code-423n4/2023-04-frankencoin-findings#155
January 2023 - RabbitHole: code-423n4/2023-01-rabbithole-findings#661

As we can see above, all of these reorg issues related to create opcode are judged as medium severity.

If there is an inconsistency in the judging, it is clear that the judging in the previous contest was the inconsistent one compared to historical judgements, not the current decision.

Kind regards,
Osman

[stalinMacias]

@osmanozdemir1 Those protocols were to be deployed on chains were reorgs are most likely to happen and don't necessarily require attackers to spend their money for the reorg to occur.

In this protocol, the contracts are being deployed to mainnet, where you've clearly mentioned that reorgs are costly and require the attackers to spend their assets to force the reorg, reorgs in mainnet don't happen because of the validators, as opposed to in other networks where the validators themselves can just do a reorg without requiring them to put their stake at risk (Polygon like example)

The finding I'm referring to is specifically for this protocol, so, if the same finding was judged as such severity in a previous contest, would not make sense to judge other similar reports exactly as how it was judged before?

[gzeon-c4]

Since sponsor acknowledged a very similar issue in a previous contest, this will be out-of-scope.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Out of scope