Fix encoding of signature+calldata in GovernorCompatibilityBravo

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

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;

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

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

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