The users will not be able to get refund of their stake for the levels lower than block level

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L731 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L741

Vulnerability details

Impact

In the current implementation of the EdgeChallengeManager, the users need to call confirmEdgeByTime() function in order to confirm that their edge is above the set confirmation timer. However, the problem is that the internal function checks whether the totalTimeUnrivaled is equal or more than the confirmationBlockThreshold after adding claimedAssertionUnrivaledBlocks to totalTimeUnrivaled. claimedAssertionUnrivaledBlocks will only have any value if the edge that was confirmed was of block level, otherwise, it'll be 0. Due to this check, edges of lower levels will not be able to set to Confirmed status and therefore will not be eligible for stake refund.

Proof of Concept

Initial public confirmByEdge() function in EdgeChallengeManager allows only layer zero edges to be confirmed:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L513-515

 if (!topEdge.isLayerZero()) {
            revert EdgeNotLayerZero(topEdge.id(), topEdge.staker, topEdge.claimId);
        }

So let's say the first edge that is to be confirmed is of Block level. The function the number of assertion blocks that the assertion that the edgeId claimed was unrivaled:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L529-530

assertionBlocks = assertionChain.getSecondChildCreationBlock(claimStateData.prevAssertionHash)
                - assertionChain.getFirstChildCreationBlock(claimStateData.prevAssertionHash);

This is then passed into internal function:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L533

uint256 totalTimeUnrivaled = store.confirmEdgeByTime(edgeId, assertionBlocks, challengePeriodBlocks);

Inside of the internal function there are these calculations and a check:

        uint256 totalTimeUnrivaled = timeUnrivaledTotal(store, edgeId);

        totalTimeUnrivaled += claimedAssertionUnrivaledBlocks;

        if (totalTimeUnrivaled < confirmationThresholdBlock) {
            revert InsufficientConfirmationBlocks(totalTimeUnrivaled, confirmationThresholdBlock);
        }

