L1SCMgmtActivationAction does not check executor role of new and prev emergency security council

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol#L30-L62 https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol#L113-L122

Vulnerability details

Proof of Concept

GovernanceChainSCMgmtActivationAction.sol checks that the newEmergencySecurityCouncil has a upgradeExecutor role whereas the prevEmergencySecurityCouncil does not have a upgradeExecutor role.

GovernanceChainSCMgmtActivationAction.sol
        // confirm updates
        bytes32 EXECUTOR_ROLE = upgradeExecutor.EXECUTOR_ROLE();
        require(
            upgradeExecutor.hasRole(EXECUTOR_ROLE, address(newEmergencySecurityCouncil)),
            "NonGovernanceChainSCMgmtActivationAction: new emergency security council not set"
        );
        require(
            !upgradeExecutor.hasRole(EXECUTOR_ROLE, address(prevEmergencySecurityCouncil)),
            "NonGovernanceChainSCMgmtActivationAction: prev emergency security council still set"
        );

In the L1 version of the protocol, L1SCMgmtActivationAction.sol, the upgradeExecutor role is not checked. Both function largely does the same thing, just on different chains, so it would be understandable if both functions are similar to each other.

Impact

The EXECUTOR_ROLE of the two emergency security councils in the L1 contract will not be adequately checked.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend adding the same upgradeExecutor() check to L1 contract instead of just using it in the L2 contract.

Assessed type

Access Control

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #249

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid