Stakers may not be refunded during rollup upgrade

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/BOLDUpgradeAction.sol#L341-#L363 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L274-#L279

Vulnerability details

Impact

During the process of upgrading the rollup contract, there exists a potential issue where the number of stakers exceeds the expected limit by the protocol. The function responsible for refunding existing stakers, pausing and upgrading the old rollup, uses a check to ensure the stakers count does not exceeds 50. However the rollup itself does not include any preventative measures to ensure that the number of stakers does not surpass this limit when creating a new stakes RollupCore.createnewStake(). As a result, if the number of stakers exceeds 50, only the first 50 stakers will be force refunded, while the rest may not receive their refunds.

As I understand the expectations are that the number of stakers will not get close to this number, nothings prevents this to happen. Stakers beyond the initial 50 may not receive their refunds during the upgrade process, leading to potential financial losses and dissatisfaction among affected stakeholders. Failure to ensure fair treatment of all stakers could erode trust in the smart contract system and damage the project's reputation within the community.

Proof of Concept

The provided code snippet illustrates the logic for handling the upgrade process and force refunding the stakers. It is shown how the stakerCount is hardcoded to 50 if it exceeds that number:

    /// @dev    Refund the existing stakers, pause and upgrade the current rollup to
    ///         allow them to withdraw after pausing
    function cleanupOldRollup() private {
        IOldRollupAdmin(address(OLD_ROLLUP)).pause();

        uint64 stakerCount = ROLLUP_READER.stakerCount();
        // since we for-loop these stakers we set an arbitrary limit - we dont
        // expect any instances to have close to this number of stakers
        if (stakerCount > 50) {
 @>         stakerCount = 50;
        }
        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);
            }
        }

        // upgrade the rollup to one that allows validators to withdraw even whilst paused
        DoubleLogicUUPSUpgradeable(address(OLD_ROLLUP)).upgradeSecondaryTo(IMPL_PATCHED_OLD_ROLLUP_USER);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Implementing an upper bound for stakers when creating a new stake in the RollupCore::createNewStake() function can help mitigate the risk of exceeding the staker limit during rollup upgrades

Assessed type

Invalid Validation

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b