Honest party's move could become invalid when re-org takes place

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol#L116 https://github.com/Vectorized/solady/blob/a95f6714868cfe5d590145f936d0661bddff40d2/src/utils/LibClone.sol#L458 https://github.com/code-423n4/2024-07-optimism/blob/70556044e5e080930f686c4e5acde420104bb2c4/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L319

Vulnerability details

Impact

When block re-org takes place, honest party's move could become invalid. A similar issue has been raised earlier, and this report shows two new scnearios, which the fix fails to address.

Proof of Concept

A new parameter _claim has been added to ensure the target is the expected one, but this is still not enough.

function move(Claim _disputed, uint256 _challengeIndex, Claim _claim, bool _isAttack) public payable virtual {

The factory uses create instead of create2 to deploy the proxy, consider the following scenario:

1. Evil Alice creates a invalid root claim A and game P.
2. Seemingly honest Bob attacks A with claim B in game P.
3. Seemingly honest Bob creates a valid root claim C and game Q.
4. Evil Alice attacks C with claim B(valid, same hash as step 2) in game Q.
5. Honest Coco defends B with claim D in game Q.

Alice and Bob are friends actually and they use another smart contract to interact with the factory and game. At the end of step 1 and 3, A,P and C,Q are stored in the contract, and in step 2 and 4, the contract executes P.move(A,...)and Q.move(C,...). Coco simply uses an eoa at step 5.

Then reorg happens, transaction order becomes 34125. The address of game P and Q are swapped (since the deployed address is derived from sender and nonce). Now it will look like

1. Seemingly honest Bob creates a valid root claim C and game Q.
2. Evil Alice attacks C with claim B(valid, same hash with step 4) in game Q.
3. Evil Alice creates a invalid root claim A and game P.
4. Seemingly honest Bob attacks A with claim B in game P.
5. Honest Coco defends B with claim D in game QP.

Alice and Bob are still in their desired game but Coco will be in a wrong game. As a result, her bond will be lost.

proxy_ = IDisputeGame(address(impl).clone(abi.encodePacked(msg.sender, _rootClaim, parentHash, _extraData)));
......
instance := create(value, add(m, add(0x0b, lt(n, 0xffd3))), add(n, 0x37))

Suppose create2 is used to address the previous scenario, but within the same game, reorg could still impact the honest party. Consider the following scenario.

1. Evil Alice creates an invalid root claim A.
2. Honest Coco attacks A with claim B.
3. Evil Alice attacks A with an invalid claim X.
4. Evil Alice attacks B with the valid claim C.
5. Honest Coco defends C(the claim created in step 4) with claim D.
6. Evil Alice attacks A with an invalid claim E.
7. Evil Alice attacks E with the valid claim C (same hash as step 4).

Now reorg happens again, new transaction order is 1267534. Alice uses the previously mentioned contract call trick(store claim and index this time) so that her txs do not revert. Then at step 5, Coco's tx will succeed since disputed hash is the same. But her defend will now imply C and E are both valid, so she'll lose her bond again.

Chain reorgs are very prevalent in Ethereum mainnet, we can check this index of reorged blocks on etherscan.

Recommended Mitigation Steps

1. Use cloneDeterministic instead.
2. claimHash(child) = keccak256(claim(child), position(child), claimHash(parent)); Then check claimHash is indeed the expected one.

Assessed type

Context

[Inphi]

This is valid. I'll note that, by default, the op-node trails behind L1 by 5 blocks. So a challenger using it as its source of outputs wouldn't accidentally move against the wrong claim/game.

[c4-judge]

zobront marked the issue as satisfactory

[c4-judge]

zobront marked the issue as selected for report

[0xEVom]

@zobront for context, as I'm sure you're aware, the decision to accept the referenced finding as a valid Med the the below reasoning was controversial.

This was initially deemed invalid by a strict interpretation of our judging guidelines ("Chain re-org and network liveness related issues are not considered valid."). This rule exists as the "blockchain is trusted" from the perspective of app builders. However, a different trust level applies when building an L1/L2.

In our opinion, this is not the case here because this code better fits the definition of an "app on L1" than "L2 core contracts on L1".

For this reason, we decided to include a reorg finding of ours in the QA report instead of submitting it as a separate finding, which we would like to ask to be assessed with the same severity as this (be it Med or Low). I believe it provides sufficient context despite its brevity, but let us know if you'd like us to provide more detail.

[xuwinnie]

@0xEVom

because the L2 state and correct output can change.

In the QA report there is not any explanation for the statement. You are simply stating “something could go wrong when reorg take place”, how could this be a medium? It's not brevity, it's called “insufficient proof” And you should reference #10 instead of this one

[zobront]

I am going to keep the originally judged severities here.

1) The QA report mentioned by @0xEVom is identical to @xuwinnie issue #10, which was downgraded to QA. This issue is different, as it points to something that could happen within the game, as opposed to just in the creation. The abilities for games to swap addresses (intentionally or not) and have moves on them persist on the wrong game is separate and more important.

2) While @0xEVom is right that the Sherlock decision was controversial, that is because Sherlock's rules explicitly rule out reorg issues, while this is not the case on C4.