code423n4 commented 1 year ago

Lines of code

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L97-L102

Vulnerability details

Impact

The documentation of the Arbitrum Security Council Elections protocol mentions that the Security Council consists of 12 members and it is split into two cohorts as shown below:

They are a 12 member council from independent organizations that hold the ability to conduct emergency actions to the Arbitrum chains. The 12 member council is separated into 2 cohorts which are elected alternatively every 6 months.

In the SecurityCouncilManager.initialize function the lengths of the two cohorts are verified as follows:

    if (_firstCohort.length != _secondCohort.length) {
        revert CohortLengthMismatch(_firstCohort, _secondCohort);
    }
    firstCohort = _firstCohort;
    secondCohort = _secondCohort;
    cohortSize = _firstCohort.length;

The above verification ensures that _firstCohort.length == _secondCohort.length but it does not verify that the _firstCohort.length == _secondCohort.length == 6 which is intended number of members per cohort as per the protocol documentation.

Hence the SecurityCouncilManager contract can be deployed with more than 12 members split into to cohorts as long as the _firstCohort.length == _secondCohort.length condition is fulfilled. This could set the cohortSize state variable to a value more than 6 as well.

Hence this could lead to unintended behaviour of the protocol since the users of the protocol assumes that total number of members of the security council are 12 split into two cohorts equally each having 6 members.

Proof of Concept

        if (_firstCohort.length != _secondCohort.length) {
            revert CohortLengthMismatch(_firstCohort, _secondCohort);
        }
        firstCohort = _firstCohort;
        secondCohort = _secondCohort;
        cohortSize = _firstCohort.length;

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L97-L102

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to update the _firstCohort.length == _secondCohort.length conditional check performed in the initialize function, as follows to ensure each cohort consists of 6 members.

    if (_firstCohort.length != _secondCohort.length) {
        revert CohortLengthMismatch(_firstCohort, _secondCohort);
    }
    require (_firstCohort.length == 6, "Number of members per cohort does not match the requirement");
    firstCohort = _firstCohort;
    secondCohort = _secondCohort;
    cohortSize = _firstCohort.length;

Assessed type

Invalid Validation

0xSorryNotSorry commented 1 year ago

Technically valid. But the potential DAO errors in restricted functions is considered as QA.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)