`SecurityCouncilManager` : `removeMember` does not make sure that the member is removed sucessfully.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Incorrect information emitted as removed address. Remover would be thinking that they have successfully removed the harm causing cohort members, but still they are sitting inside the council. This is not good given the nature of the harmed action that council member can cause. Sitting in very short span time that can cause huge impact. hence timely removal of members are important.

Proof of Concept

when we look at the removeMember, it is called by the MEMBER_REMOVER_ROLE with any member address.

function removeMember(address _member) external onlyRole(MEMBER_REMOVER_ROLE) {
    if (_member == address(0)) {
        revert ZeroAddress();
    }
    Cohort cohort = _removeMemberFromCohortArray(_member);
    _scheduleUpdate();
    emit MemberRemoved({member: _member, cohort: cohort});
}

This function ensure that no zero address is passed.

when we look at the _removeMemberFromCohortArray , it travers the two set of arrays and look to removal.

function _removeMemberFromCohortArray(address _member) internal returns (Cohort) {
    for (uint256 i = 0; i < 2; i++) {
        address[] storage cohort = i == 0 ? firstCohort : secondCohort;
        for (uint256 j = 0; j < cohort.length; j++) {
            if (_member == cohort[j]) {
                cohort[j] = cohort[cohort.length - 1];
                cohort.pop();
                return i == 0 ? Cohort.FIRST : Cohort.SECOND;
            }
        }
    }
    revert NotAMember({member: _member});
}

If both of the cohort array does not have the input member that want to be removed, no one will be removed and the council numbers remain same.

but the function removeMember emits that the member is removed.

Tools Used

Recommended Mitigation Steps

Update the codes inside the function _removeMemberFromCohortArray

If number of council members are same before and after removal, revert the function call.

Assessed type

Context

[0xSorryNotSorry]

If the both members are not found, the function reverts with the following;

revert NotAMember({member: _member});

Invalid assumption.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid