Proposals will not be submitted in `SecurityCouncilMemberElectionGovernor.sol` and `SecurityCouncilNomineeElectionGovernor.sol`

[code423n4]

Lines of code

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

Vulnerability details

Impact

SecurityCouncilNomineeElectionGovernor::_execute and SecurityCouncilNomineeElectionGovernor::createElection will always revert, since the form of description at the proposal is not compliant to the format required at GovernorUpgradable of Openzeppelin.

Proof of Concept

When we see the below line at the GovernorUpgradable contract at Openzeppelin, it calls _isValidDescriptionForProposer to check the validity of given description.

 function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description
) public virtual override returns (uint256) {
    address proposer = _msgSender();
 // check description restriction
    if (!_isValidDescriptionForProposer(proposer, description)) {
        revert GovernorRestrictedProposer(proposer);
    }
    ...

The _isValidDescriptionForProposer is checking if the length of description to be longer than 52, or it returns true and make propose function revert.

     function _isValidDescriptionForProposer(
        address proposer,
        string memory description
    ) internal view virtual returns (bool) {
        uint256 len = bytes(description).length;

        // Length is too short to contain a valid proposer suffix
        if (len < 52) {
            return true;
        }

        // Extract what would be the `#proposer=0x` marker beginning the suffix
        bytes12 marker;
        assembly {
            // - Start of the string contents in memory = description + 32
            // - First character of the marker = len - 52
            //   - Length of "#proposer=0x0000000000000000000000000000000000000000" = 52
            // - We read the memory word starting at the first character of the marker:
            //   - (description + 32) + (len - 52) = description + (len - 20)
            // - Note: Solidity will ignore anything past the first 12 bytes
            marker := mload(add(description, sub(len, 20)))
        }

However, the way SecurityCouncilNomineeElectionGovernor::_execute and SecurityCouncilNomineeElectionGovernor::createElection compose description is decided in ElectionGovernor::getProposeArgs, and it returns description as "Security Council Election #XX" format. This will revert in propose function, since its length is under 52 unless electionIndex is large enough. Refer to the below code.

string.concat("Security Council Election #", StringsUpgradeable.toString(electionIndex));

Tools Used

By hand

Recommended Mitigation Steps

Change the proposal description format at ElectionGovernor::electionIndexToDescription like below:

diff --git a/src/security-council-mgmt/governors/modules/ElectionGovernor.sol b/src/security-council-mgmt/governors/modules/ElectionGovernor.sol
index 59c3edd..9bbc88b 100644
--- a/src/security-council-mgmt/governors/modules/ElectionGovernor.sol
+++ b/src/security-council-mgmt/governors/modules/ElectionGovernor.sol
@@ -43,7 +43,7 @@ contract ElectionGovernor {
         returns (string memory)
     {
         return
-            string.concat("Security Council Election #", StringsUpgradeable.toString(electionIndex));
+            string.concat("proposer=",StringsUpgradeable.toHexString(address(this)),"Security Council Election #", StringsUpgradeable.toString(electionIndex));
     }

     /// @notice Returns the cohort for a given `electionIndex`

Assessed type

Other

[DZGoldman]

The _isValidDescriptionForProposer is checking if the length of description to be longer than 52, or it returns true and make propose function revert.

returning true doesn't make the propose function revert, returning false does; invalid submission.

See the _isValidDescriptionForProposer natspec:

 * If the proposal description ends with `#proposer=0x???`, where `0x???` is an address written as a hex string
 * (case insensitive), then the submission of this proposal will only be authorized to said address.
 ...
 * If the description does not match this pattern, it is unrestricted and anyone can submit it.

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

DZGoldman marked the issue as sponsor disputed

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid