Lines of code

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

Vulnerability details

Description

BOLDUpgradeAction.sol has a problem inside cleanupOldRollup function which will always make this function revert whenever there is more than a single staker to refund (stakerCount() > 1) which seems to warrant Medium severity.

Impacts

When the upgrade occurs and BOLDUpgradeAction::cleanupOldRollup is called, if there are more then a single staker to be refunded (which is highly probable), the function will revert. The only solution will be to deploy another instance of BOLDUpgradeAction (which include a fix) and call perform again. Since this is the first step into the upgrade, we can assume redeploying a new fixed contract will resolve the issue with no side effects (the upgrade is not in the middle and some stuff needs to be reverted for example etc) and no other action required. I still think this warrant Medium severity for the time being saved from the Arbitrum team investigating this random revert, creating doubts/fears, etc.

Proof of Concept

Create the following file test file BOLDUpgradeAction.t.sol and run the test as follow. The code from RollupMinimal is taken from the current nitro-contracts repo, and BOLDUpgradeActionMinimal is having only the cleanupOldRollup pretty much as is.

forge test --match-test=testcleanupOldRollup -vvv

That will fail with the following output.

Failing tests:
Encountered 1 failing test in test/challengeV2/BOLDUpgradeAction.t.sol:BOLDUpgradeActionTest
[FAIL. Reason: panic: array out-of-bounds access (0x32)] testcleanupOldRollup() (gas: 1459605)

To resolve the problem, apply the following simple changes, and run the test gain.

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

                ROLLUP_READER.forceRefundStaker(stakersToRefund);
            }
        }

Ran 1 test for test/challengeV2/BOLDUpgradeAction.t.sol:BOLDUpgradeActionTest
[PASS] testcleanupOldRollup() (gas: 1191121)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 565.87µs (393.97µs CPU time)

pragma solidity ^0.8.17;

import "forge-std/Test.sol";

struct Staker {
    uint256 amountStaked;
    uint64 index;
    uint64 latestStakedNode;
    // currentChallenge is 0 if staker is not in a challenge
    uint64 currentChallenge;
    bool isStaked;
}

struct Zombie {
    address stakerAddress;
    uint64 latestStakedNode;
}

uint64 constant NO_CHAL_INDEX = 0;

// Taken from nitro contract repo
contract RollupMinimal {
    address[] private _stakerList;
    mapping(address => Staker) public _stakerMap;
    Zombie[] private _zombies;
    uint64 private _lastStakeBlock;
    uint64 private _latestConfirmed;

    function createNewStake(address stakerAddress, uint256 depositAmount) public {
        uint64 stakerIndex = uint64(_stakerList.length);
        _stakerList.push(stakerAddress);
        _stakerMap[stakerAddress] = Staker(
            depositAmount,
            stakerIndex,
            _latestConfirmed,
            NO_CHAL_INDEX,
            true
        );
        _lastStakeBlock = uint64(block.number);
    }

    function stakerCount() public view returns (uint64) {
        return uint64(_stakerList.length);
    }

    function getStakerAddress(uint64 stakerNum) public view returns (address) {
        return _stakerList[stakerNum];
    }

    function getStaker(address staker) public view returns (Staker memory) {
        return _stakerMap[staker];
    }

    function forceRefundStaker(address[] calldata staker) public {
        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);
    }

    function reduceStakeTo(address stakerAddress, uint256 target) internal returns (uint256) {
        Staker storage staker = _stakerMap[stakerAddress];
        uint256 current = staker.amountStaked;
        require(target <= current, "TOO_LITTLE_STAKE");
        uint256 amountWithdrawn = current - target;
        staker.amountStaked = target;
        //increaseWithdrawableFunds(stakerAddress, amountWithdrawn); // This is irrelevant for the test
        //emit UserStakeUpdated(stakerAddress, current, target);
        return amountWithdrawn;
    }

    function turnIntoZombie(address stakerAddress) internal {
        Staker storage staker = _stakerMap[stakerAddress];
        _zombies.push(Zombie(stakerAddress, staker.latestStakedNode));
        deleteStaker(stakerAddress);
    }

    function deleteStaker(address stakerAddress) private {
        Staker storage staker = _stakerMap[stakerAddress];
        require(staker.isStaked, "NOT_STAKED");
        uint64 stakerIndex = staker.index;
        _stakerList[stakerIndex] = _stakerList[_stakerList.length - 1];
        _stakerMap[_stakerList[stakerIndex]].index = stakerIndex;
        _stakerList.pop();
        delete _stakerMap[stakerAddress];
    }
}

contract BOLDUpgradeActionMinimal {
     RollupMinimal public immutable ROLLUP_READER;

     constructor(RollupMinimal _rollup) {
        ROLLUP_READER = _rollup;
    }

    function cleanupOldRollup() public {
        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);
            Staker memory staker = ROLLUP_READER.getStaker(stakerAddr);
            if (staker.isStaked && staker.currentChallenge == 0) {
                address[] memory stakersToRefund = new address[](1);
                stakersToRefund[0] = stakerAddr;

                ROLLUP_READER.forceRefundStaker(stakersToRefund);
            }
        }
    }    
}

contract BOLDUpgradeActionTest is Test {
    function testcleanupOldRollup() public {
        RollupMinimal rollup = new RollupMinimal();
        rollup.createNewStake(address(0x1), 1);
        rollup.createNewStake(address(0x2), 2);
        rollup.createNewStake(address(0x3), 3);
        rollup.createNewStake(address(0x4), 4);
        rollup.createNewStake(address(0x5), 5);
        BOLDUpgradeActionMinimal upgradeBOLD = new BOLDUpgradeActionMinimal(rollup);
        upgradeBOLD.cleanupOldRollup();

        assertEq(rollup.stakerCount(), 0);
    }
}

Recommended Mitigation Steps

The root cause is caused by the fact that forceRefundStaker will actually delete an entry from the _stakerList, while the outter loop index keep increasing without considering this fact, so it will revert at 50% of the way.

Apply the following patch to resolve the issue, so i is used only as a counter, but not as an index to retrieve the staker.

    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);
+           address stakerAddr = ROLLUP_READER.getStakerAddress(0);
            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);
    }

Assessed type

Loop

code-423n4 / 2024-05-arbitrum-foundation-findings

BOLDUpgradeAction::cleanupOldRollup will revert whenever there is more than a single staker to refund #53