Merge remote-tracking branch 'vanilla/master' into patches

CHANGELOG.md

@@ -2,6 +2,20 @@
 
 ## Unreleased
 
+ * `Ownable`: add an internal `_transferOwnership(address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
+ * `AccessControl`: add internal `_grantRole(bytes32,address)` and `_revokeRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
+ * `AccessControl`: mark `_setupRole(bytes32,address)` as deprecated in favor of `_grantRole(bytes32,address)`. ([#2568](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/#2568))
+
+## 4.3.2
+
+ * `UUPSUpgradeable`: Add modifiers to prevent `upgradeTo` and `upgradeToAndCall` being executed on any contract that is not the active ERC1967 proxy. This prevents these functions being called on implementation contracts or minimal ERC1167 clones, in particular.
+
+## 4.3.1
+
+ * `TimelockController`: Add additional isOperationReady check.
+
+## 4.3.0 (2021-08-17)
+
  * `ERC2771Context`: use private variable from storage to store the forwarder address. Fixes issues where `_msgSender()` was not callable from constructors. ([#2754](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2754))
  * `EnumerableSet`: add `values()` functions that returns an array containing all values in a single call. ([#2768](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2768))
  * `Governor`: added a modular system of `Governor` contracts based on `GovernorAlpha` and `GovernorBravo`. ([#2672](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2672))
@@ -95,6 +109,14 @@ Make sure you're using git or another version control system to be able to recov
 
 Some further changes have been done between the different beta iterations. Transitions made during this period are configured in the `migrate-imports` script. Consequently, you can upgrade from any previous 4.0-beta.x version using the same script as described in the *How to upgrade from 3.x* section.
 
+## 3.4.2
+
+ * `TimelockController`: Add additional isOperationReady check.
+
+## 3.4.1 (2021-03-03)
+
+ * `ERC721`: made `_approve` an internal function (was private).
+
 ## 3.4.0 (2021-02-02)
 
  * `BeaconProxy`: added new kind of proxy that allows simultaneous atomic upgrades. ([#2411](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2411))

contracts/access/AccessControl.sol

@@ -178,6 +178,8 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
      * Using this function in any other way is effectively circumventing the admin
      * system imposed by {AccessControl}.
      * ====
+     *
+     * NOTE: This function is deprecated in favor of {_grantRole}.
      */
     function _setupRole(bytes32 role, address account) internal virtual {
         _grantRole(role, account);
@@ -194,14 +196,24 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
         emit RoleAdminChanged(role, previousAdminRole, adminRole);
     }
 
-    function _grantRole(bytes32 role, address account) private {
+    /**
+     * @dev Grants `role` to `account`.
+     *
+     * Internal function without access restriction.
+     */
+    function _grantRole(bytes32 role, address account) internal virtual {
         if (!hasRole(role, account)) {
             _roles[role].members[account] = true;
             emit RoleGranted(role, account, _msgSender());
         }
     }
 
-    function _revokeRole(bytes32 role, address account) private {
+    /**
+     * @dev Revokes `role` from `account`.
+     *
+     * Internal function without access restriction.
+     */
+    function _revokeRole(bytes32 role, address account) internal virtual {
         if (hasRole(role, account)) {
             _roles[role].members[account] = false;
             emit RoleRevoked(role, account, _msgSender());

contracts/access/Ownable.sol

@@ -25,7 +25,7 @@ abstract contract Ownable is Context {
      * @dev Initializes the contract setting the deployer as the initial owner.
      */
     constructor() {
-        _setOwner(_msgSender());
+        _transferOwnership(_msgSender());
     }
 
     /**
@@ -51,7 +51,7 @@ abstract contract Ownable is Context {
      * thereby removing any functionality that is only available to the owner.
      */
     function renounceOwnership() public virtual onlyOwner {
-        _setOwner(address(0));
+        _transferOwnership(address(0));
     }
 
     /**
@@ -60,10 +60,14 @@ abstract contract Ownable is Context {
      */
     function transferOwnership(address newOwner) public virtual onlyOwner {
         require(newOwner != address(0), "Ownable: new owner is the zero address");
-        _setOwner(newOwner);
+        _transferOwnership(newOwner);
     }
 
-    function _setOwner(address newOwner) private {
+    /**
+     * @dev Transfers ownership of the contract to a new account (`newOwner`).
+     * Internal function without access restriction.
+     */
+    function _transferOwnership(address newOwner) internal virtual {
         address oldOwner = _owner;
         _owner = newOwner;
         emit OwnershipTransferred(oldOwner, newOwner);

contracts/finance/PaymentSplitter.sol

@@ -4,7 +4,6 @@ pragma solidity ^0.8.0;
 
 import "../utils/Address.sol";
 import "../utils/Context.sol";
-import "../utils/math/SafeMath.sol";
 
 /**
  * @title PaymentSplitter

contracts/governance/TimelockController.sol

@@ -268,7 +268,7 @@ contract TimelockController is AccessControl {
         bytes32 salt
     ) public payable virtual onlyRoleOrOpenRole(EXECUTOR_ROLE) {
         bytes32 id = hashOperation(target, value, data, predecessor, salt);
-        _beforeCall(predecessor);
+        _beforeCall(id, predecessor);
         _call(id, 0, target, value, data);
         _afterCall(id);
     }
@@ -293,7 +293,7 @@ contract TimelockController is AccessControl {
         require(targets.length == datas.length, "TimelockController: length mismatch");
 
         bytes32 id = hashOperationBatch(targets, values, datas, predecessor, salt);
-        _beforeCall(predecessor);
+        _beforeCall(id, predecessor);
         for (uint256 i = 0; i < targets.length; ++i) {
             _call(id, i, targets[i], values[i], datas[i]);
         }
@@ -303,7 +303,8 @@ contract TimelockController is AccessControl {
     /**
      * @dev Checks before execution of an operation's calls.
      */
-    function _beforeCall(bytes32 predecessor) private view {
+    function _beforeCall(bytes32 id, bytes32 predecessor) private view {
+        require(isOperationReady(id), "TimelockController: operation is not ready");
         require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency");
     }
 

contracts/governance/compatibility/GovernorCompatibilityBravo.sol

@@ -15,6 +15,8 @@ import "./IGovernorCompatibilityBravo.sol";
  * This compatibility layer includes a voting system and requires a {IGovernorTimelock} compatible module to be added
  * through inheritance. It does not include token bindings, not does it include any variable upgrade patterns.
  *
+ * NOTE: When using this module, you may need to enable the Solidity optimizer to avoid hitting the contract size limit.
+ *
  * _Available since v4.3._
  */
 abstract contract GovernorCompatibilityBravo is

contracts/package.json

 {
   "name": "@openzeppelin/contracts-upgradeable",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.2.0",
+  "version": "4.3.0",
   "files": [
     "**/*.sol",
     "/build/contracts/*.json",

contracts/proxy/utils/Initializable.sol

@@ -13,6 +13,21 @@ pragma solidity ^0.8.0;
  *
  * CAUTION: When used with inheritance, manual care must be taken to not invoke a parent initializer twice, or to ensure
  * that all initializers are idempotent. This is not verified automatically as constructors are by Solidity.
+ *
+ * [CAUTION]
+ * ====
+ * Avoid leaving a contract uninitialized.
+ *
+ * An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation
+ * contract, which may impact the proxy. To initialize the implementation contract, you can either invoke the
+ * initializer manually, or you can include a constructor to automatically mark it as initialized when it is deployed:
+ *
+ * [.hljs-theme-light.nopadding]
+ * ```
+ * /// @custom:oz-upgrades-unsafe-allow constructor
+ * constructor() initializer {}
+ * ```
+ * ====
  */
 abstract contract Initializable {
     /**

contracts/proxy/utils/UUPSUpgradeable.sol

@@ -17,6 +17,22 @@ import "../ERC1967/ERC1967Upgrade.sol";
  * _Available since v4.1._
  */
 abstract contract UUPSUpgradeable is ERC1967Upgrade {
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
+    address private immutable __self = address(this);
+
+    /**
+     * @dev Check that the execution is being performed through a delegatecall call and that the execution context is
+     * a proxy contract with an implementation (as defined in ERC1967) pointing to self. This should only be the case
+     * for UUPS and transparent proxies that are using the current contract as their implementation. Execution of a
+     * function through ERC1167 minimal proxies (clones) would not normally pass this test, but is not guaranteed to
+     * fail.
+     */
+    modifier onlyProxy() {
+        require(address(this) != __self, "Function must be called through delegatecall");
+        require(_getImplementation() == __self, "Function must be called through active proxy");
+        _;
+    }
+
     /**
      * @dev Upgrade the implementation of the proxy to `newImplementation`.
      *
@@ -24,9 +40,9 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade {
      *
      * Emits an {Upgraded} event.
      */
-    function upgradeTo(address newImplementation) external virtual {
+    function upgradeTo(address newImplementation) external virtual onlyProxy {
         _authorizeUpgrade(newImplementation);
-        _upgradeToAndCallSecure(newImplementation, bytes(""), false);
+        _upgradeToAndCallSecure(newImplementation, new bytes(0), false);
     }
 
     /**
@@ -37,7 +53,7 @@ abstract contract UUPSUpgradeable is ERC1967Upgrade {
      *
      * Emits an {Upgraded} event.
      */
-    function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual {
+    function upgradeToAndCall(address newImplementation, bytes memory data) external payable virtual onlyProxy {
         _authorizeUpgrade(newImplementation);
         _upgradeToAndCallSecure(newImplementation, data, true);
     }

contracts/security/ReentrancyGuard.sol

@@ -43,7 +43,7 @@ abstract contract ReentrancyGuard {
      * @dev Prevents a contract from calling itself, directly or indirectly.
      * Calling a `nonReentrant` function from another `nonReentrant`
      * function is not supported. It is possible to prevent this from happening
-     * by making the `nonReentrant` function external, and make it call a
+     * by making the `nonReentrant` function external, and making it call a
      * `private` function that does the actual work.
      */
     modifier nonReentrant() {

docs/modules/ROOT/pages/erc721.adoc

@@ -40,7 +40,7 @@ contract GameItem is ERC721URIStorage {
 }
 ----
 
-The xref:api:token/ERC721.adoc#ERC721URIStorage[`ERC721URIStorage`] contract is an implementation of ERC721 that includes all standard extensions (xref:api:token/ERC721.adoc#IERC721Metadata[`IERC721Metadata`] and xref:api:token/ERC721.adoc#IERC721Enumerable[`IERC721Enumerable`]). That's where the xref:api:token/ERC721.adoc#ERC721-_setTokenURI-uint256-string-[`_setTokenURI`] method comes from: we use it to store an item's metadata.
+The xref:api:token/ERC721.adoc#ERC721URIStorage[`ERC721URIStorage`] contract is an implementation of ERC721 that includes the metadata standard extensions (xref:api:token/ERC721.adoc#IERC721Metadata[`IERC721Metadata`]) as well as a mechanism for per-token metadata. That's where the xref:api:token/ERC721.adoc#ERC721-_setTokenURI-uint256-string-[`_setTokenURI`] method comes from: we use it to store an item's metadata.
 
 Also note that, unlike ERC20, ERC721 lacks a `decimals` field, since each token is distinct and cannot be partitioned.
 

docs/modules/ROOT/pages/governance.adoc

@@ -247,6 +247,8 @@ contract MyGovernor is Governor, GovernorCompatibilityBravo, GovernorVotes, Gove
 
 It is good practice to add a timelock to governance decisions. This allows users to exit the system if they disagree with a decision before it is executed. We will use OpenZeppelin’s TimelockController in combination with the GovernorTimelockControl module.
 
+IMPORTANT: When using a timelock, it is the timelock that will execute proposals and thus the timelock that should hold any funds, ownership, and access control roles. Funds in the Governor contract are not currently retrievable when using a timelock! (As of version 4.3 there is a caveat when using the Compound Timelock: ETH in the timelock is not easily usable, so it is recommended to manage ERC20 funds only in this combination until a future version resolves the issue.)
+
 TimelockController uses an AccessControl setup that we need to understand in order to set up roles. The Proposer role is in charge of queueing operations: this is the role the Governor instance should be granted, and it should likely be the only proposer in the system. The Executor role is in charge of executing already available operations: we can assign this role to the special zero address to allow anyone to execute (if operations can be particularly time sensitive, the Governor should be made Executor instead). Lastly, there is the Admin role, which can grant and revoke the two previous roles: this is a very sensitive role that will be granted automatically to both deployer and timelock itself, but should be renounced by the deployer after setup.
 
 == Proposal Lifecycle

hardhat.config.js

 /// ENVVAR
-// - ENABLE_GAS_REPORT
-// - CI
-// - COMPILE_MODE
+// - CI:                output gas report to file instead of stdout
+// - COVERAGE:          enable coverage report
+// - ENABLE_GAS_REPORT: enable gas report
+// - COMPILE_MODE:      production modes enables optimizations (default: development)
+// - COMPILE_VERSION:   compiler version (default: 0.8.3)
+// - COINMARKETCAP:     coinmarkercat api key for USD value in gas report
 
 const fs = require('fs');
 const path = require('path');
@@ -12,6 +15,10 @@ const argv = require('yargs/yargs')()
       type: 'boolean',
       default: false,
     },
+    coverage: {
+      type: 'boolean',
+      default: false,
+    },
     gas: {
       alias: 'enableGasReport',
       type: 'boolean',
@@ -28,11 +35,14 @@ const argv = require('yargs/yargs')()
       type: 'string',
       default: '0.8.3',
     },
+    coinmarketcap: {
+      alias: 'coinmarketcapApiKey',
+      type: 'string',
+    },
   })
   .argv;
 
 require('@nomiclabs/hardhat-truffle5');
-require('solidity-coverage');
 
 if (argv.enableGasReport) {
   require('hardhat-gas-reporter');
@@ -59,7 +69,6 @@ module.exports = {
   },
   networks: {
     hardhat: {
-      hardfork: process.env.COVERAGE ? 'berlin' : 'london',
       blockGasLimit: 10000000,
       allowUnlimitedContractSize: !withOptimizations,
     },
@@ -67,5 +76,11 @@ module.exports = {
   gasReporter: {
     currency: 'USD',
     outputFile: argv.ci ? 'gas-report.txt' : undefined,
+    coinmarketcap: argv.coinmarketcap,
   },
 };
+
+if (argv.coverage) {
+  require('solidity-coverage');
+  module.exports.networks.hardhat.initialBaseFeePerGas = 0;
+}

package-lock.json

 {
   "name": "openzeppelin-solidity",
-  "version": "4.2.0",
+  "version": "4.3.0",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {
@@ -13736,9 +13736,9 @@
       "dev": true
     },
     "tar": {
-      "version": "4.4.17",
-      "resolved": "https://registry.npmjs.org/tar/-/tar-4.4.17.tgz",
-      "integrity": "sha512-q7OwXq6NTdcYIa+k58nEMV3j1euhDhGCs/VRw9ymx/PbH0jtIM2+VTgDE/BW3rbLkrBUXs5fzEKgic5oUciu7g==",
+      "version": "4.4.19",
+      "resolved": "https://registry.npmjs.org/tar/-/tar-4.4.19.tgz",
+      "integrity": "sha512-a20gEsvHnWe0ygBY8JbxoM4w3SJdhc7ZAuxkLqh+nvNQN2IOt0B5lLgM490X5Hl8FF0dl0tOf2ewFYAlIFgzVA==",
       "dev": true,
       "requires": {
         "chownr": "^1.1.4",

package.json

@@ -2,7 +2,7 @@
   "private": true,
   "name": "openzeppelin-solidity",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.2.0",
+  "version": "4.3.0",
   "files": [
     "/contracts/**/*.sol",
     "/build/contracts/*.json",
@@ -23,8 +23,8 @@
     "lint:js:fix": "eslint --ignore-path .gitignore . --fix",
     "lint:sol": "solhint 'contracts/**/*.sol' && prettier -c 'contracts/**/*.sol'",
     "lint:sol:fix": "prettier --write \"contracts/**/*.sol\"",
-    "prepublish": "rimraf build contracts/build artifacts cache",
-    "prepare": "env COMPILE_MODE=production npm run compile",
+    "clean": "hardhat clean && rimraf build contracts/build",
+    "prepare": "npm run clean && env COMPILE_MODE=production npm run compile",
     "prepack": "scripts/prepack.sh",
     "release": "scripts/release/release.sh",
     "version": "scripts/release/version.sh",

scripts/prepare-contracts-package.sh

@@ -5,7 +5,7 @@ cd "$(git rev-parse --show-toplevel)"
 
 # avoids re-compilation during publishing of both packages
 if [[ ! -v ALREADY_COMPILED ]]; then
-  npm run prepublish
+  npm run clean
   npm run prepare
   npm run prepack
 fi