Proposal cancellation

.gas-snapshot

@@ -1,32 +1,32 @@
-AIP1Point2ActionTest:testAction() (gas: 629328)
+AIP1Point2ActionTest:testAction() (gas: 629408)
 AIPNovaFeeRoutingActionTest:testAction() (gas: 3074)
 ArbitrumDAOConstitutionTest:testConstructor() (gas: 259383)
 ArbitrumDAOConstitutionTest:testMonOwnerCannotSetHash() (gas: 262836)
 ArbitrumDAOConstitutionTest:testOwnerCanSetHash() (gas: 261148)
 ArbitrumDAOConstitutionTest:testOwnerCanSetHashTwice() (gas: 263824)
-ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16332093)
-ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19243747)
-ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19247090)
-ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19247035)
-ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16335426)
-ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16327408)
-ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16329757)
-ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16332176)
-ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16337546)
-ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16329656)
-ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16451131)
+ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16711185)
+ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19622838)
+ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19626181)
+ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19626126)
+ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16714518)
+ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16706500)
+ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16708849)
+ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16711268)
+ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16716637)
+ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16708748)
+ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16830223)
 ArbitrumVestingWalletFactoryTest:testDeploy() (gas: 4589688)
 ArbitrumVestingWalletFactoryTest:testOnlyOwnerCanCreateWallets() (gas: 1504286)
-ArbitrumVestingWalletTest:testCastVote() (gas: 16201584)
-ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16151341)
-ArbitrumVestingWalletTest:testClaim() (gas: 16007768)
-ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 15967955)
-ArbitrumVestingWalletTest:testDelegate() (gas: 16081106)
-ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16008435)
-ArbitrumVestingWalletTest:testDoesDeploy() (gas: 15971342)
-ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16008649)
-ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16074917)
-E2E:testE2E() (gas: 84680100)
+ArbitrumVestingWalletTest:testCastVote() (gas: 16602853)
+ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16552681)
+ArbitrumVestingWalletTest:testClaim() (gas: 16386861)
+ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 16347048)
+ArbitrumVestingWalletTest:testDelegate() (gas: 16460199)
+ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16387528)
+ArbitrumVestingWalletTest:testDoesDeploy() (gas: 16350435)
+ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16387742)
+ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16454010)
+E2E:testE2E() (gas: 84680209)
 FixedDelegateErc20WalletTest:testInit() (gas: 5822575)
 FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 5816805)
 FixedDelegateErc20WalletTest:testTransfer() (gas: 5932218)
@@ -60,13 +60,13 @@ L1GovernanceFactoryTest:testL1GovernanceFactory() (gas: 10771109)
 L1GovernanceFactoryTest:testSetMinDelay() (gas: 10746003)
 L1GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 10798958)
 L2AddressRegistryTest:testAddressRegistryAddress() (gas: 54658)
-L2ArbitrumGovernorTest:testCantReinit() (gas: 13669489)
-L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 13706483)
-L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 13679135)
-L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 13673238)
-L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 13812715)
-L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 13737218)
-L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 13664706)
+L2ArbitrumGovernorTest:testCantReinit() (gas: 14048626)
+L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 14085537)
+L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 14058208)
+L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 14052375)
+L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 14191829)
+L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 14116355)
+L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 14043756)
 L2ArbitrumTokenTest:testCanBurn() (gas: 4066835)
 L2ArbitrumTokenTest:testCanMint2Percent() (gas: 4101512)
 L2ArbitrumTokenTest:testCanMintLessThan2Percent() (gas: 4101514)
@@ -85,23 +85,23 @@ L2ArbitrumTokenTest:testDoesNotInitialiseZeroL1Token() (gas: 3800726)
 L2ArbitrumTokenTest:testDoesNotInitialiseZeroOwner() (gas: 3800739)
 L2ArbitrumTokenTest:testIsInitialised() (gas: 4072777)
 L2ArbitrumTokenTest:testNoLogicContractInit() (gas: 2693127)
-L2GovernanceFactoryTest:testContractsDeployed() (gas: 28359365)
-L2GovernanceFactoryTest:testContractsInitialized() (gas: 28396315)
-L2GovernanceFactoryTest:testDeploySteps() (gas: 28370874)
-L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 28368375)
-L2GovernanceFactoryTest:testRoles() (gas: 28391450)
-L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28415658)
-L2GovernanceFactoryTest:testSetMinDelay() (gas: 28364371)
-L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28417242)
-L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 28657360)
+L2GovernanceFactoryTest:testContractsDeployed() (gas: 29117182)
+L2GovernanceFactoryTest:testContractsInitialized() (gas: 29154197)
+L2GovernanceFactoryTest:testDeploySteps() (gas: 29128691)
+L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 29126192)
+L2GovernanceFactoryTest:testRoles() (gas: 29149267)
+L2GovernanceFactoryTest:testSanityCheckValues() (gas: 29173498)
+L2GovernanceFactoryTest:testSetMinDelay() (gas: 29122188)
+L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 29175059)
+L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 29437526)
 L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 30524001)
 L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 30528232)
 L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 25659644)
 L2SecurityCouncilMgmtFactoryTest:testRemovalGovDeployment() (gas: 30526232)
 L2SecurityCouncilMgmtFactoryTest:testSecurityCouncilManagerDeployment() (gas: 30545325)
 NomineeGovernorV2UpgradeActionTest:testAction() (gas: 8153)
 OfficeHoursActionTest:testConstructor() (gas: 9050)
-OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 258, μ: 317071, ~: 317184)
+OfficeHoursActionTest:testFuzzOfficeHoursDeployment(uint256,uint256,int256,uint256,uint256,uint256) (runs: 256, μ: 317064, ~: 317184)
 OfficeHoursActionTest:testInvalidConstructorParameters() (gas: 235740)
 OfficeHoursActionTest:testPerformBeforeMinimumTimestamp() (gas: 8646)
 OfficeHoursActionTest:testPerformDuringOfficeHours() (gas: 9140)
@@ -153,7 +153,7 @@ SecurityCouncilMemberElectionGovernorTest:testOnlyNomineeElectionGovernorCanProp
 SecurityCouncilMemberElectionGovernorTest:testProperInitialization() (gas: 49388)
 SecurityCouncilMemberElectionGovernorTest:testProposeReverts() (gas: 32916)
 SecurityCouncilMemberElectionGovernorTest:testRelay() (gas: 42229)
-SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 258, μ: 339983, ~: 339822)
+SecurityCouncilMemberElectionGovernorTest:testSelectTopNominees(uint256) (runs: 256, μ: 340112, ~: 339983)
 SecurityCouncilMemberElectionGovernorTest:testSelectTopNomineesFails() (gas: 273335)
 SecurityCouncilMemberElectionGovernorTest:testSetFullWeightDuration() (gas: 34951)
 SecurityCouncilMemberElectionGovernorTest:testVotesToWeight() (gas: 152898)
@@ -205,7 +205,7 @@ SecurityCouncilNomineeElectionGovernorTest:testSetNomineeVetter() (gas: 39905)
 SequencerActionsTest:testAddAndRemoveSequencer() (gas: 483532)
 SequencerActionsTest:testCantAddZeroAddress() (gas: 235614)
 SetInitialGovParamsActionTest:testL1() (gas: 259904)
-SetInitialGovParamsActionTest:testL2() (gas: 688888)
+SetInitialGovParamsActionTest:testL2() (gas: 688702)
 SetSequencerInboxMaxTimeVariationAction:testSetMaxTimeVariation() (gas: 374262)
 SwitchManagerRolesActionTest:testAction() (gas: 6313)
 TokenDistributorTest:testClaim() (gas: 5742744)

src/L2ArbitrumGovernor.sol

@@ -41,6 +41,9 @@ contract L2ArbitrumGovernor is
     ///         Note that Excluded Address is a readable name with no code of PK associated with it, and thus can't vote.
     address public constant EXCLUDE_ADDRESS = address(0xA4b86);
 
+    /// @notice Get the proposer of a proposal
+    mapping(uint256 => address) public proposalProposer;
+
     constructor() {
         _disableInitializers();
     }
@@ -111,6 +114,37 @@ contract L2ArbitrumGovernor is
         AddressUpgradeable.functionCallWithValue(target, data, value);
     }
 
