Skip to content

Commit

Permalink
Fix encoding of signature+calldata in GovernorCompatibilityBravo (Ope…
Browse files Browse the repository at this point in the history 
…nZeppelin#3100)

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
  • Loading branch information
@Amxx @frangio
Amxx and frangio authored Jan 11, 2022
1 parent 80d8da0 commit c366de3
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 129 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@

Solidity pragma in `utils/Address.sol` is increased from `^0.8.0` to `^0.8.1`. This is required by the `account.code.length` syntax that replaces inline assembly. This may require users to bump their compiler version from `0.8.0` to `0.8.1` or later. Note that other parts of the code already include stricter requirements.

## 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))
Expand Down
2 changes: 1 addition & 1 deletion contracts/governance/compatibility/GovernorCompatibilityBravo.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions contracts/mocks/CallReceiverMock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ contract CallReceiverMock {
string public sharedAnswer;

event MockFunctionCalled();
event MockFunctionCalledWithArgs(uint256 a, uint256 b);

uint256[] private _array;

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

Expand Down
72 changes: 60 additions & 12 deletions test/governance/GovernorWorkflow.behavior.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -43,7 +79,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 },
),
Expand Down Expand Up @@ -103,11 +143,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);
Expand All @@ -119,11 +163,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')) {
Expand Down
183 changes: 67 additions & 116 deletions test/governance/compatibility/GovernorCompatibilityBravo.test.js
View file
Original file line number Diff line number Diff line change
@@ -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');

Expand All @@ -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));
}
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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],
Expand All @@ -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();
});
});

0 comments on commit c366de3

Please sign in to comment.