Add function documentation for SignatureChecker.

(cherry picked from commit f55d2716a8)

.github/workflows/docs.yml

@@ -0,0 +1,25 @@
+name: Build Docs
+
+on:
+  push:
+    branches: [release-v*]
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions/setup-node@v2
+        with:
+          node-version: 12.x
+      - uses: actions/cache@v2
+        id: cache
+        with:
+          path: '**/node_modules'
+          key: npm-v2-${{ hashFiles('**/package-lock.json') }}
+          restore-keys: npm-v2-
+      - run: npm ci
+        if: steps.cache.outputs.cache-hit != 'true'
+      - run: bash scripts/git-user-config.sh
+      - run: node scripts/update-docs-branch.js
+      - run: git push --all origin 

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",

contracts/token/ERC20/extensions/ERC20Votes.sol

@@ -20,8 +20,6 @@ import "../../../utils/cryptography/ECDSA.sol";
  *
  * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it
  * requires users to delegate to themselves in order to activate checkpoints and have their voting power tracked.
- * Enabling self-delegation can easily be done by overriding the {delegates} function. Keep in mind however that this
- * will significantly increase the base gas cost of transfers.
  *
  * _Available since v4.2._
  */

contracts/token/ERC20/extensions/ERC20VotesComp.sol

@@ -19,8 +19,6 @@ import "./ERC20Votes.sol";
  *
  * By default, token balance does not account for voting power. This makes transfers cheaper. The downside is that it
  * requires users to delegate to themselves in order to activate checkpoints and have their voting power tracked.
- * Enabling self-delegation can easily be done by overriding the {delegates} function. Keep in mind however that this
- * will significantly increase the base gas cost of transfers.
  *
  * _Available since v4.2._
  */

contracts/utils/cryptography/SignatureChecker.sol

@@ -8,16 +8,20 @@ import "../Address.sol";
 import "../../interfaces/IERC1271.sol";
 
 /**
- * @dev Signature verification helper: Provide a single mechanism to verify both private-key (EOA) ECDSA signature and
- * ERC1271 contract signatures. Using this instead of ECDSA.recover in your contract will make them compatible with
- * smart contract wallets such as Argent and Gnosis.
- *
- * Note: unlike ECDSA signatures, contract signature's are revocable, and the outcome of this function can thus change
- * through time. It could return true at block N and false at block N+1 (or the opposite).
+ * @dev Signature verification helper that can be used instead of `ECDSA.recover` to seamlessly support both ECDSA
+ * signatures from externally owned accounts (EOAs) as well as ERC1271 signatures from smart contract wallets like
+ * Argent and Gnosis Safe.
  *
  * _Available since v4.1._
  */
 library SignatureChecker {
+    /**
+     * @dev Checks if a signature is valid for a given signer and data hash. If the signer is a smart contract, the
+     * signature is validated against that smart contract using ERC1271, otherwise it's validated using `ECDSA.recover`.
+     *
+     * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus
+     * change through time. It could return true at block N and false at block N+1 (or the opposite).
+     */
     function isValidSignatureNow(
         address signer,
         bytes32 hash,

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/git-user-config.sh

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+set -euo pipefail -x
+
+git config user.name 'github-actions'
+git config user.email '41898282+github-actions[bot]@users.noreply.github.com'

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

scripts/update-docs-branch.js

@@ -0,0 +1,55 @@
+const proc = require('child_process');
+const read = cmd => proc.execSync(cmd, { encoding: 'utf8' }).trim();
+const run = cmd => { proc.execSync(cmd, { stdio: 'inherit' }); };
+const tryRead = cmd => { try { return read(cmd); } catch (e) { return undefined; } };
+
+const releaseBranchRegex = /^release-v(?<version>(?<major>\d+)\.(?<minor>\d+)(?:\.(?<patch>\d+))?)$/;
+
+const currentBranch = read('git rev-parse --abbrev-ref HEAD');
+const match = currentBranch.match(releaseBranchRegex);
+
+if (!match) {
+  console.error('Not currently on a release branch');
+  process.exit(1);
+}
+
+if (/-.*$/.test(require('../package.json').version)) {
+  console.error('Refusing to update docs: prerelease detected');
+  process.exit(0);
+}
+
+const current = match.groups;
+const docsBranch = `docs-v${current.major}.x`;
+
+// Fetch remotes and find the docs branch if it exists
+run('git fetch --all --no-tags');
+const matchingDocsBranches = tryRead(`git rev-parse --glob='*/${docsBranch}'`);
+
+if (!matchingDocsBranches) {
+  // Create the branch
+  run(`git checkout --orphan ${docsBranch}`);
+} else {
+  const [publishedRef, ...others] = new Set(matchingDocsBranches.split('\n'));
+  if (others.length > 0) {
+    console.error(
+      `Found conflicting ${docsBranch} branches.\n` +
+      'Either local branch is outdated or there are multiple matching remote branches.',
+    );
+    process.exit(1);
+  }
+  const publishedVersion = JSON.parse(read(`git show ${publishedRef}:package.json`)).version;
+  const publishedMinor = publishedVersion.match(/\d+\.(?<minor>\d+)\.\d+/).groups.minor;
+  if (current.minor < publishedMinor) {
+    console.error('Refusing to update docs: newer version is published');
+    process.exit(0);
+  }
+
+  run('git checkout --quiet --detach');
+  run(`git reset --soft ${publishedRef}`);
+  run(`git checkout ${docsBranch}`);
+}
+
+run('npm run prepare-docs');
+run('git add -f docs'); // --force needed because generated docs files are gitignored
+run('git commit -m "Update docs"');
+run(`git checkout ${currentBranch}`);

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