Arbitrum Security Council Election System - Sathish9098's results

LOW FINDINGS

[L-1] block.timestamp < thisElectionStartTs check allows to create elections even 6 months not ends

Impact

This check potentially break the docs . This will allow to createElection even 6 month not yet ends

If the condition block.timestamp < thisElectionStartTs is used and block.timestamp is equal to thisElectionStartTs, then the condition would evaluate to false, and the transaction would not revert. This means that the election could be created even if the desired start time thisElectionStartTs has already been reached but not yet ended.

POC

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

166: uint256 thisElectionStartTs = electionToTimestamp(electionCount);
167:        if (block.timestamp < thisElectionStartTs) {
168:            revert CreateTooEarly(block.timestamp, thisElectionStartTs);
169:        }

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L166-L169

Recommended Mitigation

Change the checks like,

167:  if (block.timestamp <= thisElectionStartTs) {

[L-2] electionCount always grow indefinitely, potentially impacting the efficiency and scalability of the network

Impact

The Ethereum blockchain has a finite capacity for storing data. If many contracts follow a pattern of unbounded growth for their state variables, it could contribute to blockchain bloat, potentially impacting the efficiency and scalability of the network. As electionCount grows, the execution time of certain transactions might increase. This could lead to longer confirmation times and delays for users interacting with the contract

POC

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

180: electionCount++;

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L180

Recommended Mitigation

If unbounded growth is necessary, you might need to implement mechanisms to manage and handle large data sets efficiently, such as pagination, data archiving, or off-chain storage solutions.

[L-3] getProposeArgs(electionCount - 1) this may cause problem when the value is 1

Impact

If electionCount always starts with 1 and you use the expression getProposeArgs(electionCount - 1), there could be an issue when the value of electionCount is 1. This is because subtracting 1 from 1 would result in 0, and if your contract's data structure is not designed to handle this scenario, it could lead to unexpected behavior or errors.

POC

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

197: ) = getProposeArgs(electionCount - 1);

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L197

[L-4] Lack of access control in addContender() function

Impact

Anyone could call the addContender function and add themselves as contenders, even if they shouldn't be allowed to participate in the election. This could lead to manipulation of the election process.

POC

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

function addContender(uint256 proposalId) external {
        ElectionInfo storage election = _elections[proposalId];

        if (election.isContender[msg.sender]) {
            revert AlreadyContender(msg.sender);
        }

        ProposalState state_ = state(proposalId);
        if (state_ != ProposalState.Active) {
            revert ProposalNotActive(state_);
        }

        // check to make sure the contender is not part of the other cohort (the cohort not currently up for election)
        // this only checks against the current the current other cohort, and against the current cohort membership
        // in the security council, so changes to those will mean this check will be inconsistent.
        // this check then is only a relevant check when the elections are running as expected - one at a time,
        // every 6 months. Updates to the sec council manager using methods other than replaceCohort can effect this check
        // and it's expected that the entity making those updates understands this.
        if (securityCouncilManager.cohortIncludes(otherCohort(), msg.sender)) {
            revert AccountInOtherCohort(otherCohort(), msg.sender);
        }

        election.isContender[msg.sender] = true;

        emit ContenderAdded(proposalId, msg.sender);
    }

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L218

Recommended Mitigation

Add access control to critical addContender() function

[L-5] threshold not checked . As per docs threshold is never lower than the member count

Impact

The threshold is an important parameter that controls how many members of the security council must approve an update before it can be executed. If the threshold is too low, it could be easier for an attacker to take control of the security council

POC

FILE: governance/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol

55: // preserve current threshold, the safe ensures that the threshold is never lower than the member count
56: uint256 threshold = securityCouncil.getThreshold();

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

Recommended Mitigation

Add threshold check

