CVE-2023-34234: Insufficient Information (NVD-CWE-noinfo)
value:  OpenZeppelin Contracts is a library for smart contract development. By frontrunning the creation of a proposal, an attacker can become the proposer and gain the ability to cancel it. The attacker can do this repeatedly to try to prevent a proposal from being proposed at all. This impacts the `Governor` contract in v4.9.0 only, and the `GovernorCompatibilityBravo` contract since v4.3.0. This problem has been patched in 4.9.1 by introducing opt-in frontrunning protection. Users are advised to upgrade. Users unable to upgrade may submit the proposal creation transaction to an endpoint with frontrunning protection as a workaround.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 5.3
Published: 2023-06-07T18:15Z
Repository: https://github.com/OpenZeppelin/openzeppelin-contracts
Language: Other
Files changed: .changeset/swift-bags-divide.md, contracts/governance/Governor.sol, test/governance/Governor.t.sol, test/governance/Governor.test.js
Commit date: 2023-06-06 21:32:14 -0300
Commit message: Merge pull request from GHSA-5h3x-9wvq-w4m2

Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>
Diff: diff --git a/.changeset/swift-bags-divide.md b/.changeset/swift-bags-divide.md
new file mode 100644
index 000000000..9af63e98e
--- /dev/null
+++ b/.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.
diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol
index 74d60c742..2e2289c6a 100644
--- a/contracts/governance/Governor.sol
+++ b/contracts/governance/Governor.sol
@@ -261,21 +261,22 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
     function _defaultParams() internal view virtual returns (bytes memory) {
         return "";
     }
 
     /**
-     * @dev See {IGovernor-propose}.
+     * @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 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"
         );
 
@@ -626,6 +627,91 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         uint256[] memory,
         bytes memory
     ) public virtual 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);
+            }
+        }
+    }
 }
diff --git a/test/governance/Governor.t.sol b/test/governance/Governor.t.sol
new file mode 100644
index 000000000..43c4c5ddd
--- /dev/null
+++ b/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 {}
+}
diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js
index d34482212..96feaf3a7 100644
--- a/test/governance/Governor.test.js
+++ b/test/governance/Governor.test.js
@@ -549,10 +549,93 @@ contract('Governor', function (accounts) {
           );
           await expectRevert(this.helper.propose(), 'Governor: invalid proposal length');
         });
       });
 
+      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');
         });
 

Vulnerable code:      * @dev See {IGovernor-propose}.
        uint256 currentTimepoint = clock();
Fixed code: ---
'openzeppelin-solidity': patch
---

`Governor`: Add a mechanism to restrict the address of the proposer using a suffix in the description.
     * @dev See {IGovernor-propose}. This function has opt-in frontrunning protection, described in {_isValidDescriptionForProposer}.
        require(_isValidDescriptionForProposer(proposer, description), "Governor: proposer restricted");
        uint256 currentTimepoint = clock();

    /**
     * @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);
            }
        }
    }
// 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 {}
}
      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');
          });
        });
      });