Lines of code
<https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L420-L443&gt;

Vulnerability details

Impact

SecurityCouncilMemberSyncAction.perform is a crucial function that will be triggered by upgrade executor via delegate call after the whole election process or after current members do some update (add/remove/replace/rotate) to update security council contract’s cohort members. However, due to lack of caller verification, any normal governance proposal can propose this and violate the whole election process.

Proof of Concept

After every update of cohort members inside SecurityCouncilManager, it will try to trigger internal function _scheduleUpdate() that will push the update trough the necessary L2/L1 security council contract via upgrade executor :

<https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L420-L443&gt;

    function _scheduleUpdate() internal {
        // always update the nonce
        // this is used to ensure that proposals in the timelocks are unique
        // and calls to the upgradeExecutors are in the correct order
        updateNonce++;
        (address[] memory newMembers, address to, bytes memory data) =
            getScheduleUpdateInnerData(updateNonce);

        ArbitrumTimelock(l2CoreGovTimelock).schedule({
            target: to, // ArbSys address - this will trigger a call from L2->L1
            value: 0,
            // call to ArbSys.sendTxToL1; target the L1 timelock with the calldata previously constucted
            data: data,
            predecessor: bytes32(0),
            // must be unique as the proposal hash is used for replay protection in the L2 timelock
            // we cant be sure another proposal wont use this salt, and the same target + data
            // but in that case the proposal will do what we want it to do anyway
            // this can however block the execution of the election - so in this case the
            // Security Council would need to unblock it by setting the election to executed state
            // in the Member Election governor
            salt: this.generateSalt(newMembers, updateNonce),
            delay: ArbitrumTimelock(l2CoreGovTimelock).getMinDelay()
        });
    }

SecurityCouncilMemberSyncAction.perform will be called for each chain, providing the securityCouncil, newMembers and nonce.

<https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L379-L416&gt;

    function getScheduleUpdateInnerData(uint256 nonce)
        public
        view
        returns (address[] memory, address, bytes memory)
    {
        // build a union array of security council members
        address[] memory newMembers = getBothCohorts();

        // build batch call to L1 timelock
        address[] memory actionAddresses = new address[](securityCouncils.length);
        bytes[] memory actionDatas = new bytes[](securityCouncils.length);
        uint256[] memory chainIds = new uint256[](securityCouncils.length);

        for (uint256 i = 0; i < securityCouncils.length; i++) {
            SecurityCouncilData memory securityCouncilData = securityCouncils[i];
            actionAddresses[i] = securityCouncilData.updateAction;
            chainIds[i] = securityCouncilData.chainId;
            actionDatas[i] = abi.encodeWithSelector(
                SecurityCouncilMemberSyncAction.perform.selector,
                securityCouncilData.securityCouncil,
                newMembers,
                nonce
            );
        }

        // unique salt used for replay protection in the L1 timelock
        bytes32 salt = this.generateSalt(newMembers, nonce);
        (address to, bytes memory data) = router.createActionRouteData(
            chainIds,
            actionAddresses,
            new uint256[](securityCouncils.length), // all values are always 0
            actionDatas,
            0,
            salt
        );

        return (newMembers, to, data);
    }

However, other governance proposal can also propose the SecurityCouncilMemberSyncAction.perform and provide the necessary data and valid nonce, violating the whole election process and security council operations (especially the time restriction for election).

Tools Used

Manual review

Recommended Mitigation Steps

Provide necessary validation to restrict SecurityCouncilMemberSyncAction.perform operation inside normal proposal lifecycle.

Assessed type

Invalid Validation

---

The text was updated successfully, but these errors were encountered:

All reactions