Anyone can call `perform` in `SecurityCouncilMemberSyncAction` to update members of security council multisig

[code423n4]

Lines of code

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol#L32

Vulnerability details

Impact

Anyone can update members of security council multisig

Proof of Concept

SecurityCouncilMemberSyncAction contract has a perform function which is used to update members of security council multisig.

File: SecurityCouncilMemberSyncAction.sol

  /// @notice Updates members of security council multisig to match provided array
  //-------------SNIP---------------------//
  function perform(address _securityCouncil, address[] memory _updatedMembers, uint256 _nonce)
      external
      returns (bool res) // @audit Anyone can call it
  {
      //-------------SNIP---------------------//
      uint256 updateNonce = getUpdateNonce(_securityCouncil);
      if (_nonce <= updateNonce) {
          // when nonce is too now, we simply return, we don't revert.
          // this way an out of date update will actual execute, rather than remaining in an unexecuted state forever
          emit UpdateNonceTooLow(_securityCouncil, updateNonce, _nonce);
          return false;
      }

      // store the nonce as a record of execution
      // use security council as the key to ensure that updates to different security councils are kept separate
      _setUpdateNonce(_securityCouncil, _nonce);
      //-------------SNIP---------------------//

      for (uint256 i = 0; i < _updatedMembers.length; i++) {
          address member = _updatedMembers[i];
          if (!securityCouncil.isOwner(member)) {
              _addMember(securityCouncil, member, threshold);
          }
      }

      for (uint256 i = 0; i < previousOwners.length; i++) {
          address owner = previousOwners[i];
          if (!SecurityCouncilMgmtUtils.isInArray(owner, _updatedMembers)) {
              _removeMember(securityCouncil, owner, threshold);
          }
      }
      return true;
  }

Link to Code

Here, the list of updated members: _updatedMembers is provided by anyone who calls it. _nonce is also controlled by the caller.

So anyone can:

1. Call perform with list of their controlled addresses for _updatedMembers.
2. To pass _nonce condition, they need to pass any value greater than current updateNonce value.
3. The _updatedMembers addresses will become members of security council multisig while previous members will be removed.

Tools Used

VS Code

Recommended Mitigation Steps

Have an Access control for the perform function.

Assessed type

Access Control

[0xSorryNotSorry]

As per the NATSPEC of the contract;

@notice Action contract for updating security council members. Used by the security council management system.
Expected to be delegate called into by an Upgrade Executor

Invalid assumption as there is no member state at the contract.

[c4-pre-sort]

0xSorryNotSorry marked the issue as low quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid