wip: DVP and cancel function

audit-ci.jsonc

@@ -47,6 +47,20 @@
         // js-yaml has prototype pollution in merge (<<)
         "GHSA-mh29-5h37-fv8m",
         // body-parser is vulnerable to denial of service when url encoding is used
-        "GHSA-wqch-xfxh-vrr4"
+        "GHSA-wqch-xfxh-vrr4",
+        // qs's arrayLimit bypass in its bracket notation allows DoS via memory exhaustion
+        "GHSA-6rw7-vpxm-498p",
+        // node-tar is Vulnerable to Arbitrary File Overwrite and Symlink Poisoning via Insufficient Path Sanitization
+        "GHSA-8qq5-rm4j-mr97",
+        // Race Condition in node-tar Path Reservations via Unicode Ligature Collisions on macOS APFS
+        "GHSA-r6q2-hw4h-h46w",
+        // jsdiff has a Denial of Service vulnerability in parsePatch and applyPatch
+        "GHSA-73rr-hh4g-fpgx",
+        // Elliptic Uses a Cryptographic Primitive with a Risky Implementation
+        "GHSA-848j-6mx2-7j84",
+        // Undici has an unbounded decompression chain in HTTP responses on Node.js Fetch API via Content-Encoding leads to resource exhaustion
+        "GHSA-g9mf-h72j-4rw9",
+        // Lodash has Prototype Pollution Vulnerability in `_.unset` and `_.omit` functions
+        "GHSA-xxjr-mmjv-4gpg"
     ]
   }

scripts/proposals/ActivateDvpQuorum/data.json

@@ -0,0 +1,12 @@
+{
+  "actionChainIds": [
+    42161
+  ],
+  "actionAddresses": [
+    "0x19C8Ea5F8288abF138D72a13344E699a7A71400c"
+  ],
+  "arbSysSendTxToL1Args": {
+    "l1Timelock": "0xE6841D92B0C345144506576eC13ECf5103aC7f49",
+    "calldata": "0x8f2a0bb000000000000000000000000000000000000000000000000000000000000000c0000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001400000000000000000000000000000000000000000000000000000000000000000b7c5875f8d6dac043d4278efb5a58e9834260314ac17df813eaf54545f5a149f000000000000000000000000000000000000000000000000000000000003f4800000000000000000000000000000000000000000000000000000000000000001000000000000000000000000a723c008e76e379c55599d2e4d93879beafda79c000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000001800000000000000000000000004dbd4fc535ac27206064b68ffcf827b0a60bab3f000000000000000000000000cf57572261c7c2bcf21ffd220ea7d1a27d40a82700000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000841cff79cd00000000000000000000000019c8ea5f8288abf138d72a13344e699a7a71400c00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000004b147f40c0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
+  }
+}

src/L2ArbitrumGovernor.sol

@@ -14,6 +14,7 @@ import
     "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorTimelockControlUpgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import {L2ArbitrumToken} from "./L2ArbitrumToken.sol";
 
 /// @title  L2ArbitrumGovernor
 /// @notice Governance controls for the Arbitrum DAO
@@ -41,6 +42,19 @@ 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 Maximum quorum allowed for a proposal
+    /// @dev    Since the setting is not checkpointed, it is possible that an existing proposal
+    ///         with quorum greater than the maximum can have its quorum suddenly jump to equal maximumQuorum
+    uint256 public maximumQuorum;
+    /// @notice Minimum quorum allowed for a proposal
+    /// @dev    Since the setting is not checkpointed, it is possible that an existing proposal
+    ///         with quorum lesser than the minimum can have its quorum suddenly jump to equal minimumQuorum
+    uint256 public minimumQuorum;
+
+    /// @notice Mapping from proposal ID to the address of the proposer.
+    /// @dev    Used in cancel() to ensure only the proposer can cancel the proposal.
+    mapping(uint256 => address) internal proposers;
+
     constructor() {
         _disableInitializers();
     }
@@ -111,6 +125,49 @@ contract L2ArbitrumGovernor is
         AddressUpgradeable.functionCallWithValue(target, data, value);
     }
 
+    /// @inheritdoc IGovernorUpgradeable
+    /// @dev See {IGovernorUpgradeable-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) {
+        require(
+            _isValidDescriptionForProposer(msg.sender, description),
+            "L2ArbitrumGovernor: PROPOSER_RESTRICTED"
+        );
+        uint256 _proposalId = GovernorUpgradeable.propose(targets, values, calldatas, description);
+        proposers[_proposalId] = msg.sender;
+        return _proposalId;
+    }
+
+    /// @notice Allows a proposer to cancel a proposal when it is pending.
+    /// @param targets The proposal's targets.
+    /// @param values The proposal's values.
+    /// @param calldatas The proposal's calldatas.
+    /// @param descriptionHash The hash of the proposal's description.
+    /// @return The id of the proposal.
+    function cancel(
+        address[] memory targets,
+        uint256[] memory values,
+        bytes[] memory calldatas,
+        bytes32 descriptionHash
+    ) public virtual returns (uint256) {
+        uint256 _proposalId = hashProposal(targets, values, calldatas, descriptionHash);
+
+        require(
+            state(_proposalId) == ProposalState.Pending, "L2ArbitrumGovernor: PROPOSAL_NOT_PENDING"
+        );
+
+        address _proposer = proposers[_proposalId];
+        require(msg.sender == _proposer, "L2ArbitrumGovernor: NOT_PROPOSER");
+
+        delete proposers[_proposalId];
+
+        return GovernorUpgradeable._cancel(targets, values, calldatas, descriptionHash);
+    }
+
     /// @notice returns l2 executor address; used internally for onlyGovernance check
     function _executor()
         internal
