From ecfc51512ffc082e24f6d3010af5a3f57e52d29b Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Sun, 23 Jun 2024 19:27:44 -0300 Subject: [PATCH 1/8] refactor: set hash proposal visibility to view --- contracts/governance/Governor.sol | 2 +- contracts/governance/IGovernor.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index be2cf79d263..72c73b017f4 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -127,7 +127,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) public pure virtual returns (uint256) { + ) public view virtual returns (uint256) { return uint256(keccak256(abi.encode(targets, values, calldatas, descriptionHash))); } diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index a35aa18aba2..7bc877272b0 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -207,7 +207,7 @@ interface IGovernor is IERC165, IERC6372 { uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) external pure returns (uint256); + ) external view returns (uint256); /** * @notice module:core From 083cab3a5e6cd47427813a249a85fb6dc6bc55f5 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Sun, 23 Jun 2024 19:47:24 -0300 Subject: [PATCH 2/8] chore: update hash proposal natspec --- contracts/governance/Governor.sol | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 72c73b017f4..2d76847b24f 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -117,10 +117,8 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * can be produced from the proposal data which is part of the {ProposalCreated} event. It can even be computed in * advance, before the proposal is submitted. * - * Note that the chainId and the governor address are not part of the proposal id computation. Consequently, the - * same proposal (with same operation and same description) will have the same id if submitted on multiple governors - * across multiple networks. This also means that in order to execute the same operation twice (on the same - * governor) the proposer will have to change the description in order to avoid proposal id conflicts. + * Note that the chainId and the governor address are not part of the proposal id computation, but can be added to + * guarantee more uniqueness if the same proposal is to submitted on multiple governors across multiple networks. */ function hashProposal( address[] memory targets, From e4364e6d38eae93de375cbde929ceeecf100ef8a Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Sun, 23 Jun 2024 19:47:38 -0300 Subject: [PATCH 3/8] feat: add changeset --- .changeset/popular-horses-tell.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/popular-horses-tell.md diff --git a/.changeset/popular-horses-tell.md b/.changeset/popular-horses-tell.md new file mode 100644 index 00000000000..e2b34062f29 --- /dev/null +++ b/.changeset/popular-horses-tell.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +- update `hashProposal()` visibility to view instead of pure to allow `address(this)` and `block.chainid` to be part of the proposal id computation if more uniqueness between different chains is desired From 3b35cab06e4cc07bdb8263cad2d5edd4636511de Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Sun, 23 Jun 2024 20:03:12 -0300 Subject: [PATCH 4/8] chore: update changeset --- .changeset/popular-horses-tell.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/popular-horses-tell.md b/.changeset/popular-horses-tell.md index e2b34062f29..0cf64d03fe7 100644 --- a/.changeset/popular-horses-tell.md +++ b/.changeset/popular-horses-tell.md @@ -2,4 +2,4 @@ 'openzeppelin-solidity': minor --- -- update `hashProposal()` visibility to view instead of pure to allow `address(this)` and `block.chainid` to be part of the proposal id computation if more uniqueness between different chains is desired +- Update `hashProposal()` visibility to view instead of pure to allow `address(this)` and `block.chainid` to be part of the `proposalId` computation if more uniqueness between different chains is desired From f4b5ba27dfbc94a0f6ab8a99daf6949efb3ffda7 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 26 Jun 2024 18:44:50 -0300 Subject: [PATCH 5/8] chore: update changeset --- .changeset/popular-horses-tell.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.changeset/popular-horses-tell.md b/.changeset/popular-horses-tell.md index 0cf64d03fe7..355fed0371b 100644 --- a/.changeset/popular-horses-tell.md +++ b/.changeset/popular-horses-tell.md @@ -2,4 +2,6 @@ 'openzeppelin-solidity': minor --- -- Update `hashProposal()` visibility to view instead of pure to allow `address(this)` and `block.chainid` to be part of the `proposalId` computation if more uniqueness between different chains is desired +- `Governor`: Change `hashProposal()` visibility to view to allow `address(this)` and `block.chainid` to be part of the `proposalId` computation if more uniqueness between different chains is desired + +--- From 20edfeddde1887b58ea3f25341398a5b6129e179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 1 Nov 2024 16:03:48 +0700 Subject: [PATCH 6/8] Update .changeset/popular-horses-tell.md --- .changeset/popular-horses-tell.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/popular-horses-tell.md b/.changeset/popular-horses-tell.md index 355fed0371b..1b074de776a 100644 --- a/.changeset/popular-horses-tell.md +++ b/.changeset/popular-horses-tell.md @@ -2,6 +2,6 @@ 'openzeppelin-solidity': minor --- -- `Governor`: Change `hashProposal()` visibility to view to allow `address(this)` and `block.chainid` to be part of the `proposalId` computation if more uniqueness between different chains is desired +`Governor`: Change `hashProposal()` visibility to view. --- From 19374277ebd12991aa92a17088a9d10ac4cc6c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ernesto=20Garc=C3=ADa?= Date: Fri, 1 Nov 2024 16:03:54 +0700 Subject: [PATCH 7/8] Update contracts/governance/Governor.sol --- contracts/governance/Governor.sol | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 2d76847b24f..2f472307b28 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -117,8 +117,14 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * can be produced from the proposal data which is part of the {ProposalCreated} event. It can even be computed in * advance, before the proposal is submitted. * - * Note that the chainId and the governor address are not part of the proposal id computation, but can be added to - * guarantee more uniqueness if the same proposal is to submitted on multiple governors across multiple networks. + * Note that the chainId and the governor address are not part of the proposal id computation. Consequently, the + * same proposal will share its id with another governor in a different network where the same proposal was + * submitted. We recommend using the description field as a discriminator to avoid proposal id conflicts. + * Similarly, developers can add both `chainId` and `address(this)` to the hash computation by overriding + * this function. + * + * IMPORTANT: Overriding this function may have unexpected effects after an upgrade. Consider to verify + * that the new implementation doesn't allow a proposer to forge an id. */ function hashProposal( address[] memory targets, From 667d07df1bc0a4813a6929c9030b6a0cc0811a04 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Fri, 1 Nov 2024 16:04:37 +0700 Subject: [PATCH 8/8] Format changeset --- .changeset/popular-horses-tell.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/.changeset/popular-horses-tell.md b/.changeset/popular-horses-tell.md index 1b074de776a..db392718549 100644 --- a/.changeset/popular-horses-tell.md +++ b/.changeset/popular-horses-tell.md @@ -3,5 +3,3 @@ --- `Governor`: Change `hashProposal()` visibility to view. - ----