Malicious user can create a challenge to lock staker fund during upgrade.

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Malicious user can create a challenge to lock staker fund during upgrade.

Proof of Concept

To upgrade the rollup contract,

the step is:

1. deploy the BOLDUpgradeAction.sol contract.
2. upgrade executor delegate call BOLDUpgradeAction.sol and trigger the upgrade.

during the upgrade, the function clean out some old state is called first

    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);
            // strange, what is the purpose of this check?
            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);

first the old rollup contract is paused,

then if the the staker is staked and there is no active challange, the codo trigger forceRefundStaker

if (staker.isStaked && staker.currentChallenge == 0) {
    address[] memory stakersToRefund = new address[](1);
    stakersToRefund[0] = stakerAddr;
    IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);
}

then in the end, the code upgrade the rollup to one that allows validators to withdraw even when the old roll is paused

the problem is that the old roll up does not get unpaused / resumed,

if there is ongoing challange during the upgrade time,

the function below will not be called.

  IOldRollupAdmin(address(OLD_ROLLUP)).forceRefundStaker(stakersToRefund);

1. if there is an ongoing challange, the staker cannot withdraw their fund.
2. if the contract is paused, the ongoing challange cannot be resolved by calling the Old rollup complete Challange function
3. the ongoing challange cannot be resolved, while upgrade the rollup to one that allows validators to withdraw fund even when paused,

because of the unresolved challange, staker fund is locked.

Sponsor confirms that the contract IMPL_PATCHED_OLD_ROLLUP_USER comes from this PR:

https://github.com/OffchainLabs/nitro-contracts/pull/48/files

the whenNotPaused modifier is removed from the function in the contract IMPL_PATCHED_OLD_ROLLUP_USER

returnOldDeposit reduceDeposit withdrawStakerFunds

if a staker wants to withdraw the fund from old roll up contract.

they needs to call withdrawStakerFunds

but if the stakers want to call withdrawStakerFunds,

the validator needs to call reduceDeposit / returnOldDeposit first increment the withdrawal fund amount.

but calling these two function requires that there is no ongoing challange for a staker

https://github.com/OffchainLabs/nitro-contracts/blob/90037b996509312ef1addb3f9352457b8a99d6a6/src/rollup/RollupUserLogic.sol#L616

  /**
     * @notice Verify that the given address is staked and not actively in a challenge
     * @param stakerAddress Address to check
     */
    function requireUnchallengedStaker(address stakerAddress) private view {
        require(isStaked(stakerAddress), "NOT_STAKED");
        require(currentChallenge(stakerAddress) == NO_CHAL_INDEX, "IN_CHAL");
    }

I think the severity is high because a user can just put some fund to intentionally challange a staker by frontrunnnig the upgrade to DOS the staker from withdraw their fund.

whether this finding is in-scope may have some debate:

from the contest readme:

https://github.com/code-423n4/2024-05-arbitrum-foundation?tab=readme-ov-file#automated-findings--publicly-known-issues

Any issues whose root cause exists in nitro-contracts@77ee9de is considered out of scope but may be eligible for our bug bounty.

but while the root cause is that the new IMPL_PATCHED_OLD_ROLLUP_USER does not remove the whenNotPaused modifier in the function completeChallange

the root cause is also that the upgrade contract does not unpause / resume the old rollup after the upgrade.

the good things is in the worst case if this happens,

the old roll up logic can be upgrade again to resolve the issue.

Tools Used

Manual Review

Recommended Mitigation Steps

two ways to resolve the issues:

1. resume / unpause the old rollup contract during the upgrade.

    function cleanupOldRollup() private {
        IOldRollupAdmin(address(OLD_ROLLUP)).pause();
        ... 
         IOldRollupAdmin(address(OLD_ROLLUP)).resume();
    }

2. remove the whenNotPaused modifier from the function completeChallange in the PR

https://github.com/OffchainLabs/nitro-contracts/pull/48/files

Assessed type

Invalid Validation

[c4-sponsor]

gzeoneth (sponsor) acknowledged

[gzeoneth]

Disagree with severity, I think the risk is low. Currently Arb1 and Nova have whitelisted validators so this is unlikely to happen, and since we are upgrading we don’t want to keep using the old challenge protocol, if stake is being stuck they can be refunded with a future proposal.

[Picodes]

I do side with the sponsor's reasoning here, validators are whitelisted so the risk remains minimal and it can be solved by the DAO

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[JeffCX]

Thanks for judging,

I think based on sponsor comments, the severity can be downgrade to medium,

but it is not a QA issue.

if there are onging challange during upgrade, the staker fund is locked as shown in the report, we are talking about likelihood here.

Currently Arb1 and Nova have whitelisted validators so this is unlikely to happen

yes, but I think the audit was done under the assumption that validator can behavior maliciously, the same assumption is used to validate the issue https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/37

since we are upgrading we don’t want to keep using the old challenge protocol

that make sense, but leave ongoing challange not solved still lock staker's fund without protocol's action to issue refund.

if stake is being stuck they can be refunded with a future proposal.

emm that is true, but I politely think a issue is still a issue even the contract is upgradeable, otherwise, we cannot say if a contract is upgradeable, all bug is out of scope.

also I think one can argue that a new action can be implemented to resolve the issue

https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/5

but issue 5 is clearly a firmly valid finding.

while the true fund at risk are small,

https://docs.arbitrum.io/build-decentralized-apps/reference/useful-addresses

the rollup address https://etherscan.io/address/0x5eF0D09d1E6204141B4d37530808eD19f60FBa35 arbitrum one has 2 ETH from staker.

the rollup address https://etherscan.io/address/0xFb209827c58283535b744575e11953DCC4bEAD88 arbitrum nova has 2 ETH from staker.

I politely think the severity can be medium.

Thank you 🙏

[JeffCX]

also, I left some comments in a issue that is marked as duplicate of this issue

https://github.com/code-423n4/2024-05-arbitrum-foundation-findings/issues/11

[yuliyu123]

Hi, left a comment in https://github.com/code-423n4/2024-05-arbitrum-foundation-validation/issues/150.

[gzeoneth]

Malicious user being able to temporarily lock honest staker's fund is an expected behavior regardless of the upgrade, as an optimistic rollup does not know who is the winner immediately on-chain. In the unlikely event there is a challenge created in the old rollup, we believe it is appropriate to let the stake be stuck there waiting for the dao to determine how to resolve.

Worth to note the current stake on Arb1 and Nova in the old rollup is 1 eth so the impact is very low.

[JeffCX]

Malicious user being able to temporarily lock honest staker's fund is an expected behavior regardless of the upgrade, as an optimistic rollup does not know who is the winner immediately on-chain

without upgrade, the old challenge can be resolved to unlock fund, that is not the case after the upgrade as shown in the original report.

In the unlikely event there is a challenge created in the old rollup, we believe it is appropriate to let the stake be stuck there waiting for the dao to determine how to resolve.

yes that is true

Worth to note the current stake on Arb1 and Nova in the old rollup is 1 eth so the impact is very low.

yes currently there are 4 ETH in total in Arb1 and Nova rollup contract.

will respect judge's final decision. 👍

[Picodes]

Unless I am missing something the bottom line of the debate is that challenges can't be resolved when the contracts are paused, so funds may be stuck while the DAO is figuring out how to handle this or unpause the contract. In particular, this does happen during upgrades. This was the expected design, so no functionality is broken, so I don't think this is worth Medium severity.