Platform: Code4rena
Start Date: 03/08/2023
Pot Size: $90,500 USDC
Total HM: 6
Participants: 36
Period: 7 days
Judge: 0xean
Total Solo HM: 1
Id: 273
League: ETH
Rank: 12/36
Findings: 1
Award: $815.43
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: catellatech
Also found by: 0xSmartContract, 0xnev, K42, MSK, Sathish9098, berlin-101, hals, kodyvim, yixxas
815.4315 USDC - $815.43
Arbitrum is a set of solutions designed to accelerate and make applications in blockchain more cost-effective. This is achieved by processing multiple transactions at once without incurring high costs. These solutions are based on Ethereum technology, a well-known blockchain platform.
Within the Arbitrum environment, there's a group called the "Arbitrum DAO" that makes decisions affecting two chains named "Arbitrum One" and "Nova". These decisions are crucial to ensure the security and proper functioning of these chains: When a new L2 chain is authorized by the DAO, the following steps should be carried out for the new chain to become DAO-governed:
In addition, there's the "Security Council", consisting of 12 individuals from various organizations. This council takes on the responsibility of making quick decisions in the event of a significant security issue arising in the Arbitrum chains. They have the authority to implement urgent actions and resolve such problems. This council is divided into two groups, each of which is elected alternately every six months.
The process of selecting these groups is referred to as "elections" and is governed by a system of smart contracts on the chain. Terms like "contenders," "nominees," and "members" are used to describe the different stages and roles within this process.
The engagement involved auditing twenty different Solidity Smart Contracts:
SecurityCouncilManager.sol: Serves as the definitive source of the current status for all security councils. It initiates updates across different chains and collaborates with the UpgradeExecRouteBuilder to craft the necessary payload.
SecurityCouncilNomineeElectionGovernor.sol: Empowers delegates to vote and select nominees from a broader pool of contenders. This module introduces a vetting period, allowing a trusted entity to include or exclude contenders based on their credibility.
SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol: This module handles the vote counting mechanism within the nominee governor system.
SecurityCouncilNomineeElectionGovernorTiming.sol: Provides timing-related functionalities for the nominee election governor module.
SecurityCouncilMemberElectionGovernor.sol: Enables delegates to vote for 6 candidates from a shortlist of nominees.
SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol: Manages the vote counting process within the member election governor module.
SecurityCouncilMemberRemovalGovernor.sol: Facilitates the voting process for the removal of a candidate from the existing security council.
ArbitrumGovernorVotesQuorumFractionUpgradeable.sol: Core functionality focused on counting only "votable" tokens.
ElectionGovernor.sol: Provides shared functionalities that are utilized by various election governors.
UpgradeExecRouteBuilder.sol: Constructs routes that target the upgrade executors on each respective chain.
SecurityCouncilMemberSyncAction.sol: An action contract responsible for updating the member list of the Gnosis Safe.
SecurityCouncilMgmtUtils.sol: Houses shared array utilities to enhance code efficiency.
Common.sol: Contains shared data structures that are widely used.
L2SecurityCouncilMgmtFactory.sol: Deploys contracts related to the election process.
GovernanceChainSCMgmtActivationAction.sol: Activates the election process on Arbitrum One.
L1SCMgmtActivationAction.sol: Initiates the election process on the L1 Ethereum chain.
NonGovernanceChainSCMgmtActivationAction.sol: Activates the election process on Arbitrum Nova.
SecurityCouncilMgmtUpgradeLib.sol: Contains shared utilities to streamline the management process.
KeyValueStore.sol: Functions as a repository for values linked to specific keys, serving as external storage to prevent actions from directly utilizing state.
ActionExecutionRecord.sol: Stores a record of executed actions for future reference.
During the analysis, our approach it was to identify key contracts and their interactions within the Security Council management system. To determine which contract was the main one, we examined the hierarchy and responsibilities of the contracts provided. The main contract is the "Security Council Manager (SCM)," as it appears to be the central core of the system that coordinates and manages various governance functions related to the Security Council.
Starting from that main contract, we constructed to show how other contracts interact with it and contribute to different aspects of the governance process, such as member elections, activation on different chains, and action record management. Each contract plays a specific role and contributes to the overall operation of the system.
Click on the diagram links:
The central and most critical contracts within the project.
The system architecture comprises various interconnected smart contracts that collaborate to manage and administer the Security Council's governance process. Here is an overview of the components and their roles within the architecture::
Overall, we consider the quality of Arbitrum codebase to be excellent. The code appears to be very mature and well-developed. Details are explained below:
Codebase Quality Categories | Comments |
---|---|
Unit Testing | Codebase is well-tested it was great to see the protocol using Forundry framework. |
Code Comments | Comments in general were solid. However is always room for improvement. Some areas could benefit from greater clarity in comments or explanations. Providing more detailed comments and documentation for complex or critical sections of the code can greatly enhance the codebase's overall readability and maintainability. This would not only help the current development team but also make it easier for future contributors to understand and build upon the existing code. |
Documentation | The documentation for Arbitrum is very good. |
Organization | Codebase is very mature and well organized with clear distinctions between the 20 contracts. |
The analysis provided highlights several significant systemic and centralization risks present in the present Arbitrum contest.
dependency risk:
package.json#L69-L70 69: "@openzeppelin/contracts": "4.7.3", 70: "@openzeppelin/contracts-upgradeable": "4.7.3",
https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts/4.9.3
The following is a list of quirks and/or potentially unexpected behaviors in the Arbitrum Governance implementation. General familiarity with the architecture is assumed:
ProposalExecuted
event will not be emitted.msg.value
value will be forwarded to each one. For the execution to be successful, the msg.value
should be set to m
and the L1ArbitrumTimelock should be prefunded with at least m * n
ETH, where m
is the max out of the costs of each retryable ticket, and n
is the number of retryable tickets created.onlyGovernance
modifier ensures a call is made from the timelock; in L2ArbitrumGoverner, the _executor() method is overriden such that onlyGovernance
enforces a call from the governor contract itself. This ensures calls guarded by onlyGovernance
go through the full core proposal path, as calls from the governor could only be sent via relay
. See the code comment on relay
in L2ArbitrumGoveror for more.CANCELLER_ROLE
affordance. Additionally, the later affordances is granted directly to the security council, not the UpgradeExecutor
(this is inconsistent with how UpgradeExecutors
are generally used elsewhere.)UpgradeExecutor
— has the affordance to cancel proposals in the L1 timelock, inconsistent with how UpgradeExecutors
are generally used elsewhere.Test Coverage: the test coverage provide by Arbitrum is the 94%, however we recomende 100% of the tets coverage.
In general, Arbitrum project exhibits an interesting and well-developed architecture we believe the team has done a good job regarding the code. Additionally, it is recommended to improve the documentation and comments in the code to enhance understanding and collaboration among developers and auditors. It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.
A total of 5 days were spent to cover this audit and fully understand the flow of the contracts.
24 hours
#0 - c4-pre-sort
2023-08-13T19:01:12Z
0xSorryNotSorry marked the issue as high quality report
#1 - c4-sponsor
2023-08-15T14:59:15Z
yahgwai marked the issue as sponsor acknowledged
#2 - c4-judge
2023-08-18T23:47:31Z
0xean marked the issue as grade-a
#3 - c4-judge
2023-08-18T23:49:58Z
0xean marked the issue as selected for report
#4 - catellaTech
2023-08-22T15:44:56Z
Hello, @0xean! reviewing my analysis, I am very satisfied to have been selected for the report. However, I realized that a diagram I created with a lot of effort had its link accidentally overwritten with another content that I am currently working on. This is the new link for the Security Council Diagram contract. Could you please help me ensure that this diagram gets published in Analysis of the main contract/ SecurityCouncilManager:? Thank you.
#5 - 0xean
2023-08-22T15:55:40Z
thanks @catellaTech
cc @C4-Staff - can you help to make sure this gets published appropriately?