CVE-2023-34234 (Other, published 2023-06-07T18:15Z)
Description: 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.
Severity: nan (CVSS v3 base score: 5.3)
Weakness: NVD-CWE-noinfo \u2014 Insufficient Information
There is insufficient information about the issue to classify it; details are unkown or unspecified.
Repository: https://github.com/OpenZeppelin/openzeppelin-contracts
Files changed:
.changeset/swift-bags-divide.md
contracts/governance/Governor.sol
test/governance/Governor.t.sol
test/governance/Governor.test.js
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>

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');
          });
        });
      });