Merge pull request from GHSA-5h3x-9wvq-w4m2

Co-authored-by: Francisco <[email protected]>
Co-authored-by: Ernesto García <[email protected]>
(cherry picked from commit d947432)

.changeset/swift-bags-divide.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`Governor`: Add a mechanism to restrict the address of the proposer using a suffix in the description.

contracts/governance/Governor.sol

@@ -263,7 +263,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     }
 
     /**
-     * @dev See {IGovernor-propose}.
+     * @dev See {IGovernor-propose}. This function has opt-in frontrunning protection, described in {_isValidDescriptionForProposer}.
      */
     function propose(
         address[] memory targets,
@@ -272,8 +272,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         string memory description
     ) public virtual override returns (uint256) {
         address proposer = _msgSender();
-        uint256 currentTimepoint = clock();
+        require(_isValidDescriptionForProposer(proposer, description), "Governor: proposer restricted");
 
+        uint256 currentTimepoint = clock();
         require(
             getVotes(proposer, currentTimepoint - 1) >= proposalThreshold(),
             "Governor: proposer votes below proposal threshold"
@@ -634,4 +635,89 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     ) public virtual override returns (bytes4) {
         return this.onERC1155BatchReceived.selector;
     }
+
+    /**
+     * @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);
+            }
+        }
+    }
 }

test/governance/Governor.t.sol

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.19;
+
+import "forge-std/Test.sol";
+import "../../contracts/utils/Strings.sol";
+import "../../contracts/governance/Governor.sol";
+
+contract GovernorInternalTest is Test, Governor {
+    constructor() Governor("") {}
+
+    function testValidDescriptionForProposer(string memory description, address proposer, bool includeProposer) public {
+        if (includeProposer) {
+            description = string.concat(description, "#proposer=", Strings.toHexString(proposer));
+        }
+        assertTrue(_isValidDescriptionForProposer(proposer, description));
+    }
+
+    function testInvalidDescriptionForProposer(
+        string memory description,
+        address commitProposer,
+        address actualProposer
+    ) public {
+        vm.assume(commitProposer != actualProposer);
+        description = string.concat(description, "#proposer=", Strings.toHexString(commitProposer));
+        assertFalse(_isValidDescriptionForProposer(actualProposer, description));
+    }
+
+    // We don't need to truly implement implement the missing functions because we are just testing
+    // internal helpers.
+
+    function clock() public pure override returns (uint48) {}
+
+    // solhint-disable-next-line func-name-mixedcase
+    function CLOCK_MODE() public pure override returns (string memory) {}
+
+    // solhint-disable-next-line func-name-mixedcase
+    function COUNTING_MODE() public pure virtual override returns (string memory) {}
+
+    function votingDelay() public pure virtual override returns (uint256) {}
+
+    function votingPeriod() public pure virtual override returns (uint256) {}
+
+    function quorum(uint256) public pure virtual override returns (uint256) {}
+
+    function hasVoted(uint256, address) public pure virtual override returns (bool) {}
+
+    function _quorumReached(uint256) internal pure virtual override returns (bool) {}
+
+    function _voteSucceeded(uint256) internal pure virtual override returns (bool) {}
+
+    function _getVotes(address, uint256, bytes memory) internal pure virtual override returns (uint256) {}
+
+    function _countVote(uint256, address, uint8, uint256, bytes memory) internal virtual override {}
+}

test/governance/Governor.test.js

@@ -544,6 +544,89 @@ contract('Governor', function (accounts) {
         });
       });
 
+      describe('frontrun protection using description suffix', function () {
+        describe('without protection', function () {
+          describe('without suffix', function () {
+            it('proposer can propose', async function () {
+              expectEvent(await this.helper.propose({ from: proposer }), 'ProposalCreated');
+            });
+
+            it('someone else can propose', async function () {
+              expectEvent(await this.helper.propose({ from: voter1 }), 'ProposalCreated');
+            });
+          });
+
+          describe('with different suffix', function () {
+            beforeEach(async function () {
+              this.proposal = this.helper.setProposal(
+                [
+                  {
+                    target: this.receiver.address,
+                    data: this.receiver.contract.methods.mockFunction().encodeABI(),
+                    value,
+                  },
+                ],
+                `<proposal description>#wrong-suffix=${proposer}`,
+              );
+            });
+
+            it('proposer can propose', async function () {
+              expectEvent(await this.helper.propose({ from: proposer }), 'ProposalCreated');
+            });
+
+            it('someone else can propose', async function () {
+              expectEvent(await this.helper.propose({ from: voter1 }), 'ProposalCreated');
+            });
+          });
+
+          describe('with proposer suffix but bad address part', function () {
+            beforeEach(async function () {
+              this.proposal = this.helper.setProposal(
+                [
+                  {
+                    target: this.receiver.address,
+                    data: this.receiver.contract.methods.mockFunction().encodeABI(),
+                    value,
+                  },
+                ],
+                `<proposal description>#proposer=0x3C44CdDdB6a900fa2b585dd299e03d12FA429XYZ`, // XYZ are not a valid hex char
+              );
+            });
+
+            it('propose can propose', async function () {
+              expectEvent(await this.helper.propose({ from: proposer }), 'ProposalCreated');
+            });
+
+            it('someone else can propose', async function () {
+              expectEvent(await this.helper.propose({ from: voter1 }), 'ProposalCreated');
+            });
+          });
+        });
+
+        describe('with protection via proposer suffix', function () {
+          beforeEach(async function () {
+            this.proposal = this.helper.setProposal(
+              [
+                {
+                  target: this.receiver.address,
+                  data: this.receiver.contract.methods.mockFunction().encodeABI(),
+                  value,
+                },
+              ],
+              `<proposal description>#proposer=${proposer}`,
+            );
+          });
+
+          it('proposer can propose', async function () {
+            expectEvent(await this.helper.propose({ from: proposer }), 'ProposalCreated');
+          });
+
+          it('someone else cannot propose', async function () {
+            await expectRevert(this.helper.propose({ from: voter1 }), 'Governor: proposer restricted');
+          });
+        });
+      });
+
       describe('onlyGovernance updates', function () {
         it('setVotingDelay is protected', async function () {
           await expectRevert(this.mock.setVotingDelay('0'), 'Governor: onlyGovernance');