4.4.2

(cherry picked from commit 1051db3802)

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

contracts/governance/compatibility/GovernorCompatibilityBravo.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-// OpenZeppelin Contracts v4.4.1 (governance/compatibility/GovernorCompatibilityBravo.sol)
+// OpenZeppelin Contracts (last updated v4.4.2) (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;

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

contracts/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@openzeppelin/contracts",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.4.1",
+  "version": "4.4.2",
   "files": [
     "**/*.sol",
     "/build/contracts/*.json",

package.json

@@ -1,7 +1,7 @@
 {
   "name": "openzeppelin-solidity",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.4.1",
+  "version": "4.4.2",
   "files": [
     "/contracts/**/*.sol",
     "/build/contracts/*.json",
@@ -78,6 +78,7 @@
     "prettier": "^2.3.0",
     "prettier-plugin-solidity": "^1.0.0-beta.16",
     "rimraf": "^3.0.2",
+    "semver": "^7.3.5",
     "solhint": "^3.3.6",
     "solidity-ast": "^0.4.25",
     "solidity-coverage": "^0.7.11",

scripts/release/update-comment.js

@@ -1,11 +1,10 @@
 #!/usr/bin/env node
-
 const fs = require('fs');
-const glob = require('glob');
 const proc = require('child_process');
+const semver = require('semver');
+const run = (cmd, ...args) => proc.execFileSync(cmd, args, { encoding: 'utf8' }).trim();
 
-const gitStatus = proc.execFileSync('git', ['status', '--porcelain', '-uno', 'contracts/**/*.sol']);
-
+const gitStatus = run('git', 'status', '--porcelain', '-uno', 'contracts/**/*.sol');
 if (gitStatus.length > 0) {
   console.error('Contracts directory is not clean');
   process.exit(1);
@@ -13,15 +12,24 @@ if (gitStatus.length > 0) {
 
 const { version } = require('../../package.json');
 
-const files = glob.sync('contracts/!(mocks)/**/*.sol');
+// Get latest tag according to semver.
+const [ tag ] = run('git', 'tag')
+  .split(/\r?\n/)
+  .filter(v => semver.lt(semver.coerce(v), version)) // only consider older tags, ignore current prereleases
+  .sort(semver.rcompare);
+
+// Ordering tag → HEAD is important here.
+const files = run('git', 'diff', tag, 'HEAD', '--name-only', 'contracts/**/*.sol')
+  .split(/\r?\n/)
+  .filter(file => file && !file.match(/mock/i));
 
 for (const file of files) {
   const current = fs.readFileSync(file, 'utf8');
   const updated = current.replace(
-    /(\/\/ SPDX-License-Identifier:.*)$(\n\/\/ OpenZeppelin Contracts v.*$)?/m,
-    `$1\n// OpenZeppelin Contracts v${version} (${file.replace('contracts/', '')})`,
+    /(\/\/ SPDX-License-Identifier:.*)$(\n\/\/ OpenZeppelin Contracts .*$)?/m,
+    `$1\n// OpenZeppelin Contracts (last updated v${version}) (${file.replace('contracts/', '')})`,
   );
   fs.writeFileSync(file, updated);
 }
 
-proc.execFileSync('git', ['add', '--update', 'contracts']);
+run('git', 'add', '--update', 'contracts');

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

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]),
+          ],
+          '<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();
   });
 });