mirror
/
openzeppelin-contracts
mirror of https://github.com/OpenZeppelin/openzeppelin-contracts
2
1
Fork 1
Code Issues Releases Wiki Activity

Fix encoding of signature+calldata in GovernorCompatibilityBravo (#3100)

Browse Source 
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
(cherry picked from commit c366de3626)
release-v4.4
Hadrien Croubois 3 years ago committed by Francisco Giordano
parent 66436cbb4e
commit eff4ad7c1d
5 changed files with 140 additions and 129 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 5
      CHANGELOG.md
  2. 2
      contracts/governance/compatibility/GovernorCompatibilityBravo.sol
  3. 7
      contracts/mocks/CallReceiverMock.sol
  4. 72
      test/governance/GovernorWorkflow.behavior.js
  5. 183
      test/governance/compatibility/GovernorCompatibilityBravo.test.js

5
CHANGELOG.md
Unescape View File

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

2
contracts/governance/compatibility/GovernorCompatibilityBravo.sol
Unescape View File

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

7
contracts/mocks/CallReceiverMock.sol
Unescape View File

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

72
test/governance/GovernorWorkflow.behavior.js
Unescape View File

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

183
test/governance/compatibility/GovernorCompatibilityBravo.test.js
Unescape View File

@ -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]),
],
'<proposal description>', // 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
'<proposal description>', // 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();
});
});

Write Preview
Loading…
Cancel
Save