Arbitrum Security Council Election System - AkshaySrivastav's results

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

General Information

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

Arbitrum Foundation

Findings Distribution

Researcher Performance

Rank: 6/36

Findings: 1

Award: $4,429.19

🌟 Selected for report: 1

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Arbitrum Foundation

Findings Information

🌟 Selected for report: AkshaySrivastav

Also found by: MiloTruck

Labels

bug

2 (Med Risk)

low quality report

primary issue

satisfactory

selected for report

M-03

Awards

4429.1881 USDC - $4,429.19

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Impact

The SecurityCouncilMemberRemovalGovernor contract inherits GovernorUpgradeable & EIP712Upgradeable contracts but does not invoke their individual initialzers during its own initialization. Due to which the state of GovernorUpgradeable & EIP712Upgradeable contracts remain uninitialized.

contract SecurityCouncilMemberRemovalGovernor is
    GovernorUpgradeable,
    GovernorVotesUpgradeable,
    ...
{
    function initialize(
        ...
    ) public initializer {
        __GovernorSettings_init(_votingDelay, _votingPeriod, _proposalThreshold);
        __GovernorCountingSimple_init();
        __GovernorVotes_init(_token);
        __ArbitrumGovernorVotesQuorumFraction_init(_quorumNumerator);
        __GovernorPreventLateQuorum_init(_minPeriodAfterQuorum);
        __ArbitrumGovernorProposalExpirationUpgradeable_init(_proposalExpirationBlocks);
        _transferOwnership(_owner);
        ...
    }
}

abstract contract GovernorUpgradeable is EIP712Upgradeable, ... {
    function __Governor_init(string memory name_) internal onlyInitializing {
        __EIP712_init_unchained(name_, version());
        __Governor_init_unchained(name_);
    }
    function __Governor_init_unchained(string memory name_) internal onlyInitializing {
        _name = name_;
    }
}

Due to this issue:

In GovernorUpgradeable the _name storage variable is never initialized and the name() function returns an empty string.
In EIP712Upgradeable the _HASHED_NAME storage variable is never initialized. This variable is used in the castVoteBySig & castVoteWithReasonAndParamsBySig functions of SecurityCouncilMemberRemovalGovernor.

The non-initialization of EIP712Upgradeable contract breaks the compatibility of SecurityCouncilMemberRemovalGovernor contract with the EIP712 standard.

It should be noted that the other contracts like SecurityCouncilMemberElectionGovernor and SecurityCouncilNomineeElectionGovernor also inherits the GovernorUpgradeable & EIP712Upgradeable contracts and initializes them correctly. So the incorrect initialization of SecurityCouncilMemberRemovalGovernor also breaks the overall code consistency of protocol.

The SecurityCouncilMemberElectionGovernor and SecurityCouncilNomineeElectionGovernor contract also force users to do voting only via castVoteWithReasonAndParams and castVoteWithReasonAndParamsBySig functions. Hence it can be clearly observed that the protocol wants to utilize voting-by-signature feature, which requires EIP712 compatibility.

Proof of Concept

