EIP-02M: Remediate Improper Domain Separator Retrieval (#402)

* caching system : re-calculated domain separator by comparing the current chain id and earlier chain id

* unit test

* review fixes

* fixed breaking test

* review fix

* review fix

* review fix

contracts/domain/BosonConstants.sol

@@ -159,6 +159,8 @@ string constant FEE_PERCENTAGE_INVALID = "Percentage representation must be less
 string constant VALUE_ZERO_NOT_ALLOWED = "Value must be greater than 0";
 
 // EIP712Lib
+string constant PROTOCOL_NAME = "Boson Protocol";
+string constant PROTOCOL_VERSION = "V2";
 bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256(
     bytes("EIP712Domain(string name,string version,address verifyingContract,bytes32 salt)")
 );

contracts/mock/MockMetaTransactionsHandlerFacet.sol

@@ -18,4 +18,13 @@ contract MockMetaTransactionsHandlerFacet is ProtocolBase {
         protocolMetaTxInfo().isMetaTransaction = true;
         protocolMetaTxInfo().currentSenderAddress = _signerAddress;
     }
+
+    /**
+     * @notice Sets the cached chain id value.
+     *
+     * @param _chainId - chain id
+     */
+    function setCachedChainId(uint256 _chainId) public {
+        protocolMetaTxInfo().cachedChainId = _chainId;
+    }
 }

contracts/protocol/facets/ConfigHandlerFacet.sol

@@ -69,7 +69,8 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase {
 
         // Initialize protocol meta-transaction config params
         ProtocolLib.ProtocolMetaTxInfo storage pmti = protocolMetaTxInfo();
-        pmti.domainSeparator = EIP712Lib.domainSeparator("BosonProtocolDiamond", "V1");
+        pmti.domainSeparator = EIP712Lib.buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION);
+        pmti.cachedChainId = block.chainid;
     }
 
     /**

contracts/protocol/libs/EIP712Lib.sol

@@ -12,12 +12,15 @@ import { ProtocolLib } from "../libs/ProtocolLib.sol";
 library EIP712Lib {
     /**
      * @notice Generates the domain separator hash.
+     * @dev Using the chainId as the salt enables the client to be active on one chain
+     * while a metatx is signed for a contract on another chain. That could happen if the client is,
+     * for instance, a metaverse scene that runs on one chain while the contracts it interacts with are deployed on another chain.
      *
      * @param _name - the name of the protocol
      * @param _version -  The version of the protocol
      * @return the domain separator hash
      */
