The time spent paused is incremented in the rollup's timing for assertion validation.

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupAdminLogic.sol#L145 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/RollupUserLogic.sol#L110

Vulnerability details

Impact

    /**
     * @notice Pause interaction with the rollup contract.
     * The time spent paused is not incremented in the rollup's timing for assertion validation.
     * @dev this function may be frontrun by a validator (ie to create a assertion before the system is paused).
     * The pause should be called atomically with required checks to be sure the system is paused in a consistent state.
     * The RollupAdmin may execute a check against the Rollup's latest assertion num or the OldChallengeManager, then execute this function atomically with it.
     */
    function pause() external override {
        _pause();
        emit OwnerFunctionCalled(3);
    }

According to the comment, the time spent paused should not be incremented in the rollup's timing for assertion validation. However, the rollup's timing does not take paused time into consideration in reality. As a result, adversary can censor transactions (within the censorship budget) and force incorrect assertion to be confirmed.

Proof of Concept

According to the contest doc

We assume that an adversary can censor transactions for at most 1 challengePeriodBlocks or confirmPeriodBlock (whichever is smaller)

Suppose challengePeriodBlocks and confirmPeriodBlock are both 7 days. The steps are as following:

1. Adversary make an invalid assertion to the latest confirmed assertion, and censor all other txs that submit assertion for 2 days.(Day 0 - Day 1)
2. Admin pauses the rollup to counteract the situation for 2 days.(Day 2 - Day 3)
3. Admin realizes pausing doesn't help, and unpauses.
4. Adversary continues to censor all txs that submit assertion for 4 days.(Day 4 - Day 7)
5. Adversary confirms this incorrect assertion.

Since no one can make new assertions when the contract is paused, the adversary only needs to spend 6 days of censorship budget (to confirm an incorrect assertion) in this case.

require(block.number >= assertion.createdAtBlock + prevConfig.confirmPeriodBlocks, "BEFORE_DEADLINE"); As this line suggests, no special care for paused time is taken.

Tools Used

Manual

Recommended Mitigation Steps

Make sure the time spent paused is not incremented in the rollup's timing for assertion validation.

Assessed type

Context

[c4-sponsor]

godzillaba (sponsor) acknowledged

[godzillaba]

We disagree with the severity and think this should be low/QA. We will add comments to the pause/unpause functions informing the admin that timers do not stop

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[Picodes]

This report shows how in the event the contract is paused, the challenge period is effectively shorter, and it could eventually drop to a point where it's within the censorship budget of an attacker. As this is a core invariant of the system, and as it wasn't documented, I tend to accept this as a valid Medium-severity issue

[gzeoneth]

This report shows how in the event the contract is paused, the challenge period is effectively shorter, and it could eventually drop to a point where it's within the censorship budget of an attacker. As this is a core invariant of the system, and as it wasn't documented, I tend to accept this as a valid Medium-severity issue

We respectfully disagree. Pausing and unpausing are highly trusted admin actions where in most case should be followed by a contract upgrade and/or force confirmation. If an emergency pause is performed, the censorship invariant is not expected to hold. Admin actions that lead to the protocol being stuck in suboptimal state should be considered as misconfiguration. I believe this should be Low risk at best.

[JeffCX]

past very similar issue is judged as medium

https://github.com/code-423n4/2024-03-taiko-findings/issues/170

also the code comments says:

the time spent paused should not be incremented in the rollup's timing for assertion validation.

impact is confirmation grace period that act as safe guard is bypassed when pausing.

[gzeoneth]

Worth to note we don't currently have a pauser role, only the admin can pause so it is quite different than a system where a lower privileged admin can pause and unpause the protocol.

[carlitox477]

Regarding the severity of this issue, I added a comment in #3

[Picodes]

I am still hesitating following all your comments on the correct decision for #3 and #7.

Note that these issues as well are related to the fact that invariants are broken following admin actions:

• https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/23
• https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/4
• https://github.com/code-423n4/2024-05-arbitrum-foundation-validation/issues/336

[Picodes]

The sponsor's argument that admin actions are meant to be used only to deal with an emergency bug or an upgrade and that therefore it's intended that invariants do not hold anymore is convincing, but up to my knowledge wasn't clearly specified before the contest

[Picodes]

Although I do agree with #3 and #7 and the overall fact that the consequence of admin actions on "usual" invariants were poorly documented, it seems that the intention of the sponsor was that indeed pausing or for fast confirming assertions is made for emergency or upgrade procedures and that therefore it's intended that special care must be taken and that once called, resuming operations isn't just calling "unpause", so it makes sense that invariant may be broken in this case.

Consequently, I believe these reports fit "state handling, function incorrect as to spec, issues with comments" more than "function of the protocol or its availability could be impacted".

I'll therefore downgrade #3 and #7 to QA.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[DecentralDisco]

For transparency, the selected for report label was removed now that all QA votes have been received and a clear top scoring report has been determined.

[DecentralDisco]

Noting that the 'grade-a' label was added on behalf of the judge after receiving confirmation.