@@ -121,21 +178,74 @@ contract L2ArbitrumGovernor is
         return address(this);
     }
 
+    /// @notice Set the quorum minimum and maximum
+    /// @dev    Since the setting is not checkpointed, it is possible that an existing proposal
+    ///         with quorum outside the new min/max can have its quorum suddenly jump to equal
+    ///         the new min or max
+    function setQuorumMinAndMax(uint256 _minimumQuorum, uint256 _maximumQuorum)
+        external
+        onlyGovernance
+    {
+        require(_minimumQuorum < _maximumQuorum, "L2ArbitrumGovernor: MIN_GT_MAX");
+        minimumQuorum = _minimumQuorum;
+        maximumQuorum = _maximumQuorum;
+    }
+
     /// @notice Get "circulating" votes supply; i.e., total minus excluded vote exclude address.
     function getPastCirculatingSupply(uint256 blockNumber) public view virtual returns (uint256) {
         return
             token.getPastTotalSupply(blockNumber) - token.getPastVotes(EXCLUDE_ADDRESS, blockNumber);
     }
 
+    /// @notice Get total delegated votes minus excluded votes
+    /// @dev    If the block number is prior to the first total delegation checkpoint, returns 0
+    ///         Can also return 0 if excluded > total delegation, which is extremely unlikely but possible
+    ///         since L2ArbitrumToken.getTotalDelegationAt is initially an estimate
+    function getPastTotalDelegatedVotes(uint256 blockNumber) public view returns (uint256) {
+        uint256 totalDvp = L2ArbitrumToken(address(token)).getTotalDelegationAt(blockNumber);
+
+        // getTotalDelegationAt may return 0 if the requested block is before the first checkpoint
+        if (totalDvp == 0) {
+            return 0;
+        }
+
+        uint256 excluded = token.getPastVotes(EXCLUDE_ADDRESS, blockNumber);
+
+        // it is possible (but unlikely) that excluded > totalDvp
+        // this is because getTotalDelegationAt is initially an _estimate_ of the total delegation
+        return totalDvp > excluded ? totalDvp - excluded : 0;
+    }
+
     /// @notice Calculates the quorum size, excludes token delegated to the exclude address
+    /// @dev    The calculated quorum is clamped between minimumQuorum and maximumQuorum
     function quorum(uint256 blockNumber)
         public
         view
         override(IGovernorUpgradeable, GovernorVotesQuorumFractionUpgradeable)
         returns (uint256)
     {
-        return (getPastCirculatingSupply(blockNumber) * quorumNumerator(blockNumber))
-            / quorumDenominator();
+        uint256 pastTotalDelegatedVotes = getPastTotalDelegatedVotes(blockNumber);
+
+        // if pastTotalDelegatedVotes is 0, then blockNumber is almost certainly prior to the first totalDelegatedVotes checkpoint
+        // in this case we should use getPastCirculatingSupply to ensure quorum of pre-existing proposals is unchanged
+        // in the unlikely event that totalDvp is 0 for a block _after_ the dvp update, getPastCirculatingSupply will be used with a larger quorumNumerator, 
+        // resulting in a much higher calculated quorum. This is okay because quorum is clamped.
+        uint256 calculatedQuorum = (
+            (
+                pastTotalDelegatedVotes == 0
+                    ? getPastCirculatingSupply(blockNumber)
+                    : pastTotalDelegatedVotes
+            ) * quorumNumerator(blockNumber)
+        ) / quorumDenominator();
+
+        // clamp the calculated quorum between minimumQuorum and maximumQuorum
+        if (calculatedQuorum < minimumQuorum) {
+            return minimumQuorum;
+        } else if (calculatedQuorum > maximumQuorum) {
+            return maximumQuorum;
+        } else {
+            return calculatedQuorum;
+        }
     }
 
     /// @inheritdoc GovernorVotesQuorumFractionUpgradeable
@@ -230,10 +340,97 @@ contract L2ArbitrumGovernor is
         return GovernorTimelockControlUpgradeable.supportsInterface(interfaceId);
     }
 
+    /**
+     * @dev 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);
+    }
+
+    /**
+     * @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[47] private __gap;
 }