Include EIP-5267 discovery in EIP-712 (#3969)

Co-authored-by: Francisco <frangio.1@gmail.com> Co-authored-by: Francisco <fg@frang.io>
OpenZeppelin · Feb 8, 2023 · d625cb4 · d625cb4
1 parent 8177c46
commit d625cb4
Show file tree

Hide file tree

Showing 16 changed files with 358 additions and 325 deletions.
diff --git a/.changeset/short-roses-judge.md b/.changeset/short-roses-judge.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`EIP712`: add EIP-5267 support for better domain discovery.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+### Breaking changes
+
+- `EIP712`: Addition of ERC5267 support requires support for user defined value types, which was released in Solidity version 0.8.8. This requires a pragma change from `^0.8.0` to `^0.8.8`.
+- `EIP712`: Optimization of the cache for the upgradeable version affects the way `name` and `version` are set. This is no longer done through an initializer, and is instead part of the implementation's constructor. As a consequence, all proxies using the same implementation will necessarily share the same `name` and `version`. Additionally, an implementation upgrade risks changing the EIP712 domain unless the same `name` and `version` are used when deploying the new implementation contract.
+
 ### Deprecations
 
 - `ERC20Permit`: Added the file `IERC20Permit.sol` and `ERC20Permit.sol` and deprecated `draft-IERC20Permit.sol` and `draft-ERC20Permit.sol` since [EIP-2612](https://eips.ethereum.org/EIPS/eip-2612) is no longer a Draft. Developers are encouraged to update their imports. ([#3793](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3793))

diff --git a/contracts/interfaces/IERC5267.sol b/contracts/interfaces/IERC5267.sol
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.0;
+
+interface IERC5267 {
+    /**
+     * @dev MAY be emitted to signal that the domain could have changed.
+     */
+    event EIP712DomainChanged();
+
+    /**
+     * @dev returns the fields and values that describe the domain separator used by this contract for EIP-712
+     * signature.
+     */
+    function eip712Domain()
+        external
+        view
+        returns (
+            bytes1 fields,
+            string memory name,
+            string memory version,
+            uint256 chainId,
+            address verifyingContract,
+            bytes32 salt,
+            uint256[] memory extensions
+        );
+}
diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol
@@ -1,9 +1,11 @@
 // SPDX-License-Identifier: MIT
 // OpenZeppelin Contracts (last updated v4.8.0) (utils/cryptography/EIP712.sol)
 
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.8;
 
 import "./ECDSA.sol";
+import "../ShortStrings.sol";
+import "../../interfaces/IERC5267.sol";
 
 /**
  * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data.
@@ -22,19 +24,34 @@ import "./ECDSA.sol";
  * NOTE: This contract implements the version of the encoding known as "v4", as implemented by the JSON RPC method
  * https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask].
  *
+ * NOTE: In the upgradeable version of this contract, the cached values will correspond to the address, and the domain
+ * separator of the implementation contract. This will cause the `_domainSeparatorV4` function to always rebuild the
+ * separator from the immutable values, which is cheaper than accessing a cached version in cold storage.
+ *
  * _Available since v3.4._
+ *
+ * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
  */
-abstract contract EIP712 {
+abstract contract EIP712 is IERC5267 {
+    using ShortStrings for *;
+
+    bytes32 private constant _TYPE_HASH =
+        keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
+
     /* solhint-disable var-name-mixedcase */
     // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to
     // invalidate the cached domain separator if the chain id changes.
-    bytes32 private immutable _CACHED_DOMAIN_SEPARATOR;
-    uint256 private immutable _CACHED_CHAIN_ID;
-    address private immutable _CACHED_THIS;
+    bytes32 private immutable _cachedDomainSeparator;
+    uint256 private immutable _cachedChainId;
+    address private immutable _cachedThis;
+
+    ShortString private immutable _name;
+    ShortString private immutable _version;
+    string private _nameFallback;
+    string private _versionFallback;
 
-    bytes32 private immutable _HASHED_NAME;
-    bytes32 private immutable _HASHED_VERSION;
-    bytes32 private immutable _TYPE_HASH;
+    bytes32 private immutable _hashedName;
+    bytes32 private immutable _hashedVersion;
 
     /* solhint-enable var-name-mixedcase */
 
@@ -51,36 +68,29 @@ abstract contract EIP712 {
      * contract upgrade].
      */
     constructor(string memory name, string memory version) {
-        bytes32 hashedName = keccak256(bytes(name));
-        bytes32 hashedVersion = keccak256(bytes(version));
-        bytes32 typeHash = keccak256(
-            "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
-        );
-        _HASHED_NAME = hashedName;
-        _HASHED_VERSION = hashedVersion;
-        _CACHED_CHAIN_ID = block.chainid;
-        _CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(typeHash, hashedName, hashedVersion);
-        _CACHED_THIS = address(this);
-        _TYPE_HASH = typeHash;
+        _name = name.toShortStringWithFallback(_nameFallback);
+        _version = version.toShortStringWithFallback(_versionFallback);
+        _hashedName = keccak256(bytes(name));
+        _hashedVersion = keccak256(bytes(version));
+
+        _cachedChainId = block.chainid;
+        _cachedDomainSeparator = _buildDomainSeparator();
+        _cachedThis = address(this);
     }
 
     /**
      * @dev Returns the domain separator for the current chain.
      */
     function _domainSeparatorV4() internal view returns (bytes32) {
-        if (address(this) == _CACHED_THIS && block.chainid == _CACHED_CHAIN_ID) {
-            return _CACHED_DOMAIN_SEPARATOR;
+        if (address(this) == _cachedThis && block.chainid == _cachedChainId) {
+            return _cachedDomainSeparator;
         } else {
-            return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION);
+            return _buildDomainSeparator();
         }
     }
 
-    function _buildDomainSeparator(
-        bytes32 typeHash,
-        bytes32 nameHash,
-        bytes32 versionHash
-    ) private view returns (bytes32) {
-        return keccak256(abi.encode(typeHash, nameHash, versionHash, block.chainid, address(this)));
+    function _buildDomainSeparator() private view returns (bytes32) {
+        return keccak256(abi.encode(_TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this)));
     }
 
     /**
@@ -101,4 +111,33 @@ abstract contract EIP712 {
     function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) {
         return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash);
     }
+
+    /**
+     * @dev See {EIP-5267}.
+     */
+    function eip712Domain()
+        public
+        view
+        virtual
+        override
+        returns (
+            bytes1 fields,
+            string memory name,
+            string memory version,
+            uint256 chainId,
+            address verifyingContract,
+            bytes32 salt,
+            uint256[] memory extensions
+        )
+    {
+        return (
+            hex"0f", // 01111
+            _name.toStringWithFallback(_nameFallback),
+            _version.toStringWithFallback(_versionFallback),
+            block.chainid,
+            address(this),
+            bytes32(0),
+            new uint256[](0)
+        );
+    }
 }
diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js
@@ -4,7 +4,7 @@ const ethSigUtil = require('eth-sig-util');
 const Wallet = require('ethereumjs-wallet').default;
 const { fromRpcSig } = require('ethereumjs-util');
 const Enums = require('../helpers/enums');
-const { EIP712Domain } = require('../helpers/eip712');
+const { getDomain, domainType } = require('../helpers/eip712');
 const { GovernorHelper } = require('../helpers/governance');
 
 const { shouldSupportInterfaces } = require('../utils/introspection/SupportsInterface.behavior');
@@ -20,7 +20,6 @@ contract('Governor', function (accounts) {
   const empty = web3.utils.toChecksumAddress(web3.utils.randomHex(20));
 
   const name = 'OZ-Governor';
-  const version = '1';
   const tokenName = 'MockToken';
   const tokenSymbol = 'MTKN';
   const tokenSupply = web3.utils.toWei('100');
@@ -148,24 +147,22 @@ contract('Governor', function (accounts) {
     const voterBySig = Wallet.generate();
     const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
 
-    const signature = async message => {
-      return fromRpcSig(
-        ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), {
-          data: {
-            types: {
-              EIP712Domain,
-              Ballot: [
-                { name: 'proposalId', type: 'uint256' },
-                { name: 'support', type: 'uint8' },
-              ],
-            },
-            domain: { name, version, chainId: this.chainId, verifyingContract: this.mock.address },
-            primaryType: 'Ballot',
-            message,
+    const signature = (contract, message) =>
+      getDomain(contract)
+        .then(domain => ({
+          primaryType: 'Ballot',
+          types: {
+            EIP712Domain: domainType(domain),
+            Ballot: [
+              { name: 'proposalId', type: 'uint256' },
+              { name: 'support', type: 'uint8' },
+            ],
           },
-        }),
-      );
-    };
+          domain,
+          message,
+        }))
+        .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data }))
+        .then(fromRpcSig);
 
     await this.token.delegate(voterBySigAddress, { from: voter1 });
 

diff --git a/test/governance/extensions/GovernorWithParams.test.js b/test/governance/extensions/GovernorWithParams.test.js
@@ -4,7 +4,7 @@ const ethSigUtil = require('eth-sig-util');
 const Wallet = require('ethereumjs-wallet').default;
 const { fromRpcSig } = require('ethereumjs-util');
 const Enums = require('../../helpers/enums');
-const { EIP712Domain } = require('../../helpers/eip712');
+const { getDomain, domainType } = require('../../helpers/eip712');
 const { GovernorHelper } = require('../../helpers/governance');
 
 const Token = artifacts.require('$ERC20VotesComp');
@@ -22,7 +22,6 @@ contract('GovernorWithParams', function (accounts) {
   const [owner, proposer, voter1, voter2, voter3, voter4] = accounts;
 
   const name = 'OZ-Governor';
-  const version = '1';
   const tokenName = 'MockToken';
   const tokenSymbol = 'MTKN';
   const tokenSupply = web3.utils.toWei('100');
@@ -116,26 +115,24 @@ contract('GovernorWithParams', function (accounts) {
     const voterBySig = Wallet.generate();
     const voterBySigAddress = web3.utils.toChecksumAddress(voterBySig.getAddressString());
 
-    const signature = async message => {
-      return fromRpcSig(
-        ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), {
-          data: {
-            types: {
-              EIP712Domain,
-              ExtendedBallot: [
-                { name: 'proposalId', type: 'uint256' },
-                { name: 'support', type: 'uint8' },
-                { name: 'reason', type: 'string' },
-                { name: 'params', type: 'bytes' },
-              ],
-            },
-            domain: { name, version, chainId: this.chainId, verifyingContract: this.mock.address },
-            primaryType: 'ExtendedBallot',
-            message,
+    const signature = (contract, message) =>
+      getDomain(contract)
+        .then(domain => ({
+          primaryType: 'ExtendedBallot',
+          types: {
+            EIP712Domain: domainType(domain),
+            ExtendedBallot: [
+              { name: 'proposalId', type: 'uint256' },
+              { name: 'support', type: 'uint8' },
+              { name: 'reason', type: 'string' },
+              { name: 'params', type: 'bytes' },
+            ],
           },
-        }),
-      );
-    };
+          domain,
+          message,
+        }))
+        .then(data => ethSigUtil.signTypedMessage(voterBySig.getPrivateKey(), { data }))
+        .then(fromRpcSig);
 
     await this.token.delegate(voterBySigAddress, { from: voter2 });