-    function domainSeparator(string memory _name, string memory _version) internal view returns (bytes32) {
+    function buildDomainSeparator(string memory _name, string memory _version) internal view returns (bytes32) {
         return
             keccak256(
                 abi.encode(
@@ -49,7 +52,7 @@ library EIP712Lib {
         bytes32 _sigR,
         bytes32 _sigS,
         uint8 _sigV
-    ) internal view returns (bool) {
+    ) internal returns (bool) {
         // Ensure signature is unique
         // See https://github.com/OpenZeppelin/openzeppelin-contracts/blob/04695aecbd4d17dddfd55de766d10e3805d6f42f/contracts/cryptography/ECDSA.sol#63
         require(
@@ -64,12 +67,23 @@ library EIP712Lib {
     }
 
     /**
-     * @notice Gets the domain separator from storage.
+     * @notice Gets the domain separator from storage if matches with the chain id and diamond address, else, build new domain separator.
      *
-     * @return the domain separator from storage
+     * @return the domain separator
      */
-    function getDomainSeparator() private view returns (bytes32) {
-        return ProtocolLib.protocolMetaTxInfo().domainSeparator;
+    function getDomainSeparator() private returns (bytes32) {
+        ProtocolLib.ProtocolMetaTxInfo storage pmti = ProtocolLib.protocolMetaTxInfo();
+        uint256 cachedChainId = pmti.cachedChainId;
+
+        if (block.chainid == cachedChainId) {
+            return pmti.domainSeparator;
+        } else {
+            bytes32 domainSeparator = buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION);
+            pmti.domainSeparator = domainSeparator;
+            pmti.cachedChainId = block.chainid;
+
+            return domainSeparator;
+        }
     }
 
     /**
@@ -84,7 +98,7 @@ library EIP712Lib {
      * @param _messageHash  - the message hash
      * @return the EIP712 compatible message hash
      */
-    function toTypedMessageHash(bytes32 _messageHash) internal view returns (bytes32) {
+    function toTypedMessageHash(bytes32 _messageHash) internal returns (bytes32) {
         return keccak256(abi.encodePacked("\x19\x01", getDomainSeparator(), _messageHash));
     }
 

contracts/protocol/libs/ProtocolLib.sol

@@ -202,6 +202,8 @@ library ProtocolLib {
         bytes32 domainSeparator;
         // address => nonce => nonce used indicator
         mapping(address => mapping(uint256 => bool)) usedNonce;
+        // The cached chain id
+        uint256 cachedChainId;
         // map function name to input type
         mapping(string => BosonTypes.MetaTxInputType) inputType;
         // map input type => hash info

scripts/util/test-utils.js

@@ -120,8 +120,8 @@ async function prepareDataSignatureParameters(
   ];
 
   const domainData = {
-    name: "BosonProtocolDiamond",
-    version: "V1",
+    name: "Boson Protocol",
+    version: "V2",
     verifyingContract: metaTransactionsHandlerAddress,
     salt: ethers.utils.hexZeroPad(ethers.BigNumber.from(31337).toHexString(), 32), //hardhat default chain id is 31337
   };

test/protocol/MetaTransactionsHandlerTest.js

@@ -29,6 +29,8 @@ const {
   mockExchange,
 } = require("../utils/mock");
 const { oneWeek, oneMonth } = require("../utils/constants");
+const { getSelectors, FacetCutAction } = require("../../scripts/util/diamond-utils.js");
+
 /**
  *  Test the Boson Meta transactions Handler interface
  */
@@ -60,7 +62,8 @@ describe("IBosonMetaTransactionsHandler", function () {
     pauseHandler,
     bosonToken,
     support,
-    result;
+    result,
+    mockMetaTransactionsHandler;
   let metaTransactionsHandler, nonce, functionSignature;
   let seller, offerId, buyerId;
   let validOfferDetails,
@@ -231,6 +234,43 @@ describe("IBosonMetaTransactionsHandler", function () {
     [bosonToken, mockToken] = await deployMockTokens(gasLimit, ["BosonToken", "Foreign20"]);
   });
 
+  async function upgradeMetaTransactionsHandlerFacet() {
+    // Upgrade the ExchangeHandlerFacet functions
+    // DiamondCutFacet
+    const cutFacetViaDiamond = await ethers.getContractAt("DiamondCutFacet", protocolDiamond.address);
+
+    // Deploy MockMetaTransactionsHandlerFacet
+    const MockMetaTransactionsHandlerFacet = await ethers.getContractFactory("MockMetaTransactionsHandlerFacet");
+    const mockMetaTransactionsHandlerFacet = await MockMetaTransactionsHandlerFacet.deploy();
+    await mockMetaTransactionsHandlerFacet.deployed();
+
+    // Define the facet cut
+    const facetCuts = [
+      {
+        facetAddress: mockMetaTransactionsHandlerFacet.address,
+        action: FacetCutAction.Add,
+        functionSelectors: getSelectors(mockMetaTransactionsHandlerFacet),
+      },
+    ];
+
+    // Send the DiamondCut transaction
+    const tx = await cutFacetViaDiamond
+      .connect(deployer)
+      .diamondCut(facetCuts, ethers.constants.AddressZero, "0x", { gasLimit });
+
+    // Wait for transaction to confirm
+    const receipt = await tx.wait();
+
+    // Be certain transaction was successful
+    assert.equal(receipt.status, 1, `Diamond upgrade failed: ${tx.hash}`);
+
+    // Cast Diamond to MockMetaTransactionsHandlerFacet
+    mockMetaTransactionsHandler = await ethers.getContractAt(
+      "MockMetaTransactionsHandlerFacet",
+      protocolDiamond.address
+    );
+  }
+
   // Interface support (ERC-156 provided by ProtocolDiamond, others by deployed facets)
   context("📋 Interfaces", async function () {
     context("👉 supportsInterface()", async function () {
@@ -422,6 +462,45 @@ describe("IBosonMetaTransactionsHandler", function () {
           assert.equal(result, expectedResult, "Nonce is unused");
         });
 
+        it("Should build a new domain separator if cachedChainId does not match with chain id used in signature", async function () {
+          await upgradeMetaTransactionsHandlerFacet();
+
+          // update the cached chain id
+          await mockMetaTransactionsHandler.setCachedChainId(123456);
+
+          // Prepare the function signature for the facet function.
+          functionSignature = accountHandler.interface.encodeFunctionData("createSeller", [
+            seller,
+            emptyAuthToken,
+            voucherInitValues,
+          ]);
+
+          message.functionSignature = functionSignature;
+
+          // Collect the signature components
+          let { r, s, v } = await prepareDataSignatureParameters(
+            operator,
+            customTransactionType,
+            "MetaTransaction",
+            message,
+            metaTransactionsHandler.address
+          );
+
+          // send a meta transaction, does not revert
+          await expect(
+            metaTransactionsHandler
+              .connect(deployer)
+              .executeMetaTransaction(operator.address, message.functionName, functionSignature, nonce, r, s, v)
+          )
+            .to.emit(metaTransactionsHandler, "MetaTransactionExecuted")
+            .withArgs(operator.address, deployer.address, message.functionName, nonce);
+
+          // Verify that nonce is used. Expect true.
+          let expectedResult = true;
+          result = await metaTransactionsHandler.connect(operator).isUsedNonce(deployer.address, nonce);
+          assert.equal(result, expectedResult, "Nonce is unused");
+        });
+
         it("does not modify revert reasons", async function () {
           // Set seller as inactive
           seller.active = false;