A malicious validator can avoid loss his money doing bad assertions

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Validator can unstake if their assertions is the last confirmed (see the the first arrow below) or his assertion has already a first child(see arrow below):

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

[Link]

Other point to understand the vulnerability is that validators can create a new assertion with non confirmed parents assertions(a parent is the past assertion). with that been said An attacker can control 2 validators and do the next steps:

1. stake in a bad assertion with his validator number 1 (he has to call newStakeOnNewAssertion).
2. stake on new assertion with a parent create in the step 1 with validator 2.
3. with validator 1 call returnOldDeposit since the assertion of the validator number 1 has already a child he can withdraw.
4. with validator 1 create a assertion with a parent assertion of the step 2.
5. at this point validator 2 can call returnOldDeposit and withdraw his money.

an attacker can keep doing this indefinitely leading to some problems:

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

        if (!getAssertionStorage(newAssertionHash).isFirstChild) {
            // We assume assertion.beforeStateData is valid here as it will be validated in createNewAssertion
            // only 1 of the children can be confirmed and get their stake refunded
            // so we send the other children's stake to the loserStakeEscrow
            // NOTE: if the losing staker have staked more than requiredStake, the excess stake will be stuck
            IERC20(stakeToken).safeTransfer(loserStakeEscrow, assertion.beforeStateData.configData.requiredStake); <------
        }
    }

[Link]

Each time that a assertion is created a amount equivalent to the requiredStake is been sent it to the loserStakeEscrow, successfully letting without funds the rollup contract.

Impact

This vulnerability has different impacts:

1. The rollup contract can be drained. (if there is a way to recover the funds in the loserStakeEscrow, it still a problem since an attacker can uses this to Dos withdrawals to honest validators).
2. A malicious validator can not be slashed (but he can not withdraw either).
3. Honest validator has work more and the malicious validator is not been actually punished

Proof of Concept

Run the next proof of concept in file:/test/Rollup.t.sol

function testSuccessCreateSecondAssertionUnstake() public {
        (bytes32 prevHash, AssertionState memory beforeState, uint64 prevInboxCount) = testSuccessCreateAssertion(); // create first assertion validator 1

        AssertionState memory afterState; // set up second assertion
        afterState.machineStatus = MachineStatus.FINISHED;
        afterState.globalState.u64Vals[0] = prevInboxCount;
        bytes32 inboxAcc = userRollup.bridge().sequencerInboxAccs(1); // 1 because we moved the position within message
        bytes32 expectedAssertionHash2 =
            RollupLib.assertionHash({parentAssertionHash: prevHash, afterState: afterState, inboxAcc: inboxAcc});
        bytes32 prevInboxAcc = userRollup.bridge().sequencerInboxAccs(0);
        vm.roll(block.number + 75);
        vm.prank(validator2); // validator 2 make a new assertion 
        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: expectedAssertionHash2,
            withdrawalAddress: validator2Withdrawal
        });

        vm.prank(validator1); //validator 1 withdraw
        userRollup.returnOldDeposit();
    }

The point of the proof of concept is demonstrate that validators can unstake no matter if his assertion is not confirmed but yes if it has a child. if the assertion is invalid they already unstake and can trick the protocol as i writed in the description.

Tools Used

Manual, foundry

Recommended Mitigation Steps

Consider don't allow validator to withdraw his stake until his assertion have been confirmed.

Assessed type

Other

[gzeoneth]

Invalid. The rollup will not be drained because funds are only sent to the escrow if isFistChild is false, which is not possible in the POC. The malicious validator is still slashed for one stake, because they’ve made one dishonest branch in the assertion tree. Honest parties are not burdened by additional work in the proposed scenario.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[jorgect207]

hey @gzeoneth and @Picodes thanks so much for the judging.

In my POC i just demonstrating that validators can unstake no matter if his assertion is not confirmed but yes if it has a child. So matter if he made a bad assertion he can withdraw as long as this assertion has a child. see my proof of concept. this break the invariant that a validator get slashed for doing bad assertions.

[gzeoneth]

It is ok because the attackers (collectively) is still being slashed by no less than 1 stake.

[jorgect207]

hey @gzeoneth the problem is that an attacker can withdraw his eth once his assertion has a child, and reuse this same eth to make a new assertion, on top of the assertion made in his first assertion.

See the scenario that i describe in my submission:

1. stake in a bad assertion with his validator number 1 (he has to call newStakeOnNewAssertion).
2. stake on new assertion with a parent create in the step 1 with validator 2.
3. with validator 1 call returnOldDeposit since the assertion of the validator number 1 has already a child he can withdraw.
4. with validator 1 create a assertion with a parent assertion of the step 2.
5. at this point validator 2 can call returnOldDeposit and withdraw his money.

Validator always get back his stake in step 3, because the contract is allowing unstake even if the assertion is not confirmed as long as it has one child. if a malicious validator control 2 stacker, he can just keep doing the same creating new child in his owns bad assertions and getting back his eth each time that a child is created. and the contract is sending the requiredStake to the scow in each child created.

Thanks so much for your time @gzeoneth, i know that is too valuable so i appreciate that.

[gzeoneth]

child of an incorrect assertion does not matter, it will never be confirmable nor causes any delay

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

The resolution of challenges that do not involve honest claims are out of scope unless they lead to incorrect assertions being confirmed

honest validator does not stake on invalid branch, child assertion of invalid assertions are considered irrelevant and is out-of-scope