Merge branch 'master' into fix/eip712-for-delegatecalls

Author: Amxx

CHANGELOG.md

@@ -1,6 +1,16 @@
 # Changelog
 
-## 4.3.1
+## 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 (2021-09-14)
+
+ * `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 (2021-08-26)
 
  * `TimelockController`: Add additional isOperationReady check.
 

contracts/access/AccessControl.sol

@@ -150,7 +150,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
      * purpose is to provide a mechanism for accounts to lose their privileges
      * if they are compromised (such as when a trusted device is misplaced).
      *
-     * If the calling account had been granted `role`, emits a {RoleRevoked}
+     * If the calling account had been revoked `role`, emits a {RoleRevoked}
      * event.
      *
      * Requirements:
@@ -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/governance/Governor.sol

@@ -55,6 +55,13 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor {
         _name = name_;
     }
 
+    /**
+     * @dev Function to receive ETH that will be handled by the governor (disabled if executor is a third party contract)
+     */
+    receive() external payable virtual {
+        require(_executor() == address(this));
+    }
+
     /**
      * @dev See {IERC165-supportsInterface}.
      */

contracts/governance/extensions/GovernorCountingSimple.sol

@@ -70,7 +70,7 @@ abstract contract GovernorCountingSimple is Governor {
     }
 
     /**
-     * @dev See {Governor-_voteSucceeded}. In this module, the forVotes must be scritly over the againstVotes.
+     * @dev See {Governor-_voteSucceeded}. In this module, the forVotes must be strictly over the againstVotes.
      */
     function _voteSucceeded(uint256 proposalId) internal view virtual override returns (bool) {
         ProposalVote storage proposalvote = _proposalVotes[proposalId];

contracts/governance/extensions/GovernorTimelockCompound.sol

@@ -177,8 +177,9 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor {
     ) internal virtual override {
         uint256 eta = proposalEta(proposalId);
         require(eta > 0, "GovernorTimelockCompound: proposal not yet queued");
+        Address.sendValue(payable(_timelock), msg.value);
         for (uint256 i = 0; i < targets.length; ++i) {
-            _timelock.executeTransaction{value: values[i]}(targets[i], values[i], "", calldatas[i], eta);
+            _timelock.executeTransaction(targets[i], values[i], "", calldatas[i], eta);
         }
     }
 

contracts/mocks/GovernorCompMock.sol

@@ -20,8 +20,6 @@ contract GovernorCompMock is Governor, GovernorVotesComp, GovernorCountingSimple
         _votingPeriod = votingPeriod_;
     }
 
-    receive() external payable {}
-
     function votingDelay() public view override returns (uint256) {
         return _votingDelay;
     }

contracts/mocks/GovernorMock.sol

@@ -21,8 +21,6 @@ contract GovernorMock is Governor, GovernorVotesQuorumFraction, GovernorCounting
         _votingPeriod = votingPeriod_;
     }
 
-    receive() external payable {}
-
     function votingDelay() public view override returns (uint256) {
         return _votingDelay;
     }

contracts/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@openzeppelin/contracts",
   "description": "Secure Smart Contract library for Solidity",
-  "version": "4.3.0",
+  "version": "4.3.2",
   "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,16 +17,32 @@ 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`.
      *
      * Calls {_authorizeUpgrade}.
      *
      * 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/utils/cryptography/SignatureChecker.sol

@@ -8,7 +8,7 @@ import "../../interfaces/IERC1271.sol";
 
 /**
  * @dev Signature verification helper: Provide a single mechanism to verify both private-key (EOA) ECDSA signature and
- * ERC1271 contract sigantures. Using this instead of ECDSA.recover in your contract will make them compatible with
+ * 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

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