Preventing future upgrade by increasing the number of stakers unlimitedly

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Having the validator whitelist disabled provides an opportunity to increase the number of stakers indefinitely, leading to an issue during the upgrade of BOLD.

Proof of Concept

If validatorWhitelistDisabled is true, then any users can stake on a new assertion.

    function stakeOnNewAssertion(AssertionInputs calldata assertion, bytes32 expectedAssertionHash)
        public
        onlyValidator
        whenNotPaused
    {
        //....
    }

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

This can lead to the situation that the number of stakers are too high to be handled easily during a future upgrade.

1. A malicious user calls newStakeOnNewAssertion to become a new staker and create a new valid assertion.

   function newStakeOnNewAssertion(
       uint256 tokenAmount,
       AssertionInputs calldata assertion,
       bytes32 expectedAssertionHash,
       address withdrawalAddress
   ) public {
       require(withdrawalAddress != address(0), "EMPTY_WITHDRAWAL_ADDRESS");
       _newStake(tokenAmount, withdrawalAddress);
       stakeOnNewAssertion(assertion, expectedAssertionHash);
       /// @dev This is an external call, safe because it's at the end of the function
       receiveTokens(tokenAmount);
   }

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

   This will invoke the internal function _newStake.

   function _newStake(uint256 depositAmount, address withdrawalAddress) internal onlyValidator whenNotPaused {
       // Verify that sender is not already a staker
       require(!isStaked(msg.sender), "ALREADY_STAKED");
       // amount will be checked when creating an assertion
       createNewStake(msg.sender, depositAmount, withdrawalAddress);
   }

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

   This will invoke the function createNewStake, where the malicious user will be added to the list of _stakerList and the flag isStaked will be set to true in the mapping _stakerMap.

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

   https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L276 https://github.com/code-423n4/2024-05-arbitrum-foundation/blob/main/src/rollup/RollupCore.sol#L277C90-L277C94

   struct Staker {
       uint256 amountStaked;
       bytes32 latestStakedAssertion;
       uint64 index;
       bool isStaked;
       address withdrawalAddress;
   }

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

   2. The malicious user reduces his staked amount to zero when this assertion is confirmed or a child is added to it. Since, this assertion is confirmed or a child is added to it, the condition requireInactiveStaker is met.

      function reduceDeposit(uint256 target) external onlyValidator whenNotPaused {
      requireInactiveStaker(msg.sender);
      // amount will be checked when creating an assertion
      reduceStakeTo(msg.sender, target);
      }

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

   function requireInactiveStaker(address stakerAddress) internal view {
       require(isStaked(stakerAddress), "NOT_STAKED");
       // A staker is inactive if
       // a) their last staked assertion is the latest confirmed assertion
       // b) their last staked assertion have a child
       bytes32 lastestAssertion = latestStakedAssertion(stakerAddress);
       bool isLatestConfirmed = lastestAssertion == latestConfirmed();
       bool haveChild = getAssertionStorage(lastestAssertion).firstChildBlock > 0;
       require(isLatestConfirmed || haveChild, "STAKE_ACTIVE");
   }

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

   By doing so, the malicious user’s withdrawable funds will increase.

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

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

   function increaseWithdrawableFunds(address account, uint256 amount) internal {
       uint256 initialWithdrawable = _withdrawableFunds[account];
       uint256 finalWithdrawable = initialWithdrawable + amount;
       _withdrawableFunds[account] = finalWithdrawable;
       totalWithdrawableFunds += amount;
       emit UserWithdrawableFundsUpdated(account, initialWithdrawable, finalWithdrawable);
   }

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

   3. The malicious user withdraws his deposit by calling withdrawStakerFunds.

      function withdrawStakerFunds() external override whenNotPaused returns (uint256) {
      uint256 amount = withdrawFunds(msg.sender);
      require(amount > 0, "NO_FUNDS_TO_WITHDRAW");
      // This is safe because it occurs after all checks and effects
      IERC20(stakeToken).safeTransfer(msg.sender, amount);
      return amount;
      }

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

   4. Since the malicious user is not deleted, only his staked amount is reduced to zero, so he is still considered as a staker.

   5. The malicious user transfers this fund to another address under his control.

   6. The malicious user repeats steps 1 to 5 many times (each time with different address) to stake on and create another valid assertion. By doing so, the malicious user increases the number of stakers indefinitely.

Later, when an upgrade to a new rollup is going to be done, there could be an issue.

In the current upgrade mechanism which is implemented to upgrade the current rollup to BOLD, first, the current rollup should be paused, and then, all the stakers are forced to be refunded. Based on the following comment, it is not expecting to have more than 50 stakers. This expectation is true based on the structure of the current rollup (out-of-scope).

// since we for-loop these stakers we set an arbitrary limit - we dont
// expect any instances to have close to this number of stakers

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

But, if in the future, another upgrade is needed to upgrade the BOLD to BOLD2 (for example), it seems that again, after pausing the BOLD, all the stakers should be forced to be refunded. But, as explained earlier, the malicious user could increase the number of stakers (with zero staked amount) indefinitely in BOLD. Therefore, forcing to refund all the stakers could be impractical when upgrading BOLD to BOLD2.

The flow of upgrade from current rollup to BOLD is as follows:

In the contract BOLDUpgradeAction, the function perform should be called, which invokes the private function cleanupOldRollup.

function perform(address[] memory validators) external {
    //....
    cleanupOldRollup();
    //.....
}

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

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

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

In this function, a for-loop goes over the stakers in the _stakerList by fetching their number by calling stakerCount, and forces them to be refunded.

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

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

At the end of the function cleanupOldRollup, it will upgrade the current rollup address to IMPL_PATCHED_OLD_ROLLUP_USER, which is similar to the current rollup contract except that the modifier whenNotPaused is removed from the function withdrawStakerFunds. This allows stakers to withdraw their withdrawable funds later even when the current rollup is paused.

// the old rollup, but with whenNotPaused protection removed from stake withdrawal functions
    address public immutable IMPL_PATCHED_OLD_ROLLUP_USER;

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

    function cleanupOldRollup() private {
        //....

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

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

Then, a new rollup proxy is deployed.

RollupProxy rollup = new RollupProxy{salt: rollupSalt}();

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

Tools Used

Recommended Mitigation Steps

It is recommended to:

• If the staked amount of a staker is reduced to zero, it becomes deleted automatically to not be considered as a staker anymore.
• Enforce the staker to keep the minimum amount as a stake, otherwise, they will not be considered as a staker.
• During the upgrade, enforce to refund all the stakers. If its length is too long, the for-loop should be executed in more than one step. When, the admin is sure that all the stakers on the old rollup are refunded, then the old rollup should be paused.

Assessed type

Context

[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)