Upgrades to EdgeChallengeManager could `DoS the assertion chain` temporarily until admin unstuck it by forcing assertion

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupUserLogic.sol#L115-L128

Vulnerability details

Description

EdgeChallengeManager (a.k.a. ECM) can be upgraded during the life cycle of a rollup and this is officialized by an Admin calling RollupAdminLogic::setChallengeManager.

Let say we have the following assertion chain setup, so 3 assertions successfully confirmed, but then the 4th one, we have a divergence, and only the assertion winning the challenge will be able to be confirmed.

                  /--- D(ECM1) - bad
     A --- B --- C    
     |     |     |\--- E(ECM2) - winner (edge confirmed)
    ECM1  ECM1  ECM1 

The problem arise with the following code from RollupUserLogic::confirmAssertion, the implementation assume that the ECM instance to retrieve the winningEdge information is the same as the previous assertion, but as you can see in my setup that is not guaranteed. So while indeed the assertion E is the winner (edge confirmed, but in ECM2 instance, not ECM1), the code here will not find the edge information and will always revert, so the assertion chain will be stuck. The team will be able to resolve the problem by pausing the rollup and calling RollupAdminLogic::forceConfirmAssertion. This seems unexpected and seem to warrant Medium severity.

        if (prevAssertion.secondChildBlock > 0) {
            // if the prev has more than 1 child, check if this assertion is the challenge winner
            ChallengeEdge memory winningEdge = IEdgeChallengeManager(prevConfig.challengeManager).getEdge(winningEdgeId);     //<----- THIS: reading the ECM from prevConfig
            require(winningEdge.claimId == assertionHash, "NOT_WINNER");
            require(winningEdge.status == EdgeStatus.Confirmed, "EDGE_NOT_CONFIRMED");
            require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK");
            // an additional number of blocks is added to ensure that the result of the challenge is widely
            // observable before it causes an assertion to be confirmed. After a winning edge is found, it will
            // always be challengeGracePeriodBlocks before an assertion can be confirmed
            require(
                block.number >= winningEdge.confirmedAtBlock + challengeGracePeriodBlocks,
                "CHALLENGE_GRACE_PERIOD_NOT_PASSED"
            );
        }

Impacts

Assertion chain temporarily stuck if an assertion divergence arise while an ECM upgrade had just happened and the winner is against the new ECM instance (while the previous assertion is against the old ECM instance)

Proof of Concept

I will demostrate the setup described initially. We have 3 actors in play: Admin, Honest and Evil validator. 1) Admin deploy the Rollup and ECM contracts. Everything run smootly for days. 2) Evil validator submit assertion D. 3) Admin decides to upgrade the contract changing the staking requirement, which require a full upgrade and call setChallengeManager. 4) Honest validator submit assertion E. 5) Both assertion D and E are challenged, E wins the battle and his edge is confirmed (in the new ECM instance). 6) Honest validator call confirmAssertion but that will revert because of require(winningEdge.claimId == assertionHash, "NOT_WINNER");, as the winner edge will not be found in the previous ECM storage.

Recommended Mitigation Steps

Add a fallback to the current ECM in case the edge is not found from the previous assertion ECM.

        if (prevAssertion.secondChildBlock > 0) {
            // if the prev has more than 1 child, check if this assertion is the challenge winner
            ChallengeEdge memory winningEdge = IEdgeChallengeManager(prevConfig.challengeManager).getEdge(winningEdgeId);
+           if (winningEdge.claimId == byte32(0)) {
+               winningEdge = challengeManager.getEdge(winningEdgeId);
+           }
            require(winningEdge.claimId == assertionHash, "NOT_WINNER");
            require(winningEdge.status == EdgeStatus.Confirmed, "EDGE_NOT_CONFIRMED");
            require(winningEdge.confirmedAtBlock != 0, "ZERO_CONFIRMED_AT_BLOCK");
            // an additional number of blocks is added to ensure that the result of the challenge is widely
            // observable before it causes an assertion to be confirmed. After a winning edge is found, it will
            // always be challengeGracePeriodBlocks before an assertion can be confirmed
            require(
                block.number >= winningEdge.confirmedAtBlock + challengeGracePeriodBlocks,
                "CHALLENGE_GRACE_PERIOD_NOT_PASSED"
            );
        }

Assessed type

Upgradable

[gzeoneth]

Invalid, all challenge from the same prev use the ECM stored in the prev even when ECM is changed for new assertions. In the diagram and POC, both D and E would use ECM1, children of D or E would use ECM2.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid