diff --git a/.changeset/swift-bags-divide.md b/.changeset/swift-bags-divide.md new file mode 100644 index 00000000000..9af63e98e3c --- /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 390d2b94a3b..9dda05e0b0b 100644 --- a/contracts/governance/Governor.sol +++ b/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); + } + } + } } diff --git a/test/governance/Governor.t.sol b/test/governance/Governor.t.sol new file mode 100644 index 00000000000..43c4c5ddd9e --- /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 f867958b582..53f63ab7b01 100644 --- a/test/governance/Governor.test.js +++ b/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, + }, + ], + `#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, + }, + ], + `#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, + }, + ], + `#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');