`SecurityCouncilManager`'s `intialize()` function contains a gas bomb

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L118-L120

Vulnerability details

Bug Description

In SecurityCouncilManager.sol, the intialize() function calls _addSecurityCouncil() in a loop to add security councils individually:

SecurityCouncilManager.sol#L118-L120

        for (uint256 i = 0; i < _securityCouncils.length; i++) {
            _addSecurityCouncil(_securityCouncils[i]);
        }

_addSecurityCouncil() performs checks, which includes ensuring the new security council (_securityCouncilData) isn't already added, before adding it to the securityCouncils array:

SecurityCouncilManager.sol#L251-L262

        for (uint256 i = 0; i < securityCouncils.length; i++) {
            SecurityCouncilData storage existantSecurityCouncil = securityCouncils[i];

            if (
                existantSecurityCouncil.chainId == _securityCouncilData.chainId
                    && existantSecurityCouncil.securityCouncil == _securityCouncilData.securityCouncil
            ) {
                revert SecurityCouncilAlreadyInRouter(_securityCouncilData);
            }
        }

        securityCouncils.push(_securityCouncilData);

However, as the duplicate check loops over all elements in the securityCouncils storage array, _addSecurityCouncil() will consume a lot of gas whenever it is called.

As it is called repeatedly in intialize(), there is a signficant chance that initialize() might consume too much gas when called, making it revert due to an out-of-gas error.

The contract declares a maximum limit of 500 security councils to mitigate this:

SecurityCouncilManager.sol#L67

    uint256 public immutable MAX_SECURITY_COUNCILS = 500;

However, this is insufficient as calling initialize() with 500 security councils will still read from storage 125,250 times, which will still consume a huge amount of gas.

Impact

If SecurityCouncilManager is initialized with a large number of security councils, the initialize() function might not be executable due to consuming too much gas.

Recommended Mitigation

Consider performing all checks in initialize() and pushing to the securityCouncils array directly, instead of calling _addSecurityCouncil. This might reduce gas consumption significantly as the iteration is performed over the array stored in memory, thereby avoiding reading from storage.

Assessed type

DoS

[0xSorryNotSorry]

Initialize can only be called once due to the initializer modifier and as per the docs it will be called with max 12 (6+6) addresses.

Invalid assumption.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[MiloTruck]

Hi @0xean, could you clarify on why this issue is considered invalid?

I think the lookout has confused the number of security councils with the number of security council members (6+6 as per the docs). Security councils are contracts on many different chains that are used by security council members to execute governing actions.

Given that the number of security councils could be very large (as seen from the maximum limit here) I think that this is a risk worth highlighting to the sponsor.

Thanks!

[0xean]

Happy to mark this as QA.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a