From ea595f59605534945a3d349a2f86a26fc7d3b9d1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 13 Apr 2023 20:47:51 +0200 Subject: [PATCH] Merge pull request from GHSA-93hq-5wgc-jc82 Co-authored-by: Francisco (cherry picked from commit 8d633cb7d169f2f8595b273660b00b69e845c2fe) --- .changeset/silent-pugs-scream.md | 5 +++++ .../compatibility/GovernorCompatibilityBravo.sol | 8 ++++++-- .../GovernorCompatibilityBravo.test.js | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 .changeset/silent-pugs-scream.md diff --git a/.changeset/silent-pugs-scream.md b/.changeset/silent-pugs-scream.md new file mode 100644 index 00000000000..c92d12486b3 --- /dev/null +++ b/.changeset/silent-pugs-scream.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`GovernorCompatibilityBravo`: Fix encoding of proposal data when signatures are missing. diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index a903ae98de0..997a81097ca 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -69,6 +69,11 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp bytes[] memory calldatas, string memory description ) public virtual override returns (uint256) { + require(signatures.length == calldatas.length, "GovernorBravo: invalid signatures length"); + // Stores the full proposal and fallback to the public (possibly overridden) propose. The fallback is done + // after the full proposal is stored, so the store operation included in the fallback will be skipped. Here we + // call `propose` and not `super.propose` to make sure if a child contract override `propose`, whatever code + // is added their is also executed when calling this alternative interface. _storeProposal(_msgSender(), targets, values, signatures, calldatas, description); return propose(targets, values, _encodeCalldata(signatures, calldatas), description); } @@ -124,8 +129,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp returns (bytes[] memory) { bytes[] memory fullcalldatas = new bytes[](calldatas.length); - - for (uint256 i = 0; i < signatures.length; ++i) { + for (uint256 i = 0; i < fullcalldatas.length; ++i) { fullcalldatas[i] = bytes(signatures[i]).length == 0 ? calldatas[i] : abi.encodePacked(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]); diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 79950474cf7..386a7e80ec3 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -223,6 +223,21 @@ contract('GovernorCompatibilityBravo', function (accounts) { ); }); + it('with inconsistent array size for selector and arguments', async function () { + const target = this.receiver.address; + this.helper.setProposal( + { + targets: [target, target], + values: [0, 0], + signatures: ['mockFunction()'], // One signature + data: ['0x', this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI()], // Two data entries + }, + '', + ); + + await expectRevert(this.helper.propose({ from: proposer }), 'GovernorBravo: invalid signatures length'); + }); + describe('should revert', function () { describe('on propose', function () { it('if proposal does not meet proposalThreshold', async function () {