From d8a5ee57d47d1349c2f30b3fb1327d059316621c Mon Sep 17 00:00:00 2001 From: FHieser Date: Wed, 6 Mar 2024 15:47:46 +0100 Subject: [PATCH 1/4] Create digest creation function --- .../forwarder/TransactionForwarder.sol | 37 +++++ test/e2e/module/ForwarderSignatureHelper.sol | 131 ------------------ test/e2e/module/MetaTxAndMulticallE2E.t.sol | 51 ++++--- 3 files changed, 68 insertions(+), 151 deletions(-) delete mode 100644 test/e2e/module/ForwarderSignatureHelper.sol diff --git a/src/external/forwarder/TransactionForwarder.sol b/src/external/forwarder/TransactionForwarder.sol index fda540031..706c10edf 100644 --- a/src/external/forwarder/TransactionForwarder.sol +++ b/src/external/forwarder/TransactionForwarder.sol @@ -22,6 +22,21 @@ contract TransactionForwarder is // Constructor constructor(string memory name) ERC2771Forwarder(name) {} + //-------------------------------------------------------------------------- + // Metatransaction Helper Functions + + /// @notice Creates a digest for the given ForwardRequestData + /// @dev The signature field of the given ForwardRequestData can be empty + /// @param req The ForwardRequest you want to get the digest from + /// @return digest The digest needed to create a signature for the request + function createDigest(ERC2771Forwarder.ForwardRequestData memory req) + external + view + returns (bytes32 digest) + { + return _hashTypedDataV4(getStructHash(req)); + } + //-------------------------------------------------------------------------- // Multicall Functions @@ -61,6 +76,28 @@ contract TransactionForwarder is } } + //-------------------------------------------------------------------------- + // Internal + + function getStructHash(ERC2771Forwarder.ForwardRequestData memory req) + internal + view + returns (bytes32) + { + return keccak256( + abi.encode( + _FORWARD_REQUEST_TYPEHASH, + req.from, + req.to, + req.value, + req.gas, + nonces(req.from), + req.deadline, + keccak256(req.data) + ) + ); + } + // Copied from the ERC2771Forwarder as it isnt declared internal ಠ_ಠ // Just added a _ because i cant override it /** diff --git a/test/e2e/module/ForwarderSignatureHelper.sol b/test/e2e/module/ForwarderSignatureHelper.sol deleted file mode 100644 index ca99043fd..000000000 --- a/test/e2e/module/ForwarderSignatureHelper.sol +++ /dev/null @@ -1,131 +0,0 @@ -// SPDX-License-Identifier: LGPL-3.0-only -pragma solidity ^0.8.0; - -import "forge-std/Test.sol"; - -//Internal Dependencies -import { - TransactionForwarder, - ERC2771Forwarder -} from "src/external/forwarder/TransactionForwarder.sol"; - -// External Dependencies -import {Nonces} from "@oz/utils/Nonces.sol"; - -contract ForwarderSignatureHelper is Nonces, Test { - address private immutable forwarder; - bytes32 private immutable _CACHED_DOMAIN_SEPARATOR; - uint private immutable _CACHED_CHAIN_ID; - - bytes32 private immutable _HASHED_NAME; - bytes32 private immutable _HASHED_VERSION; - bytes32 private immutable _TYPE_HASH; - bytes32 private immutable _FORWARD_REQUEST_TYPEHASH; - - //Domain Seperator - constructor(address _forwarder) { - forwarder = _forwarder; - string memory name = "TransactionForwarder"; - string memory version = "1"; - bytes32 hashedName = keccak256(bytes(name)); - bytes32 hashedVersion = keccak256(bytes(version)); - bytes32 typeHash = keccak256( - "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" - ); - bytes32 forwardRequestTypehash = keccak256( - "ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)" - ); - - _HASHED_NAME = hashedName; - _HASHED_VERSION = hashedVersion; - _CACHED_CHAIN_ID = block.chainid; - _CACHED_DOMAIN_SEPARATOR = - _buildDomainSeparator(typeHash, hashedName, hashedVersion); - _TYPE_HASH = typeHash; - _FORWARD_REQUEST_TYPEHASH = forwardRequestTypehash; - } - - function _buildDomainSeparator( - bytes32 typeHash, - bytes32 nameHash, - bytes32 versionHash - ) private view returns (bytes32) { - return keccak256( - abi.encode( - typeHash, nameHash, versionHash, block.chainid, forwarder - ) - ); - } - - struct HelperForwardRequest { - address from; - address to; - uint value; - uint gas; - uint48 deadline; - bytes data; - } - - // computes the hash of a permit - function getStructHash(HelperForwardRequest memory req) - internal - view - returns (bytes32) - { - return keccak256( - abi.encode( - _FORWARD_REQUEST_TYPEHASH, - req.from, - req.to, - req.value, - req.gas, - nonces(req.from), - req.deadline, - keccak256(req.data) - ) - ); - } - - //Copied from Openzeppelins MessageHashUtils - function toTypedDataHash(bytes32 domainSeparator, bytes32 structHash) - internal - pure - returns (bytes32 digest) - { - assembly { - let ptr := mload(0x40) - mstore(ptr, hex"1901") - mstore(add(ptr, 0x02), domainSeparator) - mstore(add(ptr, 0x22), structHash) - digest := keccak256(ptr, 0x42) - } - } - - function getForwardRequestData( - HelperForwardRequest memory req, - address signer, - uint signerPrivateKey - ) public returns (ERC2771Forwarder.ForwardRequestData memory) { - //Get digest for signature creation - bytes32 digest = - toTypedDataHash(_CACHED_DOMAIN_SEPARATOR, getStructHash(req)); - - //Create Signature - vm.prank(signer); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); - bytes memory signature = abi.encodePacked(r, s, v); - - //Make sure the nonce is counted up correctly - _useNonce(signer); - - return ERC2771Forwarder.ForwardRequestData( - req.from, - req.to, - req.value, - req.gas, - req.deadline, - req.data, - signature - ); - } -} diff --git a/test/e2e/module/MetaTxAndMulticallE2E.t.sol b/test/e2e/module/MetaTxAndMulticallE2E.t.sol index 1e0114cdd..f3c647f8c 100644 --- a/test/e2e/module/MetaTxAndMulticallE2E.t.sol +++ b/test/e2e/module/MetaTxAndMulticallE2E.t.sol @@ -3,8 +3,6 @@ pragma solidity ^0.8.0; import "forge-std/console.sol"; -import {ForwarderSignatureHelper} from "./ForwarderSignatureHelper.sol"; - // SuT import {RoleAuthorizer} from "src/modules/authorizer/RoleAuthorizer.sol"; @@ -22,7 +20,8 @@ import { } from "src/modules/logicModule/BountyManager.sol"; import { TransactionForwarder, - ITransactionForwarder + ITransactionForwarder, + ERC2771Forwarder } from "src/external/forwarder/TransactionForwarder.sol"; contract MetaTxAndMulticallE2E is E2ETest { @@ -121,13 +120,9 @@ contract MetaTxAndMulticallE2E is E2ETest { vm.prank(signer); token.approve(fundingManager, depositAmount); - //Because the creation of the signature and the ForwardRequestData is kind of messy I created a Helper that handles that - ForwarderSignatureHelper signatureHelper = - new ForwarderSignatureHelper(address(forwarder)); - //We create a simplyfied ForwardRequest without the signature - ForwarderSignatureHelper.HelperForwardRequest memory req = - ForwarderSignatureHelper.HelperForwardRequest({ + ERC2771Forwarder.ForwardRequestData memory req = ERC2771Forwarder + .ForwardRequestData({ from: signer, to: fundingManager, value: 0, @@ -135,15 +130,23 @@ contract MetaTxAndMulticallE2E is E2ETest { gas: 1_000_000, //This is the timestamp after which the request is not executable anymore. deadline: uint48(block.timestamp + 1 weeks), - data: abi.encodeWithSignature("deposit(uint256)", depositAmount) + data: abi.encodeWithSignature("deposit(uint256)", depositAmount), + //This has to be empty until we create the signature + signature: bytes("") }); - //We use the helper to create the sinature - TransactionForwarder.ForwardRequestData memory finalReq = - signatureHelper.getForwardRequestData(req, signer, signerPrivateKey); + //Create the digest needed to create the signature + bytes32 digest = forwarder.createDigest(req); + + //Create Signature with digest (This has to be handled by the frontend) + vm.prank(signer); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + req.signature = signature; //Do call - forwarder.execute(finalReq); + forwarder.execute(req); //Check if successful assertEq( @@ -178,7 +181,7 @@ contract MetaTxAndMulticallE2E is E2ETest { ); //Then we need to create the ForwardRequest - req = ForwarderSignatureHelper.HelperForwardRequest({ + req = ERC2771Forwarder.ForwardRequestData({ from: signer, to: address(bountyManager), value: 0, @@ -190,15 +193,23 @@ contract MetaTxAndMulticallE2E is E2ETest { 100e18, //minimumPayoutAmount 500e18, //maximumPayoutAmount bytes("This is a test bounty") //details - ) + ), + //This has to be empty until we create the signature + signature: bytes("") }); - //We use the helper to create the sinature - finalReq = - signatureHelper.getForwardRequestData(req, signer, signerPrivateKey); + //Create the digest needed to create the signature + digest = forwarder.createDigest(req); + + //Create Signature with digest (This has to be handled by the frontend) + vm.prank(signer); + (v, r, s) = vm.sign(signerPrivateKey, digest); + signature = abi.encodePacked(r, s, v); + + req.signature = signature; //Do call - forwarder.execute(finalReq); + forwarder.execute(req); //Check if successful assertTrue(bountyManager.isExistingBountyId(1)); From 9ce58ad012e20bb559bb081c3ab536b9fc953a72 Mon Sep 17 00:00:00 2001 From: FHieser Date: Wed, 6 Mar 2024 16:15:45 +0100 Subject: [PATCH 2/4] Cleanup --- src/external/forwarder/TransactionForwarder.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/external/forwarder/TransactionForwarder.sol b/src/external/forwarder/TransactionForwarder.sol index 706c10edf..fcb283177 100644 --- a/src/external/forwarder/TransactionForwarder.sol +++ b/src/external/forwarder/TransactionForwarder.sol @@ -29,7 +29,7 @@ contract TransactionForwarder is /// @dev The signature field of the given ForwardRequestData can be empty /// @param req The ForwardRequest you want to get the digest from /// @return digest The digest needed to create a signature for the request - function createDigest(ERC2771Forwarder.ForwardRequestData memory req) + function createDigest(ForwardRequestData memory req) external view returns (bytes32 digest) From 9bc916d3cc55bed61b896428390c49a0d410d51c Mon Sep 17 00:00:00 2001 From: FHieser Date: Wed, 6 Mar 2024 16:22:33 +0100 Subject: [PATCH 3/4] Add getDigest Test --- test/external/TransactionForwarder.t.sol | 41 ++++++++++++++++++- .../TransactionForwarderAccessMock.sol | 22 ++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 test/utils/mocks/external/TransactionForwarderAccessMock.sol diff --git a/test/external/TransactionForwarder.t.sol b/test/external/TransactionForwarder.t.sol index 19feec81f..8a3bdc5eb 100644 --- a/test/external/TransactionForwarder.t.sol +++ b/test/external/TransactionForwarder.t.sol @@ -10,16 +10,53 @@ import { ERC2771Forwarder } from "src/external/forwarder/TransactionForwarder.sol"; +import {TransactionForwarderAccessMock} from + "test/utils/mocks/external/TransactionForwarderAccessMock.sol"; + import {CallIntercepter} from "test/utils/mocks/external/CallIntercepter.sol"; contract TransactionForwarderTest is Test { // SuT - TransactionForwarder forwarder; + TransactionForwarderAccessMock forwarder; event CallReceived(address intercepterAddress, bytes data, address sender); function setUp() public { - forwarder = new TransactionForwarder("TransactionForwarder"); + forwarder = new TransactionForwarderAccessMock("TransactionForwarder"); + } + + //-------------------------------------------------------------------------- + // Test: createDigest + + function testCreateDigest( + ERC2771Forwarder.ForwardRequestData memory req, + uint signerPrivateKey + ) public { + //Restrict the signerKey to a space where it still should work + signerPrivateKey = bound(signerPrivateKey, 1, 2 ^ 128); + + //Derive signer from signerkey + address signer = vm.addr(signerPrivateKey); + + //set from in req to be the signer + req.from = signer; + + //Create the digest needed to create the signature + bytes32 digest = forwarder.createDigest(req); + + //Create Signature with digest (This has to be handled by the frontend) + vm.prank(signer); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + //Set correct signature in request + req.signature = signature; + + (,, bool signerMatch, address signerResult) = + forwarder.original_validate(req); + + assertTrue(signerMatch); + assertEq(signer, signerResult); } //-------------------------------------------------------------------------------- diff --git a/test/utils/mocks/external/TransactionForwarderAccessMock.sol b/test/utils/mocks/external/TransactionForwarderAccessMock.sol new file mode 100644 index 000000000..863346dc4 --- /dev/null +++ b/test/utils/mocks/external/TransactionForwarderAccessMock.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity ^0.8.0; + +import {TransactionForwarder} from + "src/external/forwarder/TransactionForwarder.sol"; + +contract TransactionForwarderAccessMock is TransactionForwarder { + constructor(string memory name) TransactionForwarder(name) {} + + function original_validate(ForwardRequestData calldata request) + external + view + returns ( + bool isTrustedForwarder, + bool active, + bool signerMatch, + address signer + ) + { + return _validate(request); + } +} From 4eb06d9377de23404e7ae074b86fb7946db78c37 Mon Sep 17 00:00:00 2001 From: FHieser Date: Tue, 12 Mar 2024 09:33:39 +0100 Subject: [PATCH 4/4] add _ to getStructHash() --- src/external/forwarder/TransactionForwarder.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/external/forwarder/TransactionForwarder.sol b/src/external/forwarder/TransactionForwarder.sol index fcb283177..b60ee592e 100644 --- a/src/external/forwarder/TransactionForwarder.sol +++ b/src/external/forwarder/TransactionForwarder.sol @@ -34,7 +34,7 @@ contract TransactionForwarder is view returns (bytes32 digest) { - return _hashTypedDataV4(getStructHash(req)); + return _hashTypedDataV4(_getStructHash(req)); } //-------------------------------------------------------------------------- @@ -79,7 +79,7 @@ contract TransactionForwarder is //-------------------------------------------------------------------------- // Internal - function getStructHash(ERC2771Forwarder.ForwardRequestData memory req) + function _getStructHash(ERC2771Forwarder.ForwardRequestData memory req) internal view returns (bytes32)