The perform function in the BOLDUpgradeAction contract can be griefing by front-running the RollupProxy deploymen with same `salt` param

[c4-bot-6]

Lines of code

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

Vulnerability details

The perform function is used to make the upgrade from the old protocol to the new protocol, is in charge to create the config, make the deploys and initialize the contracts:

 function perform(address[] memory validators) external {

        cleanupOldRollup();
        Config memory config = createConfig();
        ...
        bytes32 rollupSalt = keccak256(abi.encode(config));

        ...
        RollupProxy rollup = new RollupProxy{salt: rollupSalt}(); <-------
        ...
        emit RollupMigrated(expectedRollupAddress, address(challengeManager));
    }

[Link]

The problem is that the RollupProxy is been create by the CREATE2 opcode as the salt is deterministic an attacker can create a contract with the same salt and make the execution fail.

Impact

The upgrade process can be halted by an attacker

Proof of Concept

see the perform function:

 function perform(address[] memory validators) external {

        cleanupOldRollup();
        Config memory config = createConfig();
        ...
        bytes32 rollupSalt = keccak256(abi.encode(config));

        ...
        RollupProxy rollup = new RollupProxy{salt: rollupSalt}(); <-------
        ...
        emit RollupMigrated(expectedRollupAddress, address(challengeManager));
    }

[Link]

The salt used is keccak256(abi.encode(config)) see the createConfig():

function createConfig() private view returns (Config memory) {

        bytes32 latestConfirmedStateHash = ROLLUP_READER.getNode(ROLLUP_READER.latestConfirmed()).stateHash;
        (ExecutionState memory genesisExecState, uint256 inboxMaxCount) = PREIMAGE_LOOKUP.get(latestConfirmedStateHash);

        AssertionState memory genesisAssertionState;
        genesisAssertionState.globalState = genesisExecState.globalState;
        genesisAssertionState.machineStatus = genesisExecState.machineStatus;

        require(
            PREIMAGE_LOOKUP.stateHash(genesisAssertionState.toExecutionState(), inboxMaxCount)
                == latestConfirmedStateHash,
            "Invalid latest execution hash"
        );
        ISequencerInbox.MaxTimeVariation memory maxTimeVariation;
        BufferConfig memory bufferConfig;

        return Config({
            confirmPeriodBlocks: CONFIRM_PERIOD_BLOCKS,
            stakeToken: STAKE_TOKEN,
            baseStake: STAKE_AMOUNT,
            wasmModuleRoot: ROLLUP_READER.wasmModuleRoot(),
            owner: address(this), // upgrade executor is the owner
            loserStakeEscrow: L1_TIMELOCK, 
            chainId: CHAIN_ID,
            chainConfig: "", 
            miniStakeValues: ConstantArrayStorage(MINI_STAKE_AMOUNTS_STORAGE).array(),
            sequencerInboxMaxTimeVariation: maxTimeVariation,
            layerZeroBlockEdgeHeight: BLOCK_LEAF_SIZE,
            layerZeroBigStepEdgeHeight: BIGSTEP_LEAF_SIZE,
            layerZeroSmallStepEdgeHeight: SMALLSTEP_LEAF_SIZE,
            genesisAssertionState: genesisAssertionState,
            genesisInboxCount: inboxMaxCount,
            anyTrustFastConfirmer: ANY_TRUST_FAST_CONFIRMER,
            numBigStepLevel: NUM_BIGSTEP_LEVEL,
            challengeGracePeriodBlocks: CHALLENGE_GRACE_PERIOD_BLOCKS,
            bufferConfig: bufferConfig
        });
    }

[Link]

As you can see an attacker can craft the config since all the values are deterministic, deploy the contract with this salt an then the perform function is gonna fail

Tools Used

Manual

Recommended Mitigation Steps

The best solution that i can think is make the deployment apart of the perform function and put the address of the deploy as a input.

another solution is add the msg.sender to the salt with a some kind of nonce, chainlink vrf, between others.

Assessed type

Other

[jorgect207]

Hey @Picodes, This issue has just two comments in the validation repo, one agree, one not.

But this is a common issue and is been validate as a medium in other contest too, deterministic salt allow an attacker to front run the perform transaction and make revert.

[godzillaba]

the rollup proxy is deployed by the upgrade executor, so even though the attacker knows the salt they can't deploy to the same address