An invalid validator can prevent upgrade by creating an assertion on old Nitro rollup

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/BOLDUpgradeAction.sol#L357

Vulnerability details

Impact

The BoLD upgrade action will fail

Proof of Concept

The BoLD upgrade is processed via BOLDUpgradeAction.sol which will be called through delegatecall from the governance. During the upgrade process, it pauses the old Nitro rollup contract and force-refund all stakes in the contract before the upgrade.

function cleanupOldRollup() private {
    IOldRollupAdmin(address(OLD_ROLLUP)).pause();
    // ...
    for (uint64 i = 0; i < stakerCount; i++) {
        // ...
        IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);
        // ...
    }
    // ...
}

The pause called during the upgrade, which means validators can still create an assertion on the old Nitro contract. If any validator creates an assertion before the upgrade happens, the forceRefundStaker will revert which will prevent the upgrade as the code snippet below shows(Nitro's RollupAdminLogic.sol:L276).

function forceRefundStaker(address[] calldata staker) external override whenPaused {
    require(staker.length > 0, "EMPTY_ARRAY");
    for (uint256 i = 0; i < staker.length; i++) {
        require(_stakerMap[staker[i]].currentChallenge == NO_CHAL_INDEX, "STAKER_IN_CHALL");
        reduceStakeTo(staker[i], 0);
        turnIntoZombie(staker[i]);
    }
    emit OwnerFunctionCalled(22);
}

This reverts because the staker's current challenge exists.

Tools Used

Manual Review

Recommended Mitigation Steps

Rather than pausing and force-refunding all stakes in the Nitro contract, it should upgrade the old Nitro rollup contract to a patch contract that allows existing stakers to withdraw their stakes by themselves.

Assessed type

DoS

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[JeffCX]

this report claim

If any validator creates an assertion before the upgrade happens, the forceRefundStaker will revert which will prevent the upgrade as the code snippet below shows(Nitro's RollupAdminLogic.sol:L276).

but in the original submission code snippet:

function cleanupOldRollup() private {
    IOldRollupAdmin(address(OLD_ROLLUP)).pause();
    // ...
    for (uint64 i = 0; i < stakerCount; i++) {
        // ...
        IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);
        // ...
    }
    // ...
}

the first ... actually prevent this issue from happens.

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/6f861c85b281a29f04daacfe17a2099d7dad5f8f/src/rollup/BOLDUpgradeAction.sol#L353

only if there is no ongoing challange the staker's fund get released.

 for (uint64 i = 0; i < stakerCount; i++) {
            address stakerAddr = ROLLUP_READER.getStakerAddress(i);
            OldStaker memory staker = ROLLUP_READER.getStaker(stakerAddr);
            if (staker.isStaked && staker.currentChallenge == 0) {
                address[] memory stakersToRefund = new address[](1);
                stakersToRefund[0] = stakerAddr;

                IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);
            }
        }

note the logic there.

      OldStaker memory staker = ROLLUP_READER.getStaker(stakerAddr);
      if (staker.isStaked && staker.currentChallenge == 0) {

[c4-judge]

Picodes marked the issue as grade-b