diff --git a/CHANGELOG.md b/CHANGELOG.md index 159f2f954b6..7091041e2a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 4.4.2 (2022-01-11) + +### Bugfixes + * `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) * `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)) diff --git a/contracts/governance/compatibility/GovernorCompatibilityBravo.sol b/contracts/governance/compatibility/GovernorCompatibilityBravo.sol index 2c62f053224..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.encodeWithSignature(signatures[i], calldatas[i]); + : bytes.concat(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 70319cd44d3..3a61b1b34a0 100644 --- a/test/governance/GovernorWorkflow.behavior.js +++ b/test/governance/GovernorWorkflow.behavior.js @@ -18,11 +18,47 @@ 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 = {}; + + // 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)); - this.id = await this.mock.hashProposal(...this.settings.proposal.slice(0, -1), this.descriptionHash); + + // condensed proposal, used for queue and execute operation + 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, + ]; + + // proposal id + this.id = await this.mock.hashProposal(...this.settings.shortProposal); }); it('run', async function () { @@ -38,7 +74,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 }, ), @@ -98,11 +138,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); @@ -114,11 +158,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(); }); });