Arbitrum BoLD - Takarez's results

Lines of code

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

Vulnerability details

Impact

The StakerCount variable take hold of the count of stakers in an instance, The Natspec comfirms that the stakerCount is not expected to be > 50, but this is not implemented in the code and there didn't seem to be restriction for that. This means there is a chance of the count to be more than 50.

The stakeCount is a list of stakers that stake in an instance and is of RollUpCore.sol func.

What can go wrong here? well, during the cleanupOldRollup function call, the stakerCount has been set to 50 if its > 50, remember, this is just for the loop not to be arbitrary and lead to out-Of gas issue or something unexpected.

The problem here is that the count has now been lost to only 50 whenever there's > 50 stakerCount, this is problematic as the number has been lost forever and this also means that some staker will be missed out when a call is made to cleanupOldRollup() function, potentially leaving them behind without being touched

Proof of Concept

/// @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

I recommend putting the restriction in place to make sure there's not more than 50 stakers in an instance, this will ensure that no staker is left behind during the function call.

The createNewStake function should be adjusted to something like:

function createNewStake(address stakerAddress, uint256 depositAmount, address withdrawalAddress) internal {
        uint64 stakerIndex = uint64(_stakerList.length);
@        if (stakerIndex <= 50){
            _stakerList.push(stakerAddress);
            _stakerMap[stakerAddress] = Staker(depositAmount, _latestConfirmed, stakerIndex, true, withdrawalAddress);
        }
        emit UserStakeUpdated(stakerAddress, withdrawalAddress, 0, depositAmount);
    }

This adjustment will ensure the stakers to be included in the cleanupOldRollup() function call without leaving any behind.

Assessed type

Loop