From 0c4f2533279c0f84bbd3ec2213bf59b2bd4b05e7 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Wed, 21 Sep 2022 12:26:28 +0530 Subject: [PATCH 1/7] caching system : re-calculated domain separator by comparing the current chain id and earlier chain id --- contracts/domain/BosonConstants.sol | 2 ++ contracts/protocol/facets/ConfigHandlerFacet.sol | 4 +++- contracts/protocol/libs/EIP712Lib.sol | 13 ++++++++++--- contracts/protocol/libs/ProtocolLib.sol | 4 ++++ 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/contracts/domain/BosonConstants.sol b/contracts/domain/BosonConstants.sol index 2512c2bab..9b6dd251f 100644 --- a/contracts/domain/BosonConstants.sol +++ b/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 = "BosonProtocolDiamond"; +string constant PROTOCOL_VERSION = "V1"; bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256( bytes("EIP712Domain(string name,string version,address verifyingContract,bytes32 salt)") ); diff --git a/contracts/protocol/facets/ConfigHandlerFacet.sol b/contracts/protocol/facets/ConfigHandlerFacet.sol index a39269a1b..0d7c6c0e5 100644 --- a/contracts/protocol/facets/ConfigHandlerFacet.sol +++ b/contracts/protocol/facets/ConfigHandlerFacet.sol @@ -69,7 +69,9 @@ 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.domainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + pmti.cachedChainId = block.chainid; + pmti.cachedThis = address(this); } /** diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index e03b0b50e..cde087fe4 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -64,12 +64,19 @@ 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; + address cachedThis = ProtocolLib.protocolMetaTxInfo().cachedThis; + uint256 cachedChainId = ProtocolLib.protocolMetaTxInfo().cachedChainId; + + if (address(this) == cachedThis && block.chainid == cachedChainId) { + return ProtocolLib.protocolMetaTxInfo().domainSeparator; + } else { + return domainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + } } /** diff --git a/contracts/protocol/libs/ProtocolLib.sol b/contracts/protocol/libs/ProtocolLib.sol index f832c10b8..0f40bc3ac 100644 --- a/contracts/protocol/libs/ProtocolLib.sol +++ b/contracts/protocol/libs/ProtocolLib.sol @@ -202,6 +202,10 @@ library ProtocolLib { bytes32 domainSeparator; // address => nonce => nonce used indicator mapping(address => mapping(uint256 => bool)) usedNonce; + // The cached chain id + uint256 cachedChainId; + // The cached address of the diamond + address cachedThis; // map function name to input type mapping(string => BosonTypes.MetaTxInputType) inputType; // map input type => hash info From 25ef20d85344d039d4cf8f3508c2a78904d43ee1 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Wed, 21 Sep 2022 15:42:50 +0530 Subject: [PATCH 2/7] unit test --- contracts/domain/BosonConstants.sol | 4 +- .../mock/MockMetaTransactionsHandlerFacet.sol | 9 +++ .../protocol/facets/ConfigHandlerFacet.sol | 2 +- contracts/protocol/libs/EIP712Lib.sol | 4 +- scripts/util/test-utils.js | 4 +- test/protocol/MetaTransactionsHandlerTest.js | 81 ++++++++++++++++++- 6 files changed, 96 insertions(+), 8 deletions(-) diff --git a/contracts/domain/BosonConstants.sol b/contracts/domain/BosonConstants.sol index 9b6dd251f..31664c4b9 100644 --- a/contracts/domain/BosonConstants.sol +++ b/contracts/domain/BosonConstants.sol @@ -159,8 +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 = "BosonProtocolDiamond"; -string constant PROTOCOL_VERSION = "V1"; +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)") ); diff --git a/contracts/mock/MockMetaTransactionsHandlerFacet.sol b/contracts/mock/MockMetaTransactionsHandlerFacet.sol index f85d7193e..ad11df563 100644 --- a/contracts/mock/MockMetaTransactionsHandlerFacet.sol +++ b/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; + } } diff --git a/contracts/protocol/facets/ConfigHandlerFacet.sol b/contracts/protocol/facets/ConfigHandlerFacet.sol index 0d7c6c0e5..4aa2966d5 100644 --- a/contracts/protocol/facets/ConfigHandlerFacet.sol +++ b/contracts/protocol/facets/ConfigHandlerFacet.sol @@ -69,7 +69,7 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase { // Initialize protocol meta-transaction config params ProtocolLib.ProtocolMetaTxInfo storage pmti = protocolMetaTxInfo(); - pmti.domainSeparator = EIP712Lib.domainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + pmti.domainSeparator = EIP712Lib.buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); pmti.cachedChainId = block.chainid; pmti.cachedThis = address(this); } diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index cde087fe4..0c34ae8d2 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -17,7 +17,7 @@ library EIP712Lib { * @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( @@ -75,7 +75,7 @@ library EIP712Lib { if (address(this) == cachedThis && block.chainid == cachedChainId) { return ProtocolLib.protocolMetaTxInfo().domainSeparator; } else { - return domainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + return buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); } } diff --git a/scripts/util/test-utils.js b/scripts/util/test-utils.js index b4301f0d9..698a1790b 100644 --- a/scripts/util/test-utils.js +++ b/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 }; diff --git a/test/protocol/MetaTransactionsHandlerTest.js b/test/protocol/MetaTransactionsHandlerTest.js index 0b545d75b..40a1e52fb 100644 --- a/test/protocol/MetaTransactionsHandlerTest.js +++ b/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(nonce); + assert.equal(result, expectedResult, "Nonce is unused"); + }); + it("does not modify revert reasons", async function () { // Set seller as inactive seller.active = false; From 59cd7ddb63f707b8c4e5ea3a86da3812ab2d8a93 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Fri, 23 Sep 2022 15:02:01 +0530 Subject: [PATCH 3/7] review fixes --- contracts/protocol/facets/ConfigHandlerFacet.sol | 1 - contracts/protocol/libs/EIP712Lib.sol | 15 +++++++++------ contracts/protocol/libs/ProtocolLib.sol | 2 -- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/protocol/facets/ConfigHandlerFacet.sol b/contracts/protocol/facets/ConfigHandlerFacet.sol index 4aa2966d5..a3af6a177 100644 --- a/contracts/protocol/facets/ConfigHandlerFacet.sol +++ b/contracts/protocol/facets/ConfigHandlerFacet.sol @@ -71,7 +71,6 @@ contract ConfigHandlerFacet is IBosonConfigHandler, ProtocolBase { ProtocolLib.ProtocolMetaTxInfo storage pmti = protocolMetaTxInfo(); pmti.domainSeparator = EIP712Lib.buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); pmti.cachedChainId = block.chainid; - pmti.cachedThis = address(this); } /** diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index 0c34ae8d2..ae1e214c1 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -12,6 +12,8 @@ import { ProtocolLib } from "../libs/ProtocolLib.sol"; library EIP712Lib { /** * @notice Generates the domain separator hash. + * @dev Using chainId as the salt allows you that your client is active on one chain, + * while you sign metaTx for another chain. * * @param _name - the name of the protocol * @param _version - The version of the protocol @@ -49,7 +51,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( @@ -68,13 +70,14 @@ library EIP712Lib { * * @return the domain separator */ - function getDomainSeparator() private view returns (bytes32) { - address cachedThis = ProtocolLib.protocolMetaTxInfo().cachedThis; - uint256 cachedChainId = ProtocolLib.protocolMetaTxInfo().cachedChainId; + function getDomainSeparator() private returns (bytes32) { + ProtocolLib.ProtocolMetaTxInfo storage pmti = ProtocolLib.protocolMetaTxInfo(); + uint256 cachedChainId = pmti.cachedChainId; - if (address(this) == cachedThis && block.chainid == cachedChainId) { + if (block.chainid == cachedChainId) { return ProtocolLib.protocolMetaTxInfo().domainSeparator; } else { + pmti.cachedChainId = block.chainid; return buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); } } @@ -91,7 +94,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)); } diff --git a/contracts/protocol/libs/ProtocolLib.sol b/contracts/protocol/libs/ProtocolLib.sol index 0f40bc3ac..220de923f 100644 --- a/contracts/protocol/libs/ProtocolLib.sol +++ b/contracts/protocol/libs/ProtocolLib.sol @@ -204,8 +204,6 @@ library ProtocolLib { mapping(address => mapping(uint256 => bool)) usedNonce; // The cached chain id uint256 cachedChainId; - // The cached address of the diamond - address cachedThis; // map function name to input type mapping(string => BosonTypes.MetaTxInputType) inputType; // map input type => hash info From da2160bca6c42c3cfb670eb5e03135357993a0b6 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Fri, 23 Sep 2022 16:13:06 +0530 Subject: [PATCH 4/7] fixed breaking test --- test/protocol/MetaTransactionsHandlerTest.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/protocol/MetaTransactionsHandlerTest.js b/test/protocol/MetaTransactionsHandlerTest.js index 40a1e52fb..02d448e21 100644 --- a/test/protocol/MetaTransactionsHandlerTest.js +++ b/test/protocol/MetaTransactionsHandlerTest.js @@ -497,7 +497,7 @@ describe("IBosonMetaTransactionsHandler", function () { // Verify that nonce is used. Expect true. let expectedResult = true; - result = await metaTransactionsHandler.connect(operator).isUsedNonce(nonce); + result = await metaTransactionsHandler.connect(operator).isUsedNonce(deployer.address, nonce); assert.equal(result, expectedResult, "Nonce is unused"); }); From 8fd5fa3b635484490da36877f6e0ad7139190cdf Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Fri, 23 Sep 2022 16:48:10 +0530 Subject: [PATCH 5/7] review fix --- contracts/protocol/libs/EIP712Lib.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index ae1e214c1..6018fa3f5 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -12,8 +12,9 @@ import { ProtocolLib } from "../libs/ProtocolLib.sol"; library EIP712Lib { /** * @notice Generates the domain separator hash. - * @dev Using chainId as the salt allows you that your client is active on one chain, - * while you sign metaTx for another chain. + * @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 From 0decd8a88389e703afb6268a44d9447da6364a01 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Fri, 23 Sep 2022 17:09:07 +0530 Subject: [PATCH 6/7] review fix --- contracts/protocol/libs/EIP712Lib.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index 6018fa3f5..9e7d5b009 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -78,8 +78,11 @@ library EIP712Lib { if (block.chainid == cachedChainId) { return ProtocolLib.protocolMetaTxInfo().domainSeparator; } else { + bytes32 domainSeparator = buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + pmti.domainSeparator = domainSeparator; pmti.cachedChainId = block.chainid; - return buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); + + return domainSeparator; } } From f0dac2113f433d3917039c6f5de73d9982dbdde4 Mon Sep 17 00:00:00 2001 From: bidyut-arianelabs Date: Fri, 23 Sep 2022 17:24:31 +0530 Subject: [PATCH 7/7] review fix --- contracts/protocol/libs/EIP712Lib.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/protocol/libs/EIP712Lib.sol b/contracts/protocol/libs/EIP712Lib.sol index 9e7d5b009..5b889fb7f 100644 --- a/contracts/protocol/libs/EIP712Lib.sol +++ b/contracts/protocol/libs/EIP712Lib.sol @@ -76,7 +76,7 @@ library EIP712Lib { uint256 cachedChainId = pmti.cachedChainId; if (block.chainid == cachedChainId) { - return ProtocolLib.protocolMetaTxInfo().domainSeparator; + return pmti.domainSeparator; } else { bytes32 domainSeparator = buildDomainSeparator(PROTOCOL_NAME, PROTOCOL_VERSION); pmti.domainSeparator = domainSeparator;