So it basically calculates how much time the edgeId and its children remained unrivaled and then adds claimedAssertionUnrivaledBlocks passed from the arguments and checks whether the totalTimeUnrivaled has exceeded the confirmationThresholdBlock. The problem is that for each subchallenge on each level (Block, BigStep, SmallStep) the user has to put some stake amount that will be returned if everything was fine and the edges were confirmed. But the edge stake can only be returned if the edge status is set to Confirmed:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/EdgeChallengeManager.sol#L575-582

 edge.setRefunded();

        IERC20 st = stakeToken;
        uint256 sa = stakeAmounts[edge.level];
        // no need to refund with the token or amount where zero'd out
        if (address(st) != address(0) && sa != 0) {
            st.safeTransfer(edge.staker, sa);
        }

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/libraries/ChallengeEdgeLib.sol#L264-267

  function setRefunded(ChallengeEdge storage edge) internal {
        if (edge.status != EdgeStatus.Confirmed) {
            revert EdgeNotConfirmed(ChallengeEdgeLib.id(edge), edge.status);
        }

As you can see, setRefunded() will only work if the edge is confirmed. But the current functionality will not add assertion blocks to the edges of the lower levels (not Block) and the value will be 0:

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/challengeV2/libraries/EdgeChallengeManagerLib.sol#L517

 uint64 assertionBlocks = 0;

This essentially means that this check will never work as the value will be always significantly lower than the confirmationThresholdBlock:

 if (totalTimeUnrivaled < confirmationThresholdBlock)

Tools Used

Manual review.

Recommended Mitigation Steps

Change the way how the timing is checked when confirming different types of edges.

Assessed type

Other

[rodiontr]

Hi @Picodes ! Can you please explain why this issue is invalid? If I understand this correctly, in the current implementation, when creating every layer zero edge (subchallenge), user has to specify some stake amount that's different from the previous subchallenge:

uint256 sa = stakeAmounts[args.level];
        // when a zero layer edge is created it must include stake amount. Each time a zero layer
        // edge is created it forces the honest participants to do some work, so we want to disincentive
        // their creation. The amount should also be enough to pay for the gas costs incurred by the honest
        // participant. This can be arranged out of bound by the excess stake receiver.
        // The contract initializer can disable staking by setting zeros for token or amount, to change
        // this a new challenge manager needs to be deployed and its address updated in the assertion chain
        if (address(st) != address(0) && sa != 0) {
            // since only one edge in a group of rivals can ever be confirmed, we know that we
            // will never need to refund more than one edge. Therefore we can immediately send
            // all stakes provided after the first one to an excess stake receiver.
            address receiver = edgeAdded.hasRival ? excessStakeReceiver : address(this);
            st.safeTransferFrom(msg.sender, receiver, sa);
        }

So to get a refund, the edge should be confirmed and the only way for it to be confirmed - by accumulating enough blocks to be greater than confirmationThresholdBlocks but that's not possible for children assertions as shown in the submission

[xuwinnie]

the value will be always significantly lower than the confirmationThresholdBlock

Hey this is not true. They will also eventually reach confirmationThresholdBlocks. Just like the top level assertion did.

[rodiontr]

the value will be always significantly lower than the confirmationThresholdBlock

Hey this is not true. They will also eventually reach confirmationThresholdBlocks. Just like the top level assertion did.

thanks for the response. But what about the case where block level assertion was unrivaled for 3 days (claimedAssertionUnrivaledBlocks) and totalTimeUnrivaled for the next edgeId of level big step is 3 days. How will the user get a refund for big step if then small step layer zero edge was created ? If the confirmation threshold period = 7 days

I mean how this check will eventually pass:

 if (totalTimeUnrivaled < confirmationThresholdBlock) {
            revert InsufficientConfirmationBlocks(totalTimeUnrivaled, confirmationThresholdBlock);
        }

[xuwinnie]

Generally speaking, edge inherits timer from its descendants, and eventually its descendants will either be unrivaled or onestepproved, so all correct edges will eventually be confirmed.

[rodiontr]

Generally speaking, edge inherits timer from its descendants, and eventually its descendants will either be unrivaled or onestepproved, so all correct edges will eventually be confirmed.

so they inherit both from descendants and the edges from levels above ? i thought only upper level inherits from the level below

And if small step was onestepproved quickly enough, big step hasn't accumulated enough time to be greater than confirmation threshold (in my example above)

[xuwinnie]

This function behaves the same for edge of any level. In your case big step can inherit timer from small steps. They don't inherit timers from levels above

[rodiontr]

This function behaves the same for edge of any level. In your case big step can inherit timer from small steps. They don't inherit timers from levels above

Okay but even if they inherit the timers, there can be a situation where big step level edge was unrivaled for 1 day reaching the edge of length 1 within this time period. Then the small step was onestepproved almost right away. There is no one step proof for big step so, in this scenario, it should accumulate some time to get confirmed. And at this moment, it doesn’t have confirmation threshold blocks

[xuwinnie]

Just look at updatetimerbyclaim. Big level length 1 edge can inherit timer from small level layer zero edge

[rodiontr]

Just look at updatetimerbyclaim. Big level length 1 edge can inherit timer from small level layer zero edge

it can inherit timer but the timer of small step can be less than confirmation threshold block and it can be confirmed via one step proof before reaching it

Thank you very much for your explanation but I still disagree. Want to hear a judge opinion on that

[xuwinnie]

If it's confirmed by one step proof timer will be set to max

[Picodes]

@rodiontr generally speaking, I think if one of your report didn't pass the validation repo, the correct way to behave is to ideally do a coded PoC, or at least rewrite a more detailed, clearer PoC. It's not up to the judge or the validators to convince you that your report is wrong, it's up to you to make your case.

[Picodes]

Here it'll inherit the timer of childs if they are unrivaled, and if they are confirmed it'll be set to max here