Arbitrum BoLD - josephdara's results

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Although it is stated that not more than 50 stakers are expected, this remains a projection. Therefore if there exists more than 50 stakers, these stakers would be stuck on the older rollup.

Their funds would also be permanently stuck on the old rollup contract.

Proof of Concept

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

Tools Used

Manual Review, Josephdara

Recommended Mitigation Steps

Implement a function to complete refunds, or transfer the ownership of the old rollup contract to a new address after upgrade to allow for manual forceRefund calls.

Assessed type

Other