c4-bot-4 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L488-L496 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L502-L518

Vulnerability details

Impact

timeUnrivaledTotal will retrieve incorrect timer if both child edges have rivals.

Proof of Concept

if three nodes ( A ) ,( B ) and (C) are nonterminal nodes with children, and ( A ) has children ( A_L ) and ( A_U ), and ( B ) has children ( B_L ) and ( B_U ) and (C) has children (C_L) and (C_U), then ( A_L ) and ( B_L ) are rivals and ( A_U ) and ( C_U ) are rivals,

POC

Node A is honest party and Node A 's lower child edge has been same mutual id with malicious Node B's lower child edge.[ (A_L ) and ( B_L ) are rivals ] .

2.Node B can't bisect anymore and Node A 's been UNRIVALED for 6 days ( almost challenge period ).

3.At UNRIVALED for 6th day , malicious Node C create upper child edge which is same mutual id with Node A 's upper child edge . [( A_U ) and ( C_U ) are rivals]

4.Due to bottom up timer implementation , parent edge is always taking less totalTimeUnrivaledCache between lower child and upper child .

5.At UNRIVALED for 7th day ,updateTimerCacheByChildren is called and while trying to confirmed by Node A with Node B , only totalTimeUnrivaledCache from upper child (lesser) is updated .

6.Only after when Node A win the Node C , disputing of Node A with Node B can be confirmed .

    function timeUnrivaledTotal(EdgeStore storage store, bytes32 edgeId) internal view returns (uint256) {
    uint256 totalTimeUnrivaled = timeUnrivaled(store, edgeId);
    if (store.edges[edgeId].lowerChildId != bytes32(0)) {
        uint256 lowerTimer = store.edges[store.edges[edgeId].lowerChildId].totalTimeUnrivaledCache;
        uint256 upperTimer = store.edges[store.edges[edgeId].upperChildId].totalTimeUnrivaledCache;
        totalTimeUnrivaled += lowerTimer < upperTimer ? lowerTimer : upperTimer;
    }
    return totalTimeUnrivaled;
}

This is how bottom up timer is worked and only the less value is taken into account . It will be conflict if there is two rivals at the same time .

Tools Used

manual view

Recommended Mitigation Steps

consider about both lower and upper child have rivals at the same time .

Assessed type

DoS

irving4444 commented 1 month ago

@gzeoneth @Picodes could u pls look at this report ?

gzeoneth commented 1 month ago

This is expected, you will need to accumulate timer from bottom in both side.