This test case was added in test/security-council-mgmt/SecurityCouncilMemberRemovalGovernor.t.sol and ran using forge test --mt test_audit.

    function test_audit_notInvoking___Governor_init() public {
        assertEq(scRemovalGov.name(), "");
        assertEq(bytes(scRemovalGov.name()).length, 0);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Consider initializing the GovernorUpgradeable & EIP712Upgradeable contracts in SecurityCouncilMemberRemovalGovernor.initialize function.

    function initialize(
        ...
    ) public initializer {
        __Governor_init("SecurityCouncilMemberRemovalGovernor");
        ...
    }

Assessed type

Other

#0 - 0xSorryNotSorry
2023-08-12T11:12:19Z

OOS --> [L‑03] Upgradeable contract not initialized

#1 - c4-pre-sort
2023-08-12T11:12:22Z

0xSorryNotSorry marked the issue as low quality report

#2 - c4-judge
2023-08-18T13:43:21Z

0xean marked the issue as unsatisfactory: Out of scope

#3 - akshaysrivastav
2023-08-22T04:33:59Z

Hey @0xean I would like to add more context for this report.

I was aware that this issue has been included in the automated findings but still reported this bug due to following reasons:

The automated report does not explains the full impact of the finding and hence incorrectly judges it as low.
Almost all cases mentioned in Upgradeable contract not initialized section are false positives.

False positives reported in automated report:

SecurityCouncilManager does not need to invoke __AccessControl_init as __AccessControl_init is an empty function.
SecurityCouncilMemberElectionGovernor does not need to invoke __Ownable_init because the contract wants to provide ownership to a input parameter _owner while the __Ownable_init gives ownership to msg.sender. To achieve that the SecurityCouncilMemberElectionGovernor invokes the _transferOwnership function with _owner parameter in the initializer.
SecurityCouncilMemberRemovalGovernor:
- invokes __ArbitrumGovernorProposalExpiration_init correctly.
- handles the __Ownable_init case correctly similar to above explained SecurityCouncilMemberElectionGovernor case.
- does not invoke __Governor_init, this is the issue reported by me.
SecurityCouncilNomineeElectionGovernor handles the __Ownable_init case correctly similar to above explained SecurityCouncilMemberElectionGovernor case.

Due to the above explained reasons I separately reported this bug as medium severity. The bug leads to incompatibility of a protocol contract with EIP712 standard which the contract always wanted to adhere to.

Hope this report can be reviewed again. Thanks.

#4 - MiloTruck
2023-08-22T08:39:45Z

Hi @0xean, would like to add on to what was stated above as my issue #253 is a duplicate.

The C4 docs explicitly states that raising issues from bot reports to a higher severity is fair game, as seen here:

Wardens may use automated tools as a first pass, and build on these findings to identify High and Medium severity issues ("HM issues"). However, submissions based on automated tools will have a higher burden of proof for demonstrating to sponsors a relevant HM exploit path in order to be considered satisfactory.

In my issue, I've demonstrated how the missing __Governor_init() call results in incorrect signature verification in castVoteBySig() and castVoteWithReasonAndParamsBySig() and violates the EIP-712 standard, which in my opinion is deserving of medium severity.

Additionally, as pointed out above, it would have been extremely easy for a sponsor to miss this medium finding amongst all the other false positives in the bot report.

As such, I don't think this finding should be ruled out as OOS as we've demonstrated how it has a greater severity than stated in the bot report and provided additional value to the sponsor.

Would like to hear your opinion as a judge, thanks!

#5 - 0xean
2023-08-22T12:37:50Z

Thanks for the comments. After reviewing, I think raising this to M and awarding this is fair. The bot report is vague on impact and without this being called to the sponsors attention explicitly with a much broader impact, it could go unfixed until a later date.

Will welcome sponsor comments before making a final call ( cc @c4-sponsor )

#6 - c4-judge
2023-08-22T12:38:04Z

0xean marked the issue as satisfactory

#7 - c4-judge
2023-08-22T12:38:10Z

0xean marked the issue as primary issue

#8 - yahgwai
2023-08-22T16:24:37Z

Issue was well written and had correct mitigation I think M seems fair, ty for finding it and pointing it out!

Fix here: https://github.com/ArbitrumFoundation/governance/commit/1a6b66d23f1226e93f0d67c3ed8e9fe1174c178c

#9 - c4-judge
2023-08-23T13:37:06Z

0xean marked the issue as selected for report

Arbitrum Security Council Election System - AkshaySrivastav's results

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

General Information

Findings Distribution

Researcher Performance

External Links

🌟 [M-03] Incorrect initialization of `SecurityCouncilMemberRemovalGovernor` contract - $4,429.19

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - 0xSorryNotSorry2023-08-12T11:12:19Z

#1 - c4-pre-sort2023-08-12T11:12:22Z

#2 - c4-judge2023-08-18T13:43:21Z

#3 - akshaysrivastav2023-08-22T04:33:59Z

#4 - MiloTruck2023-08-22T08:39:45Z

#5 - 0xean2023-08-22T12:37:50Z

#6 - c4-judge2023-08-22T12:38:04Z

#7 - c4-judge2023-08-22T12:38:10Z

#8 - yahgwai2023-08-22T16:24:37Z

#9 - c4-judge2023-08-23T13:37:06Z

#0 - 0xSorryNotSorry
2023-08-12T11:12:19Z

#1 - c4-pre-sort
2023-08-12T11:12:22Z

#2 - c4-judge
2023-08-18T13:43:21Z

#3 - akshaysrivastav
2023-08-22T04:33:59Z

#4 - MiloTruck
2023-08-22T08:39:45Z

#5 - 0xean
2023-08-22T12:37:50Z

#6 - c4-judge
2023-08-22T12:38:04Z

#7 - c4-judge
2023-08-22T12:38:10Z

#8 - yahgwai
2023-08-22T16:24:37Z

#9 - c4-judge
2023-08-23T13:37:06Z