NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
Co-authored-by: Francisco <fg@frang.io>
Co-authored-by: Ernesto García <ernestognw@gmail.com>
(cherry picked from commit d9474327a4)
This commit is contained in:
Hadrien Croubois
2023-06-07 02:32:14 +02:00
committed by Francisco Giordano
parent fa3a30a580
commit 33ff9b086d
4 changed files with 231 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/swift-bags-divide.md Normal file
View File

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

90
contracts/governance/Governor.sol
View File

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

55
test/governance/Governor.t.sol Normal file
View File

@ -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 {}
}

83
test/governance/Governor.test.js
View File

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