code423n4 commented 1 year ago

Lines of code

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L166-L167 https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilManager.sol#L176-L180

Vulnerability details

Impact

The SecurityCouncilManager._removeMemberFromCohortArray function is used to remove a member from a specific cohort. The function will replace the removing member with the last element of the cohort array and will pop the last element of the array as shown below:

                cohort[j] = cohort[cohort.length - 1];
                cohort.pop();

The issue here is, once the last element of the cohort array is removed the array.length will be reduced by one.

This will DoS the SecurityCouncilManager.addMember function since the (cohort.length == cohortSize) condition will be false and the transaction will revert inside the SecurityCouncilManager._addMemberToCohortArray function.

This will further DoS the SecurityCouncilManager.replaceMember function as well. The replaceMember function calls the _swapMembers internal function. The _swapMembers function first calls the _removeMemberFromCohortArray function which removes _addressToRemove element from the cohort array. This will reduce the length of the cohort array by one.

Next it calls the _addMemberToCohortArray function to add the _addressToAdd member. But inside the _addMemberToCohortArray function there is the length equality check (cohort.length == cohortSize) which will revert since the cohort.length is one less after the removal of the member.

Proof of Concept

                    cohort[j] = cohort[cohort.length - 1];
                    cohort.pop();

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

    function addMember(address _newMember, Cohort _cohort) external onlyRole(MEMBER_ADDER_ROLE) {
        _addMemberToCohortArray(_newMember, _cohort);
        _scheduleUpdate();
        emit MemberAdded(_newMember, _cohort);
    }

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Since the cohort.length is used for conditional checks when adding new members to the cohort, it is recommended to only delete the specific member from the cohort when calling the _removeMemberFromCohortArray rather than reducing the array by one element.

This can be done as follows:

                delete cohort[j];

Assessed type

DoS

0xSorryNotSorry commented 1 year ago

The function calls _scheduleUpdate() internally and it calls getScheduleUpdateInnerData(). getScheduleUpdateInnerData() calls SecurityCouncilMemberSyncAction.perform(), and perform() updates the members of security council multisig to match provided array.

Invalid assumption.