if (threshold < securityCouncil.getOwners().length) {

[L-6] Lack of control for critical perform() function

Impact

The perform function does not check if the caller is authorized to perform the update. This is a potential security problem, as it could allow an attacker to call the function and add or remove members from the security council without authorization.

POC

FILE: governance/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol

  /// @notice Updates members of security council multisig to match provided array
    /// @dev    This function contains O(n^2) operations, so doesnt scale for large numbers of members. Expected count is 12, which is acceptable.
    ///         Gnosis OwnerManager handles reverting if address(0) is passed to remove/add owner
    /// @param _securityCouncil The security council to update
    /// @param _updatedMembers  The new list of members. The Security Council will be updated to have this exact list of members
    /// @return res indicates whether an update took place
    function perform(address _securityCouncil, address[] memory _updatedMembers, uint256 _nonce)
        external
        returns (bool res)
    {

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

Recommended Mitigation

Add access control to perform() function

[L-7] The replaceCohort function does not check if the old cohort is still in use

Impact

This could allow an COHORT_REPLACER_ROLE to replace a cohort that is still being used, which could cause the security council to be inoperable.

POC

FILE: Breadcrumbsgovernance/src/security-council-mgmt/SecurityCouncilManager.sol

/// @inheritdoc ISecurityCouncilManager
    function replaceCohort(address[] memory _newCohort, Cohort _cohort)
        external
        onlyRole(COHORT_REPLACER_ROLE)
    {
        if (_newCohort.length != cohortSize) {
            revert InvalidNewCohortLength({cohort: _newCohort, cohortSize: cohortSize});
        }

        // delete the old cohort
        _cohort == Cohort.FIRST ? delete firstCohort : delete secondCohort;

        for (uint256 i = 0; i < _newCohort.length; i++) {
            _addMemberToCohortArray(_newCohort[i], _cohort);
        }

        _scheduleUpdate();
        emit CohortReplaced(_newCohort, _cohort);
    }

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

Recommended Mitigation

Before remove or replace any cohort make sure that cohort not in use

[L-8] Lack of event emit when adding new member to Cohort

Impact

The _addMemberToCohortArray function does not emit an event when a new member is added to a cohort. This could make it difficult to track changes to the security council's members.

POC

FILE: governance/src/security-council-mgmt/SecurityCouncilManager.sol

function _addMemberToCohortArray(address _newMember, Cohort _cohort) internal {
        if (_newMember == address(0)) {
            revert ZeroAddress();
        }
        address[] storage cohort = _cohort == Cohort.FIRST ? firstCohort : secondCohort;
        if (cohort.length == cohortSize) {
            revert CohortFull({cohort: _cohort});
        }
        if (firstCohortIncludes(_newMember)) {
            revert MemberInCohort({member: _newMember, cohort: Cohort.FIRST});
        }
        if (secondCohortIncludes(_newMember)) {
            revert MemberInCohort({member: _newMember, cohort: Cohort.SECOND});
        }

        cohort.push(_newMember);
    }

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

Recommended Mitgation

Add event-emit for critical changes

[L-9] generateSalt() function is not secure

Impact

Even if the updateNonce value is increased every time a new proposal is scheduled, the generateSalt function is still not a secure function. This is because the generateSalt function simply concatenates the newMembers array and the updateNonce variable together to create a salt. This salt is not very secure, and it is possible for an attacker to generate a collision and create a valid proposal with the same salt as another proposal

POC

440: salt: this.generateSalt(newMembers, updateNonce),

370: function generateSalt(address[] memory _members, uint256 nonce)
371:        external
372:        pure
373:        returns (bytes32)
374:    {
375:        return keccak256(abi.encodePacked(_members, nonce));
376:    }

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

Recommended Mitigation

To make the generateSalt function more secure, you could use a more secure hash function, such as the sha3 function

[L-10] Division by zero not prevented

Impact

The divisions below take an input parameter which does not have any zero-value checks, which may lead to the functions reverting when zero is passed. params.quorumNumeratorValue should be checked before division operation

POC

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

128: if ((quorumDenominator() / params.quorumNumeratorValue) > 500) {

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L128

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/modules
/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol

252: uint256 decreaseAmount =
253:            votes * (blockNumber - fullWeightVotingDeadline_) / decreasingWeightDuration;

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L252-L253

Recommended Mitigation

Add the non zero check before perform division

[L-11] Lack of sanity checks when assigning uint values to critical variables in constructor and initializer function

Impact

The contract's does not have any sanity checks when assigning uint values to critical variables in the constructor and initializer function. This could lead to human errors that would require the contract to be redeployed.

POC

FILE: governance/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol

41: emergencySecurityCouncilThreshold = _emergencySecurityCouncilThreshold;
42: nonEmergencySecurityCouncilThreshold = _nonEmergencySecurityCouncilThreshold;

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol#L41-L42

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L63-L75

Recommended Mitigation

Add > 0, MAX, MIN value checks

[L-12] Protocol uses the vulnerable version of openzeppelin version

Impact

The Protocol contract uses the vulnerable version of the openzeppelin library. This means that the contract is susceptible to a number of security vulnerabilities that have been patched in newer versions of the library.

Protocol uses the contracts": "4.7.3", contracts-upgradeable": "4.7.3" there are known vulnerability in this version

• Improper Input Validation
• Missing Authorization
• Denial of Service (DoS)
• Improper Verification of Cryptographic Signature

POC

FILE: package.json

69: "@openzeppelin/contracts": "4.7.3",
70: "@openzeppelin/contracts-upgradeable": "4.7.3",

Recommended Mitigation

Use at least openzeppelin : 4.9.2

[L-13] Hardcoded MAX_SECURITY_COUNCILS may cause problem in future

Impact

The MAX_SECURITY_COUNCILS constant in the Protocol contract is hardcoded to a value of 500. This means that there can only be a maximum of 500 security councils in the protocol. This could be a problem in the future if the protocol needs to support more than 10 security councils.

If any problem need to increase/decrease security councils means its not possible. Need to redeploy the over all contract

POC

FILE: governance/src/security-council-mgmt/SecurityCouncilManager.sol

67: uint256 public immutable MAX_SECURITY_COUNCILS = 500;

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

Recommended Mitigation

Make MAX_SECURITY_COUNCILS value configurable

[L-14] Project Upgrade and Stop Scenario should be

At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “.

https://github.com/maxwoe/solidity_patterns/blob/master/security/EmergencyStop.sol

[L-15] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Impact

While it doesn’t save any gas because the compiler knows that developers often make this mistake, it’s still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

FILE: Breadcrumbsgovernance/src/security-council-mgmt/SecurityCouncilManager.sol

79:    bytes32 public constant COHORT_REPLACER_ROLE = keccak256("COHORT_REPLACER");
80:    bytes32 public constant MEMBER_ADDER_ROLE = keccak256("MEMBER_ADDER");
81:    bytes32 public constant MEMBER_REPLACER_ROLE = keccak256("MEMBER_REPLACER");
82:    bytes32 public constant MEMBER_ROTATOR_ROLE = keccak256("MEMBER_ROTATOR");
83:    bytes32 public constant MEMBER_REMOVER_ROLE = keccak256("MEMBER_REMOVER");

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

Recommended Mitigation

Use immutable instead of constant

[L-16] Shorter the inheritance list

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L16-L27

[L-17] initialize() function visibility should be external

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L58-L69

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol#L55

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L103

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

166: uint256 thisElectionStartTs = electionToTimestamp(electionCount);
167:        if (block.timestamp < thisElectionStartTs) {
168:            revert CreateTooEarly(block.timestamp, thisElectionStartTs);
169:        }

167:  if (block.timestamp <= thisElectionStartTs) {

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

197: ) = getProposeArgs(electionCount - 1);

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

function addContender(uint256 proposalId) external {
        ElectionInfo storage election = _elections[proposalId];

        if (election.isContender[msg.sender]) {
            revert AlreadyContender(msg.sender);
        }

        ProposalState state_ = state(proposalId);
        if (state_ != ProposalState.Active) {
            revert ProposalNotActive(state_);
        }

        // check to make sure the contender is not part of the other cohort (the cohort not currently up for election)
        // this only checks against the current the current other cohort, and against the current cohort membership
        // in the security council, so changes to those will mean this check will be inconsistent.
        // this check then is only a relevant check when the elections are running as expected - one at a time,
        // every 6 months. Updates to the sec council manager using methods other than replaceCohort can effect this check
        // and it's expected that the entity making those updates understands this.
        if (securityCouncilManager.cohortIncludes(otherCohort(), msg.sender)) {
            revert AccountInOtherCohort(otherCohort(), msg.sender);
        }

        election.isContender[msg.sender] = true;

        emit ContenderAdded(proposalId, msg.sender);
    }

FILE: governance/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol

55: // preserve current threshold, the safe ensures that the threshold is never lower than the member count
56: uint256 threshold = securityCouncil.getThreshold();

if (threshold < securityCouncil.getOwners().length) {

FILE: Breadcrumbsgovernance/src/security-council-mgmt/SecurityCouncilManager.sol

/// @inheritdoc ISecurityCouncilManager
    function replaceCohort(address[] memory _newCohort, Cohort _cohort)
        external
        onlyRole(COHORT_REPLACER_ROLE)
    {
        if (_newCohort.length != cohortSize) {
            revert InvalidNewCohortLength({cohort: _newCohort, cohortSize: cohortSize});
        }

        // delete the old cohort
        _cohort == Cohort.FIRST ? delete firstCohort : delete secondCohort;

        for (uint256 i = 0; i < _newCohort.length; i++) {
            _addMemberToCohortArray(_newCohort[i], _cohort);
        }

        _scheduleUpdate();
        emit CohortReplaced(_newCohort, _cohort);
    }

440: salt: this.generateSalt(newMembers, updateNonce),

370: function generateSalt(address[] memory _members, uint256 nonce)
371:        external
372:        pure
373:        returns (bytes32)
374:    {
375:        return keccak256(abi.encodePacked(_members, nonce));
376:    }

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

128: if ((quorumDenominator() / params.quorumNumeratorValue) > 500) {

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/modules
/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol

252: uint256 decreaseAmount =
253:            votes * (blockNumber - fullWeightVotingDeadline_) / decreasingWeightDuration;

FILE: package.json

69: "@openzeppelin/contracts": "4.7.3",
70: "@openzeppelin/contracts-upgradeable": "4.7.3",

FILE: governance/src/security-council-mgmt/SecurityCouncilManager.sol

67: uint256 public immutable MAX_SECURITY_COUNCILS = 500;

FILE: governance/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol

41: emergencySecurityCouncilThreshold = _emergencySecurityCouncilThreshold;
42: nonEmergencySecurityCouncilThreshold = _nonEmergencySecurityCouncilThreshold;

GAS OPTIMIZATION

All gas optimizations is calculated based on opcodes

[G-1] Using calldata instead of memory for read-only arguments in external functions saves gas

Saved 2820 GAS

When a function with a memory array is called externally, the abi.decode () step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution . Saves at least 282 GAS per instance .

_firstCohort,_secondCohor,_securityCouncils,_roles can be calldata instead of memory since all parameters are read only : Saves 1974 GAS

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

FILE: governance/src/security-council-mgmt/SecurityCouncilManager.sol

89: function initialize(
- 90:        address[] memory _firstCohort,
+ 90:        address[] calldata _firstCohort,
- 91:        address[] memory _secondCohort,
+ 91:        address[] calldata _secondCohort,
- 92:        SecurityCouncilData[] memory _securityCouncils,
+ 92:        SecurityCouncilData[] calldata _securityCouncils,
- 93:        SecurityCouncilManagerRoles memory _roles,
+ 93:        SecurityCouncilManagerRoles calldata _roles,
94:        address payable _l2CoreGovTimelock,
95:        UpgradeExecRouteBuilder _router
96:    ) external initializer {


- 124: function replaceCohort(address[] memory _newCohort, Cohort _cohort)
+ 124: function replaceCohort(address[] memory calldata, Cohort _cohort)
125:        external
126:        onlyRole(COHORT_REPLACER_ROLE)
127:    {

- 271: function addSecurityCouncil(SecurityCouncilData memory _securityCouncilData)
+ 271: function addSecurityCouncil(SecurityCouncilData calldata _securityCouncilData)
272:        external
273:        onlyRole(DEFAULT_ADMIN_ROLE)
274:    {

- 279: function removeSecurityCouncil(SecurityCouncilData memory _securityCouncilData)
+ 279: function removeSecurityCouncil(SecurityCouncilData calldata _securityCouncilData)
280:        external
281:        onlyRole(DEFAULT_ADMIN_ROLE)
282:        returns (bool)

_updatedMembers can be calldata instead of memory : Saves 282 GAS

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

FILE: Breadcrumbsgovernance/src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol

- 31: function perform(address _securityCouncil, address[] memory _updatedMembers, uint256 _nonce)
+ 31: function perform(address _securityCouncil, address[] calldata _updatedMembers, uint256 _nonce)
32:        external
33:        returns (bool res)
34:    {

dp,impls can be calldata instead of memory : Saves 564 GAS

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/factories/L2SecurityCouncilMgmtFactory.sol#L81

FILE: governance/src/security-council-mgmt/factories/L2SecurityCouncilMgmtFactory.sol

81: function deploy(DeployParams memory dp, ContractImplementations memory impls)
82:        external
83:        onlyOwner
84:        returns (DeployedContracts memory)

[G-2] Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

Saves 8000 GAS, 4 SLOT

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

+    struct UserStatus {
+        bool isContender;
+        bool isExcluded;
+    }


54: struct ElectionInfo {
- 55:        mapping(address => bool) isContender;
- 56:        mapping(address => bool) isExcluded;
+           UserStatus userStatus;
57:        uint256 excludedNomineeCount;
58:    }

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L55-L56

FILE: governance/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol

+ struct NomineeCountingInfo {
+        uint256 votesUsed;
+        uint256 votesReceived;
+        bool isNominee;
+    }

18: struct NomineeElectionCountingInfo {
+    NomineeCountingInfo _nomineeCountingInfo ;
- 19:        mapping(address => uint256) votesUsed;
- 20:        mapping(address => uint256) votesReceived;
21:        address[] nominees;
- 22:        mapping(address => bool) isNominee;
23:    }

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol#L19-L20

[G-3] State variables can be packed into fewer storage slots

Saves 4000 GAS, 2 SLOT

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper.

voteSuccessNumerator can be uint96 instead of uint256 because of this check _voteSuccessNumerator <= voteSuccessDenominator). The value always less than 10000 because voteSuccessDenominator constant : Saves 2000 GAS, 1 SLOT

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L28C1-L30

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol

28:  uint256 public constant voteSuccessDenominator = 10_000;
- 29:    uint256 public voteSuccessNumerator;
+ 29:    uint96 public voteSuccessNumerator;
30:    ISecurityCouncilManager public securityCouncilManager;

cohortSize can be changed uint96 from uint256. cohortSize saves only _firstCohort.length _firstCohort parameter length. The array length is not going to exceeds uint96 range 18446744073709551615

: Saves 2000 GAS, 1 SLOT

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

FILE: Breadcrumbsgovernance/src/security-council-mgmt/SecurityCouncilManager.sol


 /// @notice Address of the l2 timelock used by core governance
    address payable public l2CoreGovTimelock;
+   uint96 public cohortSize;

    /// @notice The list of Security Councils under management. Any changes to the cohorts in this manager
    ///         will be pushed to each of these security councils, ensuring that they all stay in sync
    SecurityCouncilData[] public securityCouncils;

    /// @notice Address of UpgradeExecRouteBuilder. Used to help create security council updates
    UpgradeExecRouteBuilder public router;

    /// @notice Maximum possible number of Security Councils to manage
    /// @dev    Since the councils array will be iterated this provides a safety check to make too many Sec Councils
    ///         aren't added to the array.
    uint256 public immutable MAX_SECURITY_COUNCILS = 500;

    /// @notice Nonce to ensure that scheduled updates create unique entries in the timelocks
    uint256 public updateNonce;

    /// @notice Size of cohort under ordinary circumstancces
-    uint256 public cohortSize;

[G-4] IF’s/require() statements that check input arguments should be at the top of the function

Saves 10000 GAS

FAIL CHEAPLY INSTEAD OF COSTLY

Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.

!Address.isContract(address(params.securityCouncilManager),!Address.isContract(address(params.securityCouncilMemberElectionGovernor), (quorumDenominator() / params.quorumNumeratorValue) > 500) should performed top of the the functions. This will avoid the unwanted storage writes : Saves around 6000 GAS . If any revert after storage writes this wastes the computation cost

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L111-L130

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

+        if (!Address.isContract(address(params.securityCouncilManager))) {
+            revert NotAContract(address(params.securityCouncilManager));
+        }
+        if (!Address.isContract(address(params.securityCouncilMemberElectionGovernor))) {
+            revert NotAContract(address(params.securityCouncilMemberElectionGovernor));
+        }

+        if ((quorumDenominator() / params.quorumNumeratorValue) > 500) {
+            revert QuorumNumeratorTooLow(params.quorumNumeratorValue);
+        }


      _transferOwnership(params.owner);

        nomineeVetter = params.nomineeVetter;
-        if (!Address.isContract(address(params.securityCouncilManager))) {
-            revert NotAContract(address(params.securityCouncilManager));
-        }
        securityCouncilManager = params.securityCouncilManager;
-        if (!Address.isContract(address(params.securityCouncilMemberElectionGovernor))) {
-            revert NotAContract(address(params.securityCouncilMemberElectionGovernor));
-        }
        securityCouncilMemberElectionGovernor = params.securityCouncilMemberElectionGovernor;

        // elsewhere we make assumptions that the number of nominees
        // is not greater than 500
        // This value can still be updated via updateQuorumNumerator to a lower value
        // if it is deemed ok, however we put a quick check here as a reminder
-        if ((quorumDenominator() / params.quorumNumeratorValue) > 500) {
-            revert QuorumNumeratorTooLow(params.quorumNumeratorValue);
-        }

!Address.isContract(address(_nomineeElectionGovernor)),!Address.isContract(address(_securityCouncilManager) should be checked top of the function : Saves 4000 GAS

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol#L68-L74

FILE: governance/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol


if (_fullWeightDuration > _votingPeriod) {
            revert InvalidDurations(_fullWeightDuration, _votingPeriod);
        }

+        if (!Address.isContract(address(_nomineeElectionGovernor))) {
+            revert NotAContract(address(_nomineeElectionGovernor));
+        }

+        if (!Address.isContract(address(_securityCouncilManager))) {
+            revert NotAContract(address(_securityCouncilManager));
+        }

        __Governor_init("SecurityCouncilMemberElectionGovernor");
        __GovernorVotes_init(_token);
        __SecurityCouncilMemberElectionGovernorCounting_init({
            initialFullWeightDuration: _fullWeightDuration
        });
        __GovernorSettings_init(0, _votingPeriod, 0);
        _transferOwnership(_owner);

-        if (!Address.isContract(address(_nomineeElectionGovernor))) {
-            revert NotAContract(address(_nomineeElectionGovernor));
-        }
        nomineeElectionGovernor = _nomineeElectionGovernor;
-        if (!Address.isContract(address(_securityCouncilManager))) {
-            revert NotAContract(address(_securityCouncilManager));
-        }
        securityCouncilManager = _securityCouncilManager;
    }

[G-5] Don't emit state variable when stack variable available

Saves 100 GAS, 1 SLOD

Emitting stack variable is more gas efficient than state variable . Saves 100 GAS

prevWeightReceived + weight should be used : saves 100 GAS, 1 SLOD

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol#L138

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol

 election.weightReceived[nominee] = prevWeightReceived + weight;

        emit VoteCastForNominee({
            voter: account,
            proposalId: proposalId,
            nominee: nominee,
            votes: votes,
            weight: weight,
            totalUsedVotes: prevVotesUsed + votes,
            usableVotes: availableVotes,
-          weightReceived: election.weightReceived[nominee]
+          weightReceived: prevWeightReceived + weight
        });

[G-6] Using bools for storage incurs overhead

Saves 60000 GAS, 3 Instances

   // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

55:        mapping(address => bool) isContender;
56:        mapping(address => bool) isExcluded;
 

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L55-L56

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol

22:        mapping(address => bool) isNominee;

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol#L22

[G-7] Structs can be packed into fewer storage slots

Saves 4000 GAS, 2 SLOT

Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct.

quorumNumeratorValue can be uint96 instead of uint256. As per this check if ((quorumDenominator() / params.quorumNumeratorValue) > 500) { uint96 is safe. Since quorumDenominator() function always return 10000 . So the numerator not exceeds denominator . If exceeds this will revert with divide by zero exception : Saves 2000 GAS, 1 SLOT

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L38-L48

FILE: Breadcrumbsgovernance/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

38: struct InitParams {
39:        Date firstNominationStartDate;
40:        uint256 nomineeVettingDuration;
41:        address nomineeVetter;
42:        ISecurityCouncilManager securityCouncilManager;
43:        ISecurityCouncilMemberElectionGovernor securityCouncilMemberElectionGovernor;
44:        IVotesUpgradeable token;
45:        address owner;
+ 46:        uint96 quorumNumeratorValue;
- 46:        uint256 quorumNumeratorValue;
47:        uint256 votingPeriod;
48:    }

chainId can be uint96 instead of uint256. Saves 2000 GAS, 1 SLOT

https://github.com/ArbitrumFoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/UpgradeExecRouteBuilder.sol#L22-L25

The chainId is a parameter that uniquely identifies different Ethereum networks. The chainId is typically used to prevent replay attacks when creating transactions or signing messages that are intended for a specific network. Ethereum chains generally have values in the range of 1 to 4294967295 (32-bit unsigned integer range).

Using a uint96 instead of a uint256 for the chainId could potentially save some gas cost in storage and execution, as smaller data types consume less storage space and require less computational effort to manipulate.

FILE: governance/src/UpgradeExecRouteBuilder.sol

17: struct UpExecLocation {
18:    address inbox; // Inbox should be set to address(0) to signify that the upgrade executor is on the L1/host chain
19:    address upgradeExecutor;
20: }


22: struct ChainAndUpExecLocation {
- 23:    uint256 chainId;
+ 23:    uint96 chainId;
24:    UpExecLocation location;
25: }

Analysis - Arbitrum Security Council Election System

Summary

Overview

Arbitrum is a suite of scaling solutions providing environments with high-throughput, low-cost smart contracts, backed by industry-leading proving technology rooted in Ethereum.

Arbitrum DAO - Governs the Arbitrum One and Nova chains

The Arbitrum DAO is a decentralized autonomous organization (DAO) built on the Ethereum blockchain. At its core, the Arbitrum DAO is a community-driven governance mechanism that allows $ARB token holders to propose and vote on changes to the organization and the technologies it governs.

The Arbitrum DAO's governance smart contracts are implemented on the Arbitrum One rollup chain, which is a Layer 2 scaling solution for the Ethereum blockchain. These smart contracts include the DAO's governance token, $ARB. DAO members use $ARB tokens to vote on Arbitrum DAO proposals (AIPs). The weight of any given voter's vote is proportional to the amount of $ARB they hold (or represent)1.

The Arbitrum DAO has a built-in treasury system (implemented as a smart contract); the DAO's treasury is used to fund ongoing development and maintenance of the organization and its technologies. Token holders can propose and vote on how to use the treasury's funds

Security Council

This is responsible for making emergency response decisions when the community needs to address a critical security risk to the protocol. They are a 12 member council from independent organisations 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.

The main contributers in over all protocols

Contender

Someone who can receive votes in the nominee selection stage but who has received less than 0.2% of votable tokens

Nominee

Someone who has received 0.2% of votable tokens in the nominee selection stage compliant / noncompliant indicates whether they’ve been excluded by the nominee vetter.

Member

A member of the Security council

Audit approach

I followed below steps while analyzing and auditing the code base.

1. Read the contest Readme.md and took the required notes.

• Arbitrum Security Council Election System
  • Inheritance
  • Test Coverage - 94%
  • Use rollups
  • Multi-Chain
  • Timelocks have code associated with them to handle crosschain messages
  • smart contracts deployed in Ethereum and Arbitrum

2. Analyzed the over all codebase one iterations very fast

3. Study of documentation to understand each contract purposes, its functionality, how it is connected with other contracts, etc.

4. Then i read old audits and already known findings. Then go through the bot races findings

5. Then setup my testing environment things. Run the tests to checks all test passed. I used foundryup and make to test Arbitrum Security Council Election System .

Commands Used for testing:

• make install
• make build
• make sc-election-test
• make coverage, make gas

6. Finally, I started with the auditing the code base in depth way I started understanding line by line code and took the necessary notes to ask some questions to sponsors.

Stages of audit

• The first stage of the audit

During the initial stage of the audit for Arbitrum Security Council Election System, the primary focus was on analyzing gas usage and quality assurance (QA) aspects. This phase of the audit aimed to ensure the efficiency of gas consumption and verify the robustness of the platform.

Gas Optimizations

Found 7 different gas optimizations and totally saved 88920 GAS

QA Analysis

Found 10 Low risk findings

• The second stage of the audit

In the second stage of the audit for Arbitrum Security Council Election System, the focus shifted towards understanding the protocol usage in more detail. This involved identifying and analyzing the important contracts and functions within the system. By examining these key components, the audit aimed to gain a comprehensive understanding of the protocol's functionality and potential risks.

• The third stage of the audit

During the third stage of the audit for Arbitrum Security Council Election System, the focus was on thoroughly examining and marking any doubtful or vulnerable areas within the protocol. This stage involved conducting comprehensive vulnerability assessments and identifying potential weaknesses in the system. Found 60-70 vulnerable and weakness code parts all marked with @audit tags.

• The fourth stage of the audit

During the fourth stage of the audit for Arbitrum Security Council Election System, a comprehensive analysis and testing of the previously identified doubtful and vulnerable areas were conducted. This stage involved diving deeper into these areas, performing in-depth examinations, and subjecting them to rigorous testing, including fuzzing with various inputs. Finally concluded findings after all research's and tests. Then i reported C4 with proper formats

Learnings

As per protocols docs i learned following things

• Decentralized Governance: The Arbitrum DAO plays a crucial role in governing both the Arbitrum One and Nova chains. This demonstrates a decentralized approach to decision-making and management of the protocol.

• Emergency Response with Security Council : The Security Council serves as a mechanism for addressing critical security risks promptly. This highlights the importance of having a specialized group with the authority to conduct emergency actions when needed.

• Cohort-Based Council : The Security Council consists of 12 members from independent organizations, divided into two cohorts. This approach ensures regular alternation of council members, promoting diversity and preventing concentration of power.

• Elections and Constitution : The elections for the Security Council adhere to a predefined constitution, emphasizing the importance of transparency, fairness, and predictable processes.

• On-Chain Governance Implementation : The Arbitrum Governance subsystem is implemented using on-chain smart contracts written in Solidity. This ensures that governance processes are transparent, tamper-resistant, and enforceable.

• Roles and Terminology : The usage of terms like "contender," "nominee," "compliant," "noncompliant," and "member" provides clear distinctions for different stages and statuses within the governance process.

Possible Systemic Risks

1. Operational Clashes: As noted, there's a possibility of operational clashes between Core Governance and election operations due to overlapping timelocks. This could potentially cause delays or block certain actions if not managed properly
2. Security Council Updates: The Security Council's ability to create updates that might impact election execution could lead to failures if these updates aren't carefully managed. This risk highlights the importance of responsible and thorough decision-making within the Security Council.
3. Election Contract Logic Errors: Errors or vulnerabilities in the election contract logic could lead to unexpected behavior, security breaches, or unintended outcomes during the election process
4. Constitution and Code Misalignment: There's a risk that the implemented code doesn't perfectly align with the Arbitrum Constitution. Such discrepancies could lead to confusion, disputes, or even security vulnerabilities if not promptly addressed.
5. Security Council Concentration of Power: While the Security Council holds the responsibility for emergency actions, there's a potential risk of centralization of power within this group. Ensuring diversity, transparency, and accountability within the council can help mitigate this risk.
6. Limited Participation: The requirement for candidates to have a certain percentage of votable tokens (0.2%) might exclude potential contributors who have valid viewpoints but lack the required threshold, leading to limited representation.

Centralization risks

A single point of failure is not acceptable for this project Centrality risk is high in the project, the role of onlyOwnerdetailed below has very critical and important powers

Project and funds may be compromised by a malicious or stolen private key onlyOwner msg.sender

File: src/security-council-mgmt/factories/L2SecurityCouncilMgmtFactory.sol

81       function deploy(DeployParams memory dp, ContractImplementations memory impls)
82           external
83           onlyOwner
84           returns (DeployedContracts memory)
85:      {

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/factories/L2SecurityCouncilMgmtFactory.sol#L81-L85

File: src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol

103      function relay(address target, uint256 value, bytes calldata data)
104          external
105          virtual
106          override
107          onlyOwner
108:     {

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol#L103-L108

File: src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol

178:     function setVoteSuccessNumerator(uint256 _voteSuccessNumerator) public onlyOwner {

203      function relay(address target, uint256 value, bytes calldata data)
204          external
205          virtual
206          override
207          onlyOwner
208:     {

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol#L178-L178

File: src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol

254      function relay(address target, uint256 value, bytes calldata data)
255          external
256          virtual
257          override
258          onlyOwner
259:     {

https://github.com/arbitrumfoundation/governance/blob/c18de53820c505fc459f766c1b224810eaeaabc5/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol#L254-L259

Gas Optimizations

In the pursuit of gas-efficient smart contracts, several key optimizations have been identified that can enhance performance and resource utilization. These optimizations focus on improving gas efficiency in various aspects of contract design and execution.

One prominent optimization involves the strategic use of calldata over memory for read-only arguments in external functions. By directly utilizing calldata, the need for a loop-driven process, such as abi.decode(), to copy calldata to memory is circumvented. This results in streamlined contract code and more efficient runtime execution.

Another significant optimization suggests consolidating multiple address/ID mappings into a single mapping that associates addresses/IDs with corresponding data structures. This approach not only simplifies data management but also leads to reduced storage usage and more efficient write operations. The resulting optimization enhances both gas efficiency and contract performance.

A third optimization entails the careful packing of state variables into fewer storage slots. This practice leads to minimized gas costs for both reading and writing variables, especially when variables within the same slot need to be updated in a single function. This strategy optimizes storage space and contributes to efficient contract execution.

Furthermore, organizing if conditions and require() statements that validate input arguments at the top of functions is advised. This prudent arrangement helps in early detection of invalid inputs, resulting in swift reverts and avoiding unnecessary computational overhead. This optimization is a proactive measure to enhance contract reliability and execution efficiency.

In the context of event emission, emitting stack variables instead of state variables is recommended. Emitting stack variables conserves gas and improves execution efficiency during contract operations that involve emitting events.

The optimization of storage usage is also extended to the choice of data types. Replacing bool with uint256(1) and uint256(2) for representing true and false avoids additional storage read and write operations, thus contributing to gas savings and efficient contract execution.

Lastly, the optimization of storage slots is achieved by packing structs into fewer slots. This practice leads to more cost-effective write operations and minimizes the need for additional storage-related operations, thus enhancing both gas efficiency and contract performance.

These optimizations collectively contribute to the creation of gas-efficient smart contracts that operate smoothly and effectively on the blockchain. By integrating these strategies into contract design and development, developers can harness improved performance, reduced gas costs, and optimal resource utilization.

Code Commentary

General Code suggestions for all contracts

1. Use more recent version of solidity
2. Add specific imports instead of over all
3. Try to reduce the contract names size . Now the contract names really very hard to understand and very big names
4. Follow the namings as per solidity naming standards . internal/private function and variable names always start with _ . Example address[] internal firstCohort; address[] internal secondCohort;
5. Don't initailize default values in loops . Like for (uint256 i = 0; i < _securityCouncils.length; i++) {
6. address.iscontract() checks can use modifiers
7. Hard coded values should be avoided
8. Provide meaningful error messages in require statements to enhance debugging
9. Add comments to explain complex logic, calculations, and the purpose of functions

Only wrote 2 contracts reports. This covers the majority of the suggestions.

SecurityCouncilManager.sol

• There's some code duplication between _addMemberToCohortArray and _removeMemberFromCohortArray. You could potentially refactor this logic into a separate private function to reduce redundancy

• In _removeMemberFromCohortArray, you're iterating twice through both cohorts to remove the member. Instead, you can optimize this by using a single loop that checks both cohorts and removes the member when found.

• Emitting events is a good practice for keeping track of contract state changes. Ensure that all relevant state changes are accompanied by corresponding event emissions. Some critical functions like _addMemberToCohortArray,_removeMemberFromCohortArray not emit any events .

• If the SecurityCouncilData structure is used in multiple places, consider using a struct to define it. This can help centralize the structure and make it easier to manage changes

• Consider defining interfaces for access control roles. This can make it clearer what each role is responsible for and can improve the overall readability of your code.

• Instead of using if statements followed by revert, consider using require statements. It makes the code cleaner and clearer

• If you anticipate the need for upgrades in the future, you might want to consider using a pattern like the OpenZeppelin's Upgrades Plugins to enable seamless contract upgrades while preserving storage data.

• Create a modifier to check for zero addresses and reuse it in functions that require this check

SecurityCouncilNomineeElectionGovernor.sol

• If possible, order the struct members based on their size (from largest to smallest) to minimize gas usage due to potential padding.

• initialize() function should be external

• Reduce the inheritance list

• To improve readability, organize functions and modifiers in a consistent order. For example, you can list constructors first, followed by external functions, internal functions, and then modifiers

• In the initialize function, add checks to ensure that address parameters (nomineeVetter, securityCouncilManager, securityCouncilMemberElectionGovernor) are not set to the zero address.

• In _requireLastMemberElectionHasExecuted instead of directly reverting, you could emit a custom event when the requirement is not met. This event can be useful for monitoring the state of the contract and providing more context on the error.

• If some of your functions are complex, consider breaking them down into smaller functions with descriptive names. This can improve code readability and maintainability.

• Instead of scattering the initialization steps throughout the initialize function, consider consolidating them into a separate internal function. This can improve the overall readability of the initialize function.

• Replace magic numbers (e.g., 500) with named constants or variables to improve code readability and maintainability. This makes it easier to understand the purpose of these numbers in context.

Risks as per Analysis

SecurityCouncilManager.sol

• The contract manipulates arrays (firstCohort and secondCohort) to add, remove, and replace members. Incorrect array manipulation can result in unintended state changes or even data corruption.

• If the arrays firstCohort and secondCohort grow too large, there could be a risk of hitting gas limits when manipulating these arrays, leading to transactions failing or becoming too expensive.

• The contract relies on external contracts like UpgradeExecRouteBuilder and ArbitrumTimelock. Any vulnerabilities or misconfigurations in these contracts can impact the security of this contract.

• Add bounds checking when accessing arrays to prevent out-of-bounds errors.

• Replace the custom role-based access control with OpenZeppelin's AccessControl contract for more standardized and tested access control.

SecurityCouncilNomineeElectionGovernor.sol

• The replaceCohort function replaces a cohort with a new set of members. It is crucial to ensure that this operation is atomic and prevents any race conditions to maintain data consistency across the contract's state.

• The contract relies on UpgradeExecRouteBuilder to construct security council updates. If there are vulnerabilities or malicious behaviors in this external contract, it could compromise the entire process.

• If this nonce is not managed correctly or is predictable, attackers could manipulate timelock actions and execute unauthorized updates

• The interaction with the L2 timelock contract is crucial. If there are vulnerabilities in the ArbitrumTimelock contract or if the contract's interaction with it is not secure, it could result in unauthorized changes being scheduled.

• The contract relies on external data, such as router and l2CoreGovTimelock. If these external contracts change their behavior or state unexpectedly, it could disrupt the contract's operations.

Risks Found Based on QA Reports

1. If the condition block.timestamp < thisElectionStartTs is used and block.timestamp is equal to thisElectionStartTs, then the condition would evaluate to false, and the transaction would not revert. This means that the election could be created even if the desired start time thisElectionStartTs has already been reached but not yet ended.

2. The threshold is an important parameter that controls how many members of the security council must approve an update before it can be executed. If the threshold is too low, it could be easier for an attacker to take control of the security council

3. The perform function does not check if the caller is authorized to perform the update. This is a potential security problem, as it could allow an attacker to call the function and add or remove members from the security council without authorization.

4. This could allow an COHORT_REPLACER_ROLE to replace a cohort that is still being used, which could cause the security council to be inoperable.

5. Even if the updateNonce value is increased every time a new proposal is scheduled, the generateSalt function is still not a secure function. This is because the generateSalt function simply concatenates the newMembers array and the updateNonce variable together to create a salt. This salt is not very secure, and it is possible for an attacker to generate a collision and create a valid proposal with the same salt as another proposal.

6. The divisions below take an input parameter which does not have any zero-value checks, which may lead to the functions reverting when zero is passed. params.quorumNumeratorValue should be checked before division operation

7. The contract's does not have any sanity checks when assigning uint values to critical variables in the constructor and initializer function. This could lead to human errors that would require the contract to be redeployed.

8. The Protocol contract uses the vulnerable version of the openzeppelin library. This means that the contract is susceptible to a number of security vulnerabilities that have been patched in newer versions of the library.Protocol uses the contracts": "4.7.3", contracts-upgradeable": "4.7.3" there are known vulnerability in this version

9. The MAX_SECURITY_COUNCILS constant in the Protocol contract is hardcoded to a value of 500. This means that there can only be a maximum of 500 security councils in the protocol. This could be a problem in the future if the protocol needs to support more than 10 security councils.If any problem need to increase/decrease security councils means its not possible. Need to redeploy the over all contract .

10. At the start of the project, the system may need to be stopped or upgraded, I suggest you have a script beforehand and add it to the documentation. This can also be called an ” EMERGENCY STOP (CIRCUIT BREAKER) PATTERN “.

Non-functional aspects

• Aim for high test coverage to validate the contract's behavior and catch potential bugs or vulnerabilities
• The protocol execution flow is not explained in efficient way.
• Its best to explain over all protocol with architecture is easy to understandings

Time spent on analysis

15 Hours

Time spent:

15 hours