From 173eb778077b9f9d5d280467bd40569e319563c4 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 10 Jan 2022 16:36:10 +0100 Subject: [PATCH 1/6] fixing the way signature+calldata are encoded in the compatibility layer --- .../GovernorCompatibilityBravo.sol | 4 +- contracts/mocks/CallReceiverMock.sol | 7 + test/governance/GovernorWorkflow.behavior.js | 64 ++++-- .../GovernorCompatibilityBravo.test.js | 183 +++++++----------- 4 files changed, 128 insertions(+), 130 deletions(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 2c62f053224..9881acfe705 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.1 (governance/compatibility/GovernorCompatibilityBravo.sol) +// OpenZeppelin Contracts v4.4.0 (governance/compatibility/GovernorCompatibilityBravo.sol) pragma solidity ^0.8.0; @@ -132,7 +132,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp for (uint256 i = 0; i < signatures.length; ++i) { fullcalldatas[i] = bytes(signatures[i]).length == 0 ? calldatas[i] - : abi.encodeWithSignature(signatures[i], calldatas[i]); + : abi.encodePacked(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]); } return fullcalldatas; diff --git a/contracts/mocks/CallReceiverMock.sol b/contracts/mocks/CallReceiverMock.sol index 11d21b40586..926db68bf16 100644 --- a/contracts/mocks/CallReceiverMock.sol +++ b/contracts/mocks/CallReceiverMock.sol @@ -6,6 +6,7 @@ contract CallReceiverMock { string public sharedAnswer; event MockFunctionCalled(); + event MockFunctionCalledWithArgs(uint256 a, uint256 b); uint256[] private _array; @@ -15,6 +16,12 @@ contract CallReceiverMock { return "0x1234"; } + function mockFunctionWithArgs(uint256 a, uint256 b) public payable returns (string memory) { + emit MockFunctionCalledWithArgs(a, b); + + return "0x1234"; + } + function mockFunctionNonPayable() public returns (string memory) { emit MockFunctionCalled(); diff --git a/test/governance/GovernorWorkflow.behavior.js b/test/governance/GovernorWorkflow.behavior.js index 7bcaed06abf..2e01be49b63 100644 --- a/test/governance/GovernorWorkflow.behavior.js +++ b/test/governance/GovernorWorkflow.behavior.js @@ -18,11 +18,39 @@ function tryGet (obj, path = '') { } } +function zip (...args) { + return Array(Math.max(...args.map(array => array.length))) + .fill() + .map((_, i) => args.map(array => array[i])); +} + +function concatHex (...args) { + return web3.utils.bytesToHex([].concat(...args.map(h => web3.utils.hexToBytes(h || '0x')))); +} + function runGovernorWorkflow () { beforeEach(async function () { this.receipts = {}; + + this.useCompatibilityInterface = this.settings.proposal.length === 5; this.descriptionHash = web3.utils.keccak256(this.settings.proposal.slice(-1).find(Boolean)); - this.id = await this.mock.hashProposal(...this.settings.proposal.slice(0, -1), this.descriptionHash); + this.settings.shortProposal = [ + // targets + this.settings.proposal[0], + // values + this.settings.proposal[1], + // calldata (prefix selector if necessary) + this.useCompatibilityInterface + ? zip( + this.settings.proposal[2].map(selector => selector && web3.eth.abi.encodeFunctionSignature(selector)), + this.settings.proposal[3], + ).map(hexs => concatHex(...hexs)) + : this.settings.proposal[2], + // descriptionHash + this.descriptionHash, + ]; + + this.id = await this.mock.hashProposal(...this.settings.shortProposal); }); it('run', async function () { @@ -43,7 +71,11 @@ function runGovernorWorkflow () { // propose if (this.mock.propose && tryGet(this.settings, 'steps.propose.enable') !== false) { this.receipts.propose = await getReceiptOrRevert( - this.mock.methods['propose(address[],uint256[],bytes[],string)']( + this.mock.methods[ + this.useCompatibilityInterface + ? 'propose(address[],uint256[],string[],bytes[],string)' + : 'propose(address[],uint256[],bytes[],string)' + ]( ...this.settings.proposal, { from: this.settings.proposer }, ), @@ -103,11 +135,15 @@ function runGovernorWorkflow () { // queue if (this.mock.queue && tryGet(this.settings, 'steps.queue.enable') !== false) { this.receipts.queue = await getReceiptOrRevert( - this.mock.methods['queue(address[],uint256[],bytes[],bytes32)']( - ...this.settings.proposal.slice(0, -1), - this.descriptionHash, - { from: this.settings.queuer }, - ), + this.useCompatibilityInterface + ? this.mock.methods['queue(uint256)']( + this.id, + { from: this.settings.queuer }, + ) + : this.mock.methods['queue(address[],uint256[],bytes[],bytes32)']( + ...this.settings.shortProposal, + { from: this.settings.queuer }, + ), tryGet(this.settings, 'steps.queue.error'), ); this.eta = await this.mock.proposalEta(this.id); @@ -119,11 +155,15 @@ function runGovernorWorkflow () { // execute if (this.mock.execute && tryGet(this.settings, 'steps.execute.enable') !== false) { this.receipts.execute = await getReceiptOrRevert( - this.mock.methods['execute(address[],uint256[],bytes[],bytes32)']( - ...this.settings.proposal.slice(0, -1), - this.descriptionHash, - { from: this.settings.executer }, - ), + this.useCompatibilityInterface + ? this.mock.methods['execute(uint256)']( + this.id, + { from: this.settings.executer }, + ) + : this.mock.methods['execute(address[],uint256[],bytes[],bytes32)']( + ...this.settings.shortProposal, + { from: this.settings.executer }, + ), tryGet(this.settings, 'steps.execute.error'), ); if (tryGet(this.settings, 'steps.execute.delay')) { diff --git a/test/governance/compatibility/GovernorCompatibilityBravo.test.js b/test/governance/compatibility/GovernorCompatibilityBravo.test.js index 9153726eece..e877a552810 100644 --- a/test/governance/compatibility/GovernorCompatibilityBravo.test.js +++ b/test/governance/compatibility/GovernorCompatibilityBravo.test.js @@ -1,4 +1,4 @@ -const { BN, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { BN, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const Enums = require('../../helpers/enums'); const RLP = require('rlp'); @@ -11,24 +11,6 @@ const Timelock = artifacts.require('CompTimelock'); const Governor = artifacts.require('GovernorCompatibilityBravoMock'); const CallReceiver = artifacts.require('CallReceiverMock'); -async function getReceiptOrRevert (promise, error = undefined) { - if (error) { - await expectRevert(promise, error); - return undefined; - } else { - const { receipt } = await promise; - return receipt; - } -} - -function tryGet (obj, path = '') { - try { - return path.split('.').reduce((o, k) => o[k], obj); - } catch (_) { - return undefined; - } -} - function makeContractAddress (creator, nonce) { return web3.utils.toChecksumAddress(web3.utils.sha3(RLP.encode([creator, nonce])).slice(12).substring(14)); } @@ -194,6 +176,67 @@ contract('GovernorCompatibilityBravo', function (accounts) { runGovernorWorkflow(); }); + describe('with function selector and arguments', function () { + beforeEach(async function () { + this.settings = { + proposal: [ + Array(4).fill(this.receiver.address), + Array(4).fill(web3.utils.toWei('0')), + [ + '', + '', + 'mockFunctionNonPayable()', + 'mockFunctionWithArgs(uint256,uint256)', + ], + [ + this.receiver.contract.methods.mockFunction().encodeABI(), + this.receiver.contract.methods.mockFunctionWithArgs(17, 42).encodeABI(), + '0x', + web3.eth.abi.encodeParameters(['uint256', 'uint256'], [18, 43]), + ], + '', // description + ], + proposer, + tokenHolder: owner, + voters: [ + { + voter: voter1, + weight: web3.utils.toWei('10'), + support: Enums.VoteType.For, + }, + ], + steps: { + queue: { delay: 7 * 86400 }, + }, + }; + }); + runGovernorWorkflow(); + afterEach(async function () { + await expectEvent.inTransaction( + this.receipts.execute.transactionHash, + this.receiver, + 'MockFunctionCalled', + ); + await expectEvent.inTransaction( + this.receipts.execute.transactionHash, + this.receiver, + 'MockFunctionCalled', + ); + await expectEvent.inTransaction( + this.receipts.execute.transactionHash, + this.receiver, + 'MockFunctionCalledWithArgs', + { a: '17', b: '42' }, + ); + await expectEvent.inTransaction( + this.receipts.execute.transactionHash, + this.receiver, + 'MockFunctionCalledWithArgs', + { a: '18', b: '43' }, + ); + }); + }); + describe('proposalThreshold not reached', function () { beforeEach(async function () { this.settings = { @@ -266,8 +309,8 @@ contract('GovernorCompatibilityBravo', function (accounts) { proposal: [ [ this.receiver.address ], // targets [ web3.utils.toWei('0') ], // values - [ '' ], // signatures - [ this.receiver.contract.methods.mockFunction().encodeABI() ], // calldatas + [ 'mockFunction()' ], // signatures + [ '0x' ], // calldatas '', // description ], proposer, @@ -351,8 +394,8 @@ contract('GovernorCompatibilityBravo', function (accounts) { proposer, targets: this.settings.proposal[0], // values: this.settings.proposal[1].map(value => new BN(value)), - signatures: this.settings.proposal[2], - calldatas: this.settings.proposal[3], + signatures: this.settings.proposal[2].map(_ => ''), + calldatas: this.settings.shortProposal[2], startBlock: new BN(this.receipts.propose.blockNumber).add(this.votingDelay), endBlock: new BN(this.receipts.propose.blockNumber).add(this.votingDelay).add(this.votingPeriod), description: this.settings.proposal[4], @@ -378,98 +421,6 @@ contract('GovernorCompatibilityBravo', function (accounts) { 'MockFunctionCalled', ); }); - - it('run', async function () { - // transfer tokens - if (tryGet(this.settings, 'voters')) { - for (const voter of this.settings.voters) { - if (voter.weight) { - await this.token.transfer(voter.voter, voter.weight, { from: this.settings.tokenHolder }); - } - } - } - - // propose - if (this.mock.propose && tryGet(this.settings, 'steps.propose.enable') !== false) { - this.receipts.propose = await getReceiptOrRevert( - this.mock.methods['propose(address[],uint256[],string[],bytes[],string)']( - ...this.settings.proposal, - { from: this.settings.proposer }, - ), - tryGet(this.settings, 'steps.propose.error'), - ); - - if (tryGet(this.settings, 'steps.propose.error') === undefined) { - this.id = this.receipts.propose.logs.find(({ event }) => event === 'ProposalCreated').args.proposalId; - this.snapshot = await this.mock.proposalSnapshot(this.id); - this.deadline = await this.mock.proposalDeadline(this.id); - } - - if (tryGet(this.settings, 'steps.propose.delay')) { - await time.increase(tryGet(this.settings, 'steps.propose.delay')); - } - - if ( - tryGet(this.settings, 'steps.propose.error') === undefined && - tryGet(this.settings, 'steps.propose.noadvance') !== true - ) { - await time.advanceBlockTo(this.snapshot); - } - } - - // vote - if (tryGet(this.settings, 'voters')) { - this.receipts.castVote = []; - for (const voter of this.settings.voters) { - if (!voter.signature) { - this.receipts.castVote.push( - await getReceiptOrRevert( - this.mock.castVote(this.id, voter.support, { from: voter.voter }), - voter.error, - ), - ); - } else { - const { v, r, s } = await voter.signature({ proposalId: this.id, support: voter.support }); - this.receipts.castVote.push( - await getReceiptOrRevert( - this.mock.castVoteBySig(this.id, voter.support, v, r, s), - voter.error, - ), - ); - } - if (tryGet(voter, 'delay')) { - await time.increase(tryGet(voter, 'delay')); - } - } - } - - // fast forward - if (tryGet(this.settings, 'steps.wait.enable') !== false) { - await time.advanceBlockTo(this.deadline); - } - - // queue - if (this.mock.queue && tryGet(this.settings, 'steps.queue.enable') !== false) { - this.receipts.queue = await getReceiptOrRevert( - this.mock.methods['queue(uint256)'](this.id, { from: this.settings.queuer }), - tryGet(this.settings, 'steps.queue.error'), - ); - this.eta = await this.mock.proposalEta(this.id); - if (tryGet(this.settings, 'steps.queue.delay')) { - await time.increase(tryGet(this.settings, 'steps.queue.delay')); - } - } - - // execute - if (this.mock.execute && tryGet(this.settings, 'steps.execute.enable') !== false) { - this.receipts.execute = await getReceiptOrRevert( - this.mock.methods['execute(uint256)'](this.id, { from: this.settings.executer }), - tryGet(this.settings, 'steps.execute.error'), - ); - if (tryGet(this.settings, 'steps.execute.delay')) { - await time.increase(tryGet(this.settings, 'steps.execute.delay')); - } - } - }); + runGovernorWorkflow(); }); }); From 677073e582f54551f7ed21d9633474985ed68283 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 10 Jan 2022 16:53:48 +0100 Subject: [PATCH 2/6] revert version change --- .../governance/compatibility/GovernorCompatibilityBravo.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 9881acfe705..2f4cfa32e82 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts v4.4.0 (governance/compatibility/GovernorCompatibilityBravo.sol) +// OpenZeppelin Contracts v4.4.1 (governance/compatibility/GovernorCompatibilityBravo.sol) pragma solidity ^0.8.0; From f56dec99aad14eab2065830111ca4d5426f65dee Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 10 Jan 2022 17:24:01 +0100 Subject: [PATCH 3/6] use bytes.concat --- .../governance/compatibility/GovernorCompatibilityBravo.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 2f4cfa32e82..3e45d36ce39 100644 --- a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol +++ b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol @@ -132,7 +132,7 @@ abstract contract GovernorCompatibilityBravo is IGovernorTimelock, IGovernorComp for (uint256 i = 0; i < signatures.length; ++i) { fullcalldatas[i] = bytes(signatures[i]).length == 0 ? calldatas[i] - : abi.encodePacked(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]); + : bytes.concat(bytes4(keccak256(bytes(signatures[i]))), calldatas[i]); } return fullcalldatas; From 829ee5d37e16799fbbdc87690ce495167c0db32b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 10 Jan 2022 17:59:07 +0100 Subject: [PATCH 4/6] add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b27ca352f79..5e325bb917c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs`. ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056)) +### Bugfixes + * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposal submitted through the compatibility interface. Older versions incorrectly prefixed the calldata with the bytes length, leadding to incorrect call executions.([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) + ## 4.4.1 (2021-12-14) * `Initializable`: change the existing `initializer` modifier and add a new `onlyInitializing` modifier to prevent reentrancy risk. ([#3006](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3006)) From 8037b2f63b5cf75a8215ed490c45c8c0cff93008 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 11 Jan 2022 00:15:02 +0100 Subject: [PATCH 5/6] address comments from PR --- CHANGELOG.md | 2 +- test/governance/GovernorWorkflow.behavior.js | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e325bb917c..4945cec488d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056)) ### Bugfixes - * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposal submitted through the compatibility interface. Older versions incorrectly prefixed the calldata with the bytes length, leadding to incorrect call executions.([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) + * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposals submitted through the compatibility interface. Older versions incorrectly prefixed the calldata with the bytes length, leading to incorrect call executions. ([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) ## 4.4.1 (2021-12-14) diff --git a/test/governance/GovernorWorkflow.behavior.js b/test/governance/GovernorWorkflow.behavior.js index 2e01be49b63..8cfa9dcafd9 100644 --- a/test/governance/GovernorWorkflow.behavior.js +++ b/test/governance/GovernorWorkflow.behavior.js @@ -32,8 +32,15 @@ function runGovernorWorkflow () { beforeEach(async function () { this.receipts = {}; + // distinguish depending on the proposal length + // - length 4: propose(address[], uint256[], bytes[], string) → GovernorCore + // - length 5: propose(address[], uint256[], string[], bytes[], string) → GovernorCompatibilityBravo this.useCompatibilityInterface = this.settings.proposal.length === 5; + + // compute description hash this.descriptionHash = web3.utils.keccak256(this.settings.proposal.slice(-1).find(Boolean)); + + // condensed proposal, used for queue and execute operation this.settings.shortProposal = [ // targets this.settings.proposal[0], @@ -50,6 +57,7 @@ function runGovernorWorkflow () { this.descriptionHash, ]; + // proposal id this.id = await this.mock.hashProposal(...this.settings.shortProposal); }); From bd0a78cfc5df33adac614d17b5e8cbeda7b373b8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 11 Jan 2022 15:40:43 -0300 Subject: [PATCH 6/6] add changelog section for 4.4.2 --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4945cec488d..920ef480560 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,10 @@ * `Base64`: add a library to parse bytes into base64 strings using `encode(bytes memory)` function, and provide examples to show how to use to build URL-safe `tokenURIs`. ([#2884](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2884)) * `ERC20`: reduce allowance before triggering transfer. ([#3056](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3056)) +## 4.4.2 (2022-01-11) + ### Bugfixes - * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposals submitted through the compatibility interface. Older versions incorrectly prefixed the calldata with the bytes length, leading to incorrect call executions. ([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) + * `GovernorCompatibilityBravo`: Fix error in the encoding of calldata for proposals submitted through the compatibility interface with explicit signatures. ([#3100](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#3100)) ## 4.4.1 (2021-12-14)