+    /** 
+     * @notice Cancel a proposal. Can only be called by the proposer.
+     */
+    function cancel(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public virtual returns (uint256 proposalId) {
+        proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+        require(state(proposalId) == ProposalState.Pending, "L2ArbitrumGovernor: too late to cancel");
+        require(_msgSender() == proposalProposer[proposalId], "L2ArbitrumGovernor: not proposer");
+        _cancel(targets, values, calldatas, descriptionHash);
+    }
+
+    /**
+     * @dev See {IGovernor-propose}. This function has opt-in frontrunning protection, described in {_isValidDescriptionForProposer}.
+     */
+    function propose(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        string memory description
+    ) public virtual override(IGovernorUpgradeable, GovernorUpgradeable) returns (uint256) {
+        address _proposer = _msgSender();
+        require(_isValidDescriptionForProposer(_proposer, description), "L2ArbitrumGovernor: proposer restricted");
+        uint256 proposalId = GovernorUpgradeable.propose(targets, values, calldatas, description);
+        proposalProposer[proposalId] = _proposer;
+        return proposalId;
+    }
+
     /// @notice returns l2 executor address; used internally for onlyGovernance check
     function _executor()
         internal
@@ -227,13 +261,107 @@ contract L2ArbitrumGovernor is
         override(GovernorUpgradeable, GovernorTimelockControlUpgradeable)
         returns (bool)
     {
-        return GovernorTimelockControlUpgradeable.supportsInterface(interfaceId);
+        bytes4 governorCancelId = this.cancel.selector ^ this.proposalProposer.selector;
+        return interfaceId == governorCancelId || GovernorTimelockControlUpgradeable.supportsInterface(interfaceId);
+    }
+
+    /**
+     * @dev This project uses OZ v4.7.3, which has a vulnerability in proposal cancellation that was patched in v4.9.1.
+     *
+     * For details on the vulnerability, see here: 
+     * https://github.com/OpenZeppelin/openzeppelin-contracts/security/advisories/GHSA-5h3x-9wvq-w4m2
+     *
+     * _isValidDescriptionForProposer and its usage in propose is a backport of the fix in OZ v4.9.1, which can be found here:
+     * https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/commit/66f390fa516b550838e2c2f65132b5bc2afe1ced
+     * Check if the proposer is authorized to submit a proposal with the given description.
+     *
+     * 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.
+     *
+     * This is used for frontrunning protection. By adding this pattern at the end of their proposal, one can ensure
+     * that no other address can submit the same proposal. An attacker would have to either remove or change that part,
+     * which would result in a different proposal id.
+     *
+     * If the description does not match this pattern, it is unrestricted and anyone can submit it. This includes:
+     * - If the `0x???` part is not a valid hex string.
+     * - If the `0x???` part is a valid hex string, but does not contain exactly 40 hex digits.
+     * - If it ends with the expected suffix followed by newlines or other whitespace.
+     * - If it ends with some other similar suffix, e.g. `#other=abc`.
+     * - If it does not end with any such suffix.
+     */
+    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)))
+        }
+
+        // If the marker is not found, there is no proposer suffix to check
+        if (marker != bytes12("#proposer=0x")) {
+            return true;
+        }
+
+        // Parse the 40 characters following the marker as uint160
+        uint160 recovered = 0;
+        for (uint256 i = len - 40; i < len; ++i) {
+            (bool isHex, uint8 value) = _tryHexToUint(bytes(description)[i]);
+            // If any of the characters is not a hex digit, ignore the suffix entirely
+            if (!isHex) {
+                return true;
+            }
+            recovered = (recovered << 4) | value;
+        }
+
+        return recovered == uint160(proposer_);
+    }
+
+    /**
+     * from https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/commit/66f390fa516b550838e2c2f65132b5bc2afe1ced
+     * @dev Try to parse a character from a string as a hex value. Returns `(true, value)` if the char is in
+     * `[0-9a-fA-F]` and `(false, 0)` otherwise. Value is guaranteed to be in the range `0 <= value < 16`
+     */
+    function _tryHexToUint(bytes1 char) private pure returns (bool, uint8) {
+        uint8 c = uint8(char);
+        unchecked {
+            // Case 0-9
+            if (47 < c && c < 58) {
+                return (true, c - 48);
+            }
+            // Case A-F
+            else if (64 < c && c < 71) {
+                return (true, c - 55);
+            }
+            // Case a-f
+            else if (96 < c && c < 103) {
+                return (true, c - 87);
+            }
+            // Else: not a hex char
+            else {
+                return (false, 0);
+            }
+        }
     }
 
     /**
      * @dev This empty reserved space is put in place to allow future versions to add new
      * variables without shifting down storage in the inheritance chain.
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
-    uint256[50] private __gap;
+    uint256[49] private __gap;
 }

src/interfaces/IL2ArbitrumGovernor.sol

@@ -16,4 +16,11 @@ interface IL2ArbitrumGoverner {
     function owner() external view returns (address);
     function proposalThreshold() external view returns (uint256);
     function setProposalThreshold(uint256 newProposalThreshold) external;
+    function proposalProposer(uint256 proposalId) external view returns (address);
+    function cancel(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) external;
 }

test/signatures/L2ArbitrumGovernor

@@ -3,6 +3,7 @@
   "COUNTING_MODE()": "dd4e2ba5",
   "EXCLUDE_ADDRESS()": "5e12ebbd",
   "EXTENDED_BALLOT_TYPEHASH()": "2fe3e261",
+  "cancel(address[],uint256[],bytes[],bytes32)": "452115d6",
   "castVote(uint256,uint8)": "56781388",
   "castVoteBySig(uint256,uint8,uint8,bytes32,bytes32)": "3bccf4fd",
   "castVoteWithReason(uint256,uint8,string)": "7b3c71d3",
@@ -23,6 +24,7 @@
   "owner()": "8da5cb5b",
   "proposalDeadline(uint256)": "c01f9e37",
   "proposalEta(uint256)": "ab58fb8e",
+  "proposalProposer(uint256)": "143489d0",
   "proposalSnapshot(uint256)": "2d63f693",
   "proposalThreshold()": "b58131b0",
   "proposalVotes(uint256)": "544ffc9c",

test/storage/L2ArbitrumGovernor

@@ -32,4 +32,5 @@
 | __gap                   | uint256[48]                                                               | 556  | 0      | 1536  | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
 | _owner                  | address                                                                   | 604  | 0      | 20    | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
 | __gap                   | uint256[49]                                                               | 605  | 0      | 1568  | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
-| __gap                   | uint256[50]                                                               | 654  | 0      | 1600  | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
+| proposalProposer        | mapping(uint256 => address)                                               | 654  | 0      | 32    | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
+| __gap                   | uint256[49]                                                               | 655  | 0      | 1568  | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |