howlbot-integration[bot] commented 1 month ago

Lines of code

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

Vulnerability details

Impact

cleanupOldRollup() gets the stakerCount from the oldRollup, then loops over that number if they are less than 50 or 50 addresses if they are over 50 stakers.

Rollup.stakerCount() gets the number of staker
Rollup.getStakerAddress() gets the staker address from the _stakerList array
Rollup.forceRefundStaker() makes the staker's stake withdrawable and then deletes the staker

However, by deleting a staker, the array is being reduced from the right while the by incrementing the loop, we are skipping addresses from the left.

Proof of Concept

        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);
            }

By increaasing the index i, and getting the next staker address, the loop will run out of bounds since the array is being reduced in the OLD_ROLLUP contract.

This is not an issue with the old rollup, but with the loop depicted above.

Tools Used

MANUAL REVIEW, Josephdara

Recommended Mitigation Steps

Always get the first address in the loop, only change that if the currentChallenge for the first address is not zero.

uint64 j;
       for (uint64 i = 0; i < stakerCount; i++) {
            address stakerAddr = ROLLUP_READER.getStakerAddress(j);

            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);
            }else{
                j++;
            }
        }