From eea140d53e2c1841e1801175e54e51cec3c29146 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 3 Feb 2021 00:09:39 +0100 Subject: [PATCH 01/24] Add code for a minimal GSNv2 compliant forwarder and recipient --- contracts/GSNv2/BaseRelayRecipient.sol | 37 +++++++++++++++++ contracts/GSNv2/MinimalForwarder.sol | 57 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 contracts/GSNv2/BaseRelayRecipient.sol create mode 100644 contracts/GSNv2/MinimalForwarder.sol diff --git a/contracts/GSNv2/BaseRelayRecipient.sol b/contracts/GSNv2/BaseRelayRecipient.sol new file mode 100644 index 00000000000..986e1795e57 --- /dev/null +++ b/contracts/GSNv2/BaseRelayRecipient.sol @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../utils/Context.sol"; + +/* + * @dev Context variant with ERC2771 support. + */ +abstract contract BaseRelayRecipient is Context { + address immutable _trustedForwarder; + + constructor(address trustedForwarder) { + _trustedForwarder = trustedForwarder; + } + + function isTrustedForwarder(address forwarder) public view virtual returns(bool) { + return forwarder == _trustedForwarder; + } + + function _msgSender() internal view virtual override returns (address sender) { + if (isTrustedForwarder(msg.sender)) { + // return abi.decode(abi.encodePacked(bytes12(0), msg.data[msg.data.length-20:]), (address)); + assembly { sender := shr(96, calldataload(sub(calldatasize(), 20))) } + } else { + return Context._msgSender(); + } + } + + function _msgData() internal view virtual override returns (bytes calldata) { + if (isTrustedForwarder(msg.sender)) { + return msg.data[:msg.data.length-20]; + } else { + return Context._msgData(); + } + } +} diff --git a/contracts/GSNv2/MinimalForwarder.sol b/contracts/GSNv2/MinimalForwarder.sol new file mode 100644 index 00000000000..4fe4a82cfe8 --- /dev/null +++ b/contracts/GSNv2/MinimalForwarder.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../cryptography/ECDSA.sol"; +import "../drafts/EIP712.sol"; + +/* + * @dev Minimal forwarder for GSNv2 + */ +contract MinimalForwarder is EIP712 { + using ECDSA for bytes32; + + struct ForwardRequest { + address from; + address to; + uint256 value; + uint256 gas; + uint256 nonce; + bytes data; + } + + bytes32 private constant TYPEHASH = keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); + + mapping(address => uint256) private _nonces; + + constructor() EIP712("GSNv2 Forwarder", "0.0.1") {} + + function getNonce(address from) public view returns (uint256) { + return _nonces[from]; + } + + function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) { + address signer = _hashTypedDataV4(keccak256(abi.encode( + TYPEHASH, + req.from, + req.to, + req.value, + req.gas, + req.nonce, + req.data + ))).recover(signature); + return _nonces[req.from] == req.nonce && signer == req.from; + } + + function execute(ForwardRequest calldata req, bytes calldata signature) public payable returns (bool, bytes memory) { + require(verify(req, signature)); + _nonces[req.from] = req.nonce + 1; + + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); + // Check gas: https://ronan.eth.link/blog/ethereum-gas-dangers/ + assert(gasleft() > req.gas / 63); + + return (success, returndata); + } +} From b1823d1b0ab4bce228acfb447e043249673d60b7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 8 Feb 2021 15:12:21 +0100 Subject: [PATCH 02/24] fix erc712 hash computation --- contracts/GSNv2/MinimalForwarder.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/GSNv2/MinimalForwarder.sol b/contracts/GSNv2/MinimalForwarder.sol index 4fe4a82cfe8..23aa6ebc43f 100644 --- a/contracts/GSNv2/MinimalForwarder.sol +++ b/contracts/GSNv2/MinimalForwarder.sol @@ -38,7 +38,7 @@ contract MinimalForwarder is EIP712 { req.value, req.gas, req.nonce, - req.data + keccak256(req.data) ))).recover(signature); return _nonces[req.from] == req.nonce && signer == req.from; } From a0efed688d3e6a8623188273f50669d602dbf205 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 8 Feb 2021 15:12:32 +0100 Subject: [PATCH 03/24] add testing for gsnv2 --- contracts/mocks/BaseRelayRecipientMock.sol | 19 ++++ test/GSNv2/BaseRelayRecipient.test.js | 104 +++++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 contracts/mocks/BaseRelayRecipientMock.sol create mode 100644 test/GSNv2/BaseRelayRecipient.test.js diff --git a/contracts/mocks/BaseRelayRecipientMock.sol b/contracts/mocks/BaseRelayRecipientMock.sol new file mode 100644 index 00000000000..cefa0c670e3 --- /dev/null +++ b/contracts/mocks/BaseRelayRecipientMock.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./ContextMock.sol"; +import "../GSNv2/BaseRelayRecipient.sol"; + +// By inheriting from BaseRelayRecipient, Context's internal functions are overridden automatically +contract BaseRelayRecipientMock is ContextMock, BaseRelayRecipient { + constructor(address trustedForwarder) BaseRelayRecipient(trustedForwarder) {} + + function _msgSender() internal override(Context, BaseRelayRecipient) view virtual returns (address) { + return BaseRelayRecipient._msgSender(); + } + + function _msgData() internal override(Context, BaseRelayRecipient) view virtual returns (bytes calldata) { + return BaseRelayRecipient._msgData(); + } +} diff --git a/test/GSNv2/BaseRelayRecipient.test.js b/test/GSNv2/BaseRelayRecipient.test.js new file mode 100644 index 00000000000..9818b5c7a91 --- /dev/null +++ b/test/GSNv2/BaseRelayRecipient.test.js @@ -0,0 +1,104 @@ +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { EIP712Domain, domainSeparator } = require('../helpers/eip712'); + +const { expectEvent } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); +const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ContextMockCaller = artifacts.require('ContextMockCaller'); + +const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); + +contract('GSNRecipient', function (accounts) { + beforeEach(async function () { + this.forwarder = await MinimalForwarder.new(); + this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + }); + + it('recognize trusted forwarder', async function () { + expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); + }); + + context('when called directly', function () { + beforeEach(async function () { + this.context = this.recipient; // The Context behavior expects the contract in this.context + this.caller = await ContextMockCaller.new(); + }); + + shouldBehaveLikeRegularContext(...accounts); + }); + + context('when receiving a relayed call', function () { + beforeEach(async function () { + this.wallet = Wallet.generate(); + this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); + this.data = { + types: { + EIP712Domain, + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'data', type: 'bytes' }, + ], + }, + domain: { + name: "GSNv2 Forwarder", + version: "0.0.1", + chainId: await web3.eth.getChainId(), + verifyingContract: this.forwarder.address, + }, + primaryType: 'ForwardRequest', + } + }); + + describe('msgSender', function () { + it('returns the relayed transaction original sender', async function () { + const req = { + from: this.sender, + to: this.recipient.address, + value: "0", + gas: "100000", + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data: this.recipient.contract.methods.msgSender().encodeABI(), + }; + + const data = { ...this.data, message: req }; + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data }); + + expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); + }); + }); + + describe('msgData', function () { + it('returns the relayed transaction original data', async function () { + const integerValue = '42'; + const stringValue = 'OpenZeppelin'; + + const req = { + from: this.sender, + to: this.recipient.address, + value: "0", + gas: "100000", + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data: this.recipient.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI(), + }; + + const data = { ...this.data, message: req }; + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data }); + + expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data: req.data, integerValue, stringValue }); + }); + }); + }); +}); From 15839a7ea3950a855e84a43e08b68f37ba6ee3e5 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 8 Feb 2021 15:14:37 +0100 Subject: [PATCH 04/24] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdc5834e927..27d19ecd9d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * Now targeting the 0.8.x line of Solidity compilers. For 0.6.x (resp 0.7.x) support, use version 3.4.0 (resp 3.4.0-solc-0.7) of OpenZeppelin. * `Context`: making `_msgData` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) +* `GSNv2`: add a `BaseRelayRecipient` and a `MinimalForwarder` for experimenting with GSNv2 ([#2508](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2508)) ## 3.4.0 (2021-02-02) From b7183a4507da852509ab71e970f93994b6ffd9ac Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 8 Feb 2021 15:21:16 +0100 Subject: [PATCH 05/24] fix lint --- test/GSNv2/BaseRelayRecipient.test.js | 45 ++++++++++++++------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/test/GSNv2/BaseRelayRecipient.test.js b/test/GSNv2/BaseRelayRecipient.test.js index 9818b5c7a91..a70990b9c2a 100644 --- a/test/GSNv2/BaseRelayRecipient.test.js +++ b/test/GSNv2/BaseRelayRecipient.test.js @@ -1,6 +1,6 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain, domainSeparator } = require('../helpers/eip712'); +const { EIP712Domain } = require('../helpers/eip712'); const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); @@ -47,30 +47,32 @@ contract('GSNRecipient', function (accounts) { ], }, domain: { - name: "GSNv2 Forwarder", - version: "0.0.1", + name: 'GSNv2 Forwarder', + version: '0.0.1', chainId: await web3.eth.getChainId(), verifyingContract: this.forwarder.address, }, primaryType: 'ForwardRequest', - } + }; }); describe('msgSender', function () { it('returns the relayed transaction original sender', async function () { + const data = this.recipient.contract.methods.msgSender().encodeABI(); + const req = { - from: this.sender, - to: this.recipient.address, - value: "0", - gas: "100000", + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data: this.recipient.contract.methods.msgSender().encodeABI(), + data, }; - const data = { ...this.data, message: req }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data }); + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.be.true; + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; const { tx } = await this.forwarder.execute(req, sign); await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); @@ -81,23 +83,24 @@ contract('GSNRecipient', function (accounts) { it('returns the relayed transaction original data', async function () { const integerValue = '42'; const stringValue = 'OpenZeppelin'; + const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); const req = { - from: this.sender, - to: this.recipient.address, - value: "0", - gas: "100000", + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data: this.recipient.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI(), + data, }; - const data = { ...this.data, message: req }; - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data }); + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - expect(await this.forwarder.verify(req, sign)).to.be.true; + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; const { tx } = await this.forwarder.execute(req, sign); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data: req.data, integerValue, stringValue }); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); }); }); }); From a74c5c7b887ef394c1e3be71c77edf14b180ab2e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 00:42:59 +0100 Subject: [PATCH 06/24] Apply suggestions from code review Co-authored-by: Francisco Giordano --- contracts/GSNv2/BaseRelayRecipient.sol | 6 +++--- contracts/GSNv2/MinimalForwarder.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/GSNv2/BaseRelayRecipient.sol b/contracts/GSNv2/BaseRelayRecipient.sol index 986e1795e57..199079e9794 100644 --- a/contracts/GSNv2/BaseRelayRecipient.sol +++ b/contracts/GSNv2/BaseRelayRecipient.sol @@ -20,10 +20,10 @@ abstract contract BaseRelayRecipient is Context { function _msgSender() internal view virtual override returns (address sender) { if (isTrustedForwarder(msg.sender)) { - // return abi.decode(abi.encodePacked(bytes12(0), msg.data[msg.data.length-20:]), (address)); + // The assembly code is more direct than the Solidity version using `abi.decode`. assembly { sender := shr(96, calldataload(sub(calldatasize(), 20))) } } else { - return Context._msgSender(); + return super._msgSender(); } } @@ -31,7 +31,7 @@ abstract contract BaseRelayRecipient is Context { if (isTrustedForwarder(msg.sender)) { return msg.data[:msg.data.length-20]; } else { - return Context._msgData(); + return super._msgData(); } } } diff --git a/contracts/GSNv2/MinimalForwarder.sol b/contracts/GSNv2/MinimalForwarder.sol index 23aa6ebc43f..79060b34ac1 100644 --- a/contracts/GSNv2/MinimalForwarder.sol +++ b/contracts/GSNv2/MinimalForwarder.sol @@ -44,7 +44,7 @@ contract MinimalForwarder is EIP712 { } function execute(ForwardRequest calldata req, bytes calldata signature) public payable returns (bool, bytes memory) { - require(verify(req, signature)); + require(verify(req, signature), "MinimalForwarder: signature does not match request"); _nonces[req.from] = req.nonce + 1; // solhint-disable-next-line avoid-low-level-calls From 138fbd1facb59ff48bc98434005acd633abc5a0d Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 23:28:51 +0100 Subject: [PATCH 07/24] use ERC2770 for gsnv2 forwarder --- contracts/GSNv2/IForwarder.sol | 70 +++++++++++++++ contracts/GSNv2/MinimalForwarder.sol | 125 ++++++++++++++++++++------ contracts/cryptography/ECDSA.sol | 9 ++ test/GSNv2/BaseRelayRecipient.test.js | 71 ++++++++++----- 4 files changed, 226 insertions(+), 49 deletions(-) create mode 100644 contracts/GSNv2/IForwarder.sol diff --git a/contracts/GSNv2/IForwarder.sol b/contracts/GSNv2/IForwarder.sol new file mode 100644 index 00000000000..8c64c6ab291 --- /dev/null +++ b/contracts/GSNv2/IForwarder.sol @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +interface IForwarder { + + struct ForwardRequest { + address from; + address to; + uint256 value; + uint256 gas; + uint256 nonce; + bytes data; + } + + function getNonce(address from) external view returns(uint256); + + /** + * verify the transaction would execute. + * validate the signature and the nonce of the request. + * revert if either signature or nonce are incorrect. + */ + function verify( + ForwardRequest calldata forwardRequest, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) external view; + + /** + * execute a transaction + * @param forwardRequest - all transaction parameters + * @param domainSeparator - domain used when signing this request + * @param requestTypeHash - request type used when signing this request. + * @param suffixData - the extension data used when signing this request. + * @param signature - signature to validate. + * + * the transaction is verified, and then executed. + * the success and ret of "call" are returned. + * This method would revert only verification errors. target errors + * are reported using the returned "success" and ret string + */ + function execute( + ForwardRequest calldata forwardRequest, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) + external payable returns (bool success, bytes memory ret); + + /** + * Register a new Request typehash. + * @param typeName - the name of the request type. + * @param typeSuffix - anything after the generic params can be empty string (if no extra fields are needed) + * if it does contain a value, then a comma is added first. + */ + // function registerRequestType(string calldata typeName, string calldata typeSuffix) external; + + /** + * Register a new domain separator. + * The domain separator must have the following fields: name,version,chainId, verifyingContract. + * the chainId is the current network's chainId, and the verifyingContract is this forwarder. + * This method is given the domain name and version to create and register the domain separator value. + * @param name the domain's display name + * @param version the domain/protocol version + */ + // function registerDomainSeparator(string calldata name, string calldata version) external; +} diff --git a/contracts/GSNv2/MinimalForwarder.sol b/contracts/GSNv2/MinimalForwarder.sol index 79060b34ac1..4479e7af6bc 100644 --- a/contracts/GSNv2/MinimalForwarder.sol +++ b/contracts/GSNv2/MinimalForwarder.sol @@ -2,50 +2,82 @@ pragma solidity ^0.8.0; +import "./IForwarder.sol"; import "../cryptography/ECDSA.sol"; -import "../drafts/EIP712.sol"; +import "../utils/Counters.sol"; /* * @dev Minimal forwarder for GSNv2 */ -contract MinimalForwarder is EIP712 { +contract MinimalForwarder is IForwarder { using ECDSA for bytes32; + using Counters for Counters.Counter; - struct ForwardRequest { - address from; - address to; - uint256 value; - uint256 gas; - uint256 nonce; - bytes data; - } + bytes32 private constant _EIP721DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - bytes32 private constant TYPEHASH = keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); + mapping(address => Counters.Counter) private _nonces; + mapping(bytes32 => bool) public _typeHashes; + mapping(bytes32 => bool) public _domains; - mapping(address => uint256) private _nonces; + event NewTypeHash(bytes32 typeHash); + event NewDomainSeparator(bytes32 domainSeparator); - constructor() EIP712("GSNv2 Forwarder", "0.0.1") {} + constructor(string memory name, string memory version) { + registerDomainSeparator(name, version); + registerRequestType("ForwardRequest", "", ""); + } - function getNonce(address from) public view returns (uint256) { - return _nonces[from]; + function getNonce(address from) public view override returns (uint256) { + return _nonces[from].current(); } - function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) { - address signer = _hashTypedDataV4(keccak256(abi.encode( - TYPEHASH, - req.from, - req.to, - req.value, - req.gas, - req.nonce, - keccak256(req.data) - ))).recover(signature); - return _nonces[req.from] == req.nonce && signer == req.from; + function verify( + ForwardRequest calldata req, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) + public view override + { + require(_domains[domainSeparator], "Forwarder: invalid domainSeparator"); + require(_typeHashes[requestTypeHash], "Forwarder: invalid requestTypeHash"); + bytes32 structhash = ECDSA.toTypedDataHash( + domainSeparator, + keccak256(abi.encodePacked( + requestTypeHash, + abi.encode( + req.from, + req.to, + req.value, + req.gas, + req.nonce, + keccak256(req.data) + ), + suffixData + )) + ); + require(_nonces[req.from].current() == req.nonce, "Forwarder: invalid nonce"); + require(structhash.recover(signature) == req.from, "Forwarder: invalid signature"); } - function execute(ForwardRequest calldata req, bytes calldata signature) public payable returns (bool, bytes memory) { - require(verify(req, signature), "MinimalForwarder: signature does not match request"); - _nonces[req.from] = req.nonce + 1; + function execute( + ForwardRequest calldata req, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) + public payable override returns (bool, bytes memory) + { + verify( + req, + domainSeparator, + requestTypeHash, + suffixData, + signature + ); + _nonces[req.from].increment(); // solhint-disable-next-line avoid-low-level-calls (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); @@ -54,4 +86,39 @@ contract MinimalForwarder is EIP712 { return (success, returndata); } + + function registerDomainSeparator( + string memory name, + string memory version + ) + public + { + bytes32 domainSeparator = keccak256(abi.encode( + _EIP721DOMAIN_TYPEHASH, + keccak256(bytes(name)), + keccak256(bytes(version)), + block.chainid, + address(this) + )); + _domains[domainSeparator] = true; + emit NewDomainSeparator(domainSeparator); + } + + function registerRequestType( + string memory typeName, + string memory extraFields, + string memory extraTypes + ) + public + { + bytes32 typeHash = keccak256(abi.encodePacked( + typeName, + "(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data", + extraFields, + ")", + extraTypes + )); + _typeHashes[typeHash] = true; + emit NewTypeHash(typeHash); + } } diff --git a/contracts/cryptography/ECDSA.sol b/contracts/cryptography/ECDSA.sol index e70815df1fd..a877d9c8370 100644 --- a/contracts/cryptography/ECDSA.sol +++ b/contracts/cryptography/ECDSA.sol @@ -83,4 +83,13 @@ library ECDSA { // enforced by the type signature above return keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)); } + + /** + * @dev TODO. + * + * See {recover}. + */ + function toTypedDataHash(bytes32 domainSeparator, bytes32 structHash) internal pure returns (bytes32) { + return keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash)); + } } diff --git a/test/GSNv2/BaseRelayRecipient.test.js b/test/GSNv2/BaseRelayRecipient.test.js index a70990b9c2a..99f6c3c990f 100644 --- a/test/GSNv2/BaseRelayRecipient.test.js +++ b/test/GSNv2/BaseRelayRecipient.test.js @@ -11,16 +11,50 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); + +const name = 'GSNv2 Forwarder'; +const version = '0.0.1'; + contract('GSNRecipient', function (accounts) { beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(); + this.forwarder = await MinimalForwarder.new(name, version); this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + + this.domain = { + name, + version, + chainId: await web3.eth.getChainId(), + verifyingContract: this.forwarder.address, + }; + this.types = { + EIP712Domain, + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'data', type: 'bytes' }, + ], + }; + this.domainSeparator = ethSigUtil.TypedDataUtils.hashStruct('EIP712Domain', this.domain, this.types); + this.requestTypeHash = ethSigUtil.TypedDataUtils.hashType('ForwardRequest', this.types); }); it('recognize trusted forwarder', async function () { expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); }); + context('configuration', function () { + it('domainSeparator whitelisted', async function () { + expect(await this.forwarder._domains(this.domainSeparator)).to.be.true; + }); + + it('typeHash whitelisted', async function () { + expect(await this.forwarder._typeHashes(this.requestTypeHash)).to.be.true; + }); + }); + context('when called directly', function () { beforeEach(async function () { this.context = this.recipient; // The Context behavior expects the contract in this.context @@ -35,23 +69,8 @@ contract('GSNRecipient', function (accounts) { this.wallet = Wallet.generate(); this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); this.data = { - types: { - EIP712Domain, - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'data', type: 'bytes' }, - ], - }, - domain: { - name: 'GSNv2 Forwarder', - version: '0.0.1', - chainId: await web3.eth.getChainId(), - verifyingContract: this.forwarder.address, - }, + types: this.types, + domain: this.domain, primaryType: 'ForwardRequest', }; }); @@ -74,7 +93,13 @@ contract('GSNRecipient', function (accounts) { // rejected by lint :/ // expect(await this.forwarder.verify(req, sign)).to.be.true; - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute( + req, + this.domainSeparator, + this.requestTypeHash, + "0x", + sign, + ); await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); }); }); @@ -99,7 +124,13 @@ contract('GSNRecipient', function (accounts) { // rejected by lint :/ // expect(await this.forwarder.verify(req, sign)).to.be.true; - const { tx } = await this.forwarder.execute(req, sign); + const { tx } = await this.forwarder.execute( + req, + this.domainSeparator, + this.requestTypeHash, + "0x", + sign, + ); await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); }); }); From 6a02d75aee101ee24b4db11a0b5c9551cc21b7cf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 23:29:33 +0100 Subject: [PATCH 08/24] rename gsnv2 into metatx --- contracts/{GSNv2 => metatx}/BaseRelayRecipient.sol | 0 contracts/{GSNv2 => metatx}/IForwarder.sol | 0 contracts/{GSNv2 => metatx}/MinimalForwarder.sol | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename contracts/{GSNv2 => metatx}/BaseRelayRecipient.sol (100%) rename contracts/{GSNv2 => metatx}/IForwarder.sol (100%) rename contracts/{GSNv2 => metatx}/MinimalForwarder.sol (100%) diff --git a/contracts/GSNv2/BaseRelayRecipient.sol b/contracts/metatx/BaseRelayRecipient.sol similarity index 100% rename from contracts/GSNv2/BaseRelayRecipient.sol rename to contracts/metatx/BaseRelayRecipient.sol diff --git a/contracts/GSNv2/IForwarder.sol b/contracts/metatx/IForwarder.sol similarity index 100% rename from contracts/GSNv2/IForwarder.sol rename to contracts/metatx/IForwarder.sol diff --git a/contracts/GSNv2/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol similarity index 100% rename from contracts/GSNv2/MinimalForwarder.sol rename to contracts/metatx/MinimalForwarder.sol From 50c2fa4216101ba7c7bd2e54948fd7fcb26c8538 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 23:34:57 +0100 Subject: [PATCH 09/24] change event signature and fix import path --- contracts/metatx/MinimalForwarder.sol | 21 ++++++++++++--------- contracts/mocks/BaseRelayRecipientMock.sol | 2 +- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 4479e7af6bc..b9f46837c44 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -19,8 +19,9 @@ contract MinimalForwarder is IForwarder { mapping(bytes32 => bool) public _typeHashes; mapping(bytes32 => bool) public _domains; - event NewTypeHash(bytes32 typeHash); - event NewDomainSeparator(bytes32 domainSeparator); + event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); + event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); + constructor(string memory name, string memory version) { registerDomainSeparator(name, version); @@ -93,15 +94,16 @@ contract MinimalForwarder is IForwarder { ) public { - bytes32 domainSeparator = keccak256(abi.encode( + bytes memory domainValue = abi.encode( _EIP721DOMAIN_TYPEHASH, keccak256(bytes(name)), keccak256(bytes(version)), block.chainid, address(this) - )); + ); + bytes32 domainSeparator = keccak256(domainValue); _domains[domainSeparator] = true; - emit NewDomainSeparator(domainSeparator); + emit DomainRegistered(domainSeparator, domainValue); } function registerRequestType( @@ -111,14 +113,15 @@ contract MinimalForwarder is IForwarder { ) public { - bytes32 typeHash = keccak256(abi.encodePacked( + bytes memory requestType = abi.encodePacked( typeName, "(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data", extraFields, ")", extraTypes - )); - _typeHashes[typeHash] = true; - emit NewTypeHash(typeHash); + ); + bytes32 requestTypehash = keccak256(requestType); + _typeHashes[requestTypehash] = true; + emit RequestTypeRegistered(requestTypehash, string(requestType)); } } diff --git a/contracts/mocks/BaseRelayRecipientMock.sol b/contracts/mocks/BaseRelayRecipientMock.sol index cefa0c670e3..b1b60c40c6c 100644 --- a/contracts/mocks/BaseRelayRecipientMock.sol +++ b/contracts/mocks/BaseRelayRecipientMock.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.0; import "./ContextMock.sol"; -import "../GSNv2/BaseRelayRecipient.sol"; +import "../metatx/BaseRelayRecipient.sol"; // By inheriting from BaseRelayRecipient, Context's internal functions are overridden automatically contract BaseRelayRecipientMock is ContextMock, BaseRelayRecipient { From 435324f2059afbb8bd3eb4254be81457339f8836 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 23:40:37 +0100 Subject: [PATCH 10/24] fix lint --- contracts/metatx/MinimalForwarder.sol | 1 - test/GSNv2/BaseRelayRecipient.test.js | 29 ++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index b9f46837c44..bfa96cf728c 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -22,7 +22,6 @@ contract MinimalForwarder is IForwarder { event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); - constructor(string memory name, string memory version) { registerDomainSeparator(name, version); registerRequestType("ForwardRequest", "", ""); diff --git a/test/GSNv2/BaseRelayRecipient.test.js b/test/GSNv2/BaseRelayRecipient.test.js index 99f6c3c990f..bbe76150e60 100644 --- a/test/GSNv2/BaseRelayRecipient.test.js +++ b/test/GSNv2/BaseRelayRecipient.test.js @@ -11,7 +11,6 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); - const name = 'GSNv2 Forwarder'; const version = '0.0.1'; @@ -47,11 +46,11 @@ contract('GSNRecipient', function (accounts) { context('configuration', function () { it('domainSeparator whitelisted', async function () { - expect(await this.forwarder._domains(this.domainSeparator)).to.be.true; + expect(await this.forwarder._domains(this.domainSeparator)).to.be.equal(true); }); it('typeHash whitelisted', async function () { - expect(await this.forwarder._typeHashes(this.requestTypeHash)).to.be.true; + expect(await this.forwarder._typeHashes(this.requestTypeHash)).to.be.equal(true); }); }); @@ -90,14 +89,20 @@ contract('GSNRecipient', function (accounts) { const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - // rejected by lint :/ - // expect(await this.forwarder.verify(req, sign)).to.be.true; + // expect not throw + await this.forwarder.verify( + req, + this.domainSeparator, + this.requestTypeHash, + '0x', + sign, + ); const { tx } = await this.forwarder.execute( req, this.domainSeparator, this.requestTypeHash, - "0x", + '0x', sign, ); await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); @@ -121,14 +126,20 @@ contract('GSNRecipient', function (accounts) { const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - // rejected by lint :/ - // expect(await this.forwarder.verify(req, sign)).to.be.true; + // expect not throw + await this.forwarder.verify( + req, + this.domainSeparator, + this.requestTypeHash, + '0x', + sign, + ); const { tx } = await this.forwarder.execute( req, this.domainSeparator, this.requestTypeHash, - "0x", + '0x', sign, ); await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); From 0abb2530ecd8dbfde32551cb22d1db6a054dfe64 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 9 Feb 2021 23:50:36 +0100 Subject: [PATCH 11/24] provide 2 version of the forwarder --- contracts/metatx/Forwarder.sol | 126 +++++++++++++++++ contracts/metatx/MinimalForwarder.sol | 127 ++++-------------- .../Forwarder.test.js} | 2 +- test/metatx/MinimalForwarder.test.js | 107 +++++++++++++++ 4 files changed, 263 insertions(+), 99 deletions(-) create mode 100644 contracts/metatx/Forwarder.sol rename test/{GSNv2/BaseRelayRecipient.test.js => metatx/Forwarder.test.js} (98%) create mode 100644 test/metatx/MinimalForwarder.test.js diff --git a/contracts/metatx/Forwarder.sol b/contracts/metatx/Forwarder.sol new file mode 100644 index 00000000000..6800a5da75b --- /dev/null +++ b/contracts/metatx/Forwarder.sol @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "./IForwarder.sol"; +import "../cryptography/ECDSA.sol"; +import "../utils/Counters.sol"; + +/* + * @dev Minimal forwarder for GSNv2 + */ +contract Forwarder is IForwarder { + using ECDSA for bytes32; + using Counters for Counters.Counter; + + bytes32 private constant _EIP721DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + + mapping(address => Counters.Counter) private _nonces; + mapping(bytes32 => bool) public _typeHashes; + mapping(bytes32 => bool) public _domains; + + event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); + event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); + + constructor(string memory name, string memory version) { + registerDomainSeparator(name, version); + registerRequestType("ForwardRequest", "", ""); + } + + function getNonce(address from) public view override returns (uint256) { + return _nonces[from].current(); + } + + function verify( + ForwardRequest calldata req, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) + public view override + { + require(_domains[domainSeparator], "Forwarder: invalid domainSeparator"); + require(_typeHashes[requestTypeHash], "Forwarder: invalid requestTypeHash"); + bytes32 structhash = ECDSA.toTypedDataHash( + domainSeparator, + keccak256(abi.encodePacked( + requestTypeHash, + abi.encode( + req.from, + req.to, + req.value, + req.gas, + req.nonce, + keccak256(req.data) + ), + suffixData + )) + ); + require(_nonces[req.from].current() == req.nonce, "Forwarder: invalid nonce"); + require(structhash.recover(signature) == req.from, "Forwarder: invalid signature"); + } + + function execute( + ForwardRequest calldata req, + bytes32 domainSeparator, + bytes32 requestTypeHash, + bytes calldata suffixData, + bytes calldata signature + ) + public payable override returns (bool, bytes memory) + { + verify( + req, + domainSeparator, + requestTypeHash, + suffixData, + signature + ); + _nonces[req.from].increment(); + + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); + // Check gas: https://ronan.eth.link/blog/ethereum-gas-dangers/ + assert(gasleft() > req.gas / 63); + + return (success, returndata); + } + + function registerDomainSeparator( + string memory name, + string memory version + ) + public + { + bytes memory domainValue = abi.encode( + _EIP721DOMAIN_TYPEHASH, + keccak256(bytes(name)), + keccak256(bytes(version)), + block.chainid, + address(this) + ); + bytes32 domainSeparator = keccak256(domainValue); + _domains[domainSeparator] = true; + emit DomainRegistered(domainSeparator, domainValue); + } + + function registerRequestType( + string memory typeName, + string memory extraFields, + string memory extraTypes + ) + public + { + bytes memory requestType = abi.encodePacked( + typeName, + "(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data", + extraFields, + ")", + extraTypes + ); + bytes32 requestTypehash = keccak256(requestType); + _typeHashes[requestTypehash] = true; + emit RequestTypeRegistered(requestTypehash, string(requestType)); + } +} diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index bfa96cf728c..79060b34ac1 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -2,82 +2,50 @@ pragma solidity ^0.8.0; -import "./IForwarder.sol"; import "../cryptography/ECDSA.sol"; -import "../utils/Counters.sol"; +import "../drafts/EIP712.sol"; /* * @dev Minimal forwarder for GSNv2 */ -contract MinimalForwarder is IForwarder { +contract MinimalForwarder is EIP712 { using ECDSA for bytes32; - using Counters for Counters.Counter; - bytes32 private constant _EIP721DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); + struct ForwardRequest { + address from; + address to; + uint256 value; + uint256 gas; + uint256 nonce; + bytes data; + } - mapping(address => Counters.Counter) private _nonces; - mapping(bytes32 => bool) public _typeHashes; - mapping(bytes32 => bool) public _domains; + bytes32 private constant TYPEHASH = keccak256("ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data)"); - event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); - event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); + mapping(address => uint256) private _nonces; - constructor(string memory name, string memory version) { - registerDomainSeparator(name, version); - registerRequestType("ForwardRequest", "", ""); - } + constructor() EIP712("GSNv2 Forwarder", "0.0.1") {} - function getNonce(address from) public view override returns (uint256) { - return _nonces[from].current(); + function getNonce(address from) public view returns (uint256) { + return _nonces[from]; } - function verify( - ForwardRequest calldata req, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) - public view override - { - require(_domains[domainSeparator], "Forwarder: invalid domainSeparator"); - require(_typeHashes[requestTypeHash], "Forwarder: invalid requestTypeHash"); - bytes32 structhash = ECDSA.toTypedDataHash( - domainSeparator, - keccak256(abi.encodePacked( - requestTypeHash, - abi.encode( - req.from, - req.to, - req.value, - req.gas, - req.nonce, - keccak256(req.data) - ), - suffixData - )) - ); - require(_nonces[req.from].current() == req.nonce, "Forwarder: invalid nonce"); - require(structhash.recover(signature) == req.from, "Forwarder: invalid signature"); + function verify(ForwardRequest calldata req, bytes calldata signature) public view returns (bool) { + address signer = _hashTypedDataV4(keccak256(abi.encode( + TYPEHASH, + req.from, + req.to, + req.value, + req.gas, + req.nonce, + keccak256(req.data) + ))).recover(signature); + return _nonces[req.from] == req.nonce && signer == req.from; } - function execute( - ForwardRequest calldata req, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) - public payable override returns (bool, bytes memory) - { - verify( - req, - domainSeparator, - requestTypeHash, - suffixData, - signature - ); - _nonces[req.from].increment(); + function execute(ForwardRequest calldata req, bytes calldata signature) public payable returns (bool, bytes memory) { + require(verify(req, signature), "MinimalForwarder: signature does not match request"); + _nonces[req.from] = req.nonce + 1; // solhint-disable-next-line avoid-low-level-calls (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); @@ -86,41 +54,4 @@ contract MinimalForwarder is IForwarder { return (success, returndata); } - - function registerDomainSeparator( - string memory name, - string memory version - ) - public - { - bytes memory domainValue = abi.encode( - _EIP721DOMAIN_TYPEHASH, - keccak256(bytes(name)), - keccak256(bytes(version)), - block.chainid, - address(this) - ); - bytes32 domainSeparator = keccak256(domainValue); - _domains[domainSeparator] = true; - emit DomainRegistered(domainSeparator, domainValue); - } - - function registerRequestType( - string memory typeName, - string memory extraFields, - string memory extraTypes - ) - public - { - bytes memory requestType = abi.encodePacked( - typeName, - "(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data", - extraFields, - ")", - extraTypes - ); - bytes32 requestTypehash = keccak256(requestType); - _typeHashes[requestTypehash] = true; - emit RequestTypeRegistered(requestTypehash, string(requestType)); - } } diff --git a/test/GSNv2/BaseRelayRecipient.test.js b/test/metatx/Forwarder.test.js similarity index 98% rename from test/GSNv2/BaseRelayRecipient.test.js rename to test/metatx/Forwarder.test.js index bbe76150e60..2e9018218eb 100644 --- a/test/GSNv2/BaseRelayRecipient.test.js +++ b/test/metatx/Forwarder.test.js @@ -6,7 +6,7 @@ const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); -const MinimalForwarder = artifacts.require('MinimalForwarder'); +const MinimalForwarder = artifacts.require('Forwarder'); const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js new file mode 100644 index 00000000000..a70990b9c2a --- /dev/null +++ b/test/metatx/MinimalForwarder.test.js @@ -0,0 +1,107 @@ +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { EIP712Domain } = require('../helpers/eip712'); + +const { expectEvent } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); +const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ContextMockCaller = artifacts.require('ContextMockCaller'); + +const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); + +contract('GSNRecipient', function (accounts) { + beforeEach(async function () { + this.forwarder = await MinimalForwarder.new(); + this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + }); + + it('recognize trusted forwarder', async function () { + expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); + }); + + context('when called directly', function () { + beforeEach(async function () { + this.context = this.recipient; // The Context behavior expects the contract in this.context + this.caller = await ContextMockCaller.new(); + }); + + shouldBehaveLikeRegularContext(...accounts); + }); + + context('when receiving a relayed call', function () { + beforeEach(async function () { + this.wallet = Wallet.generate(); + this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); + this.data = { + types: { + EIP712Domain, + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'data', type: 'bytes' }, + ], + }, + domain: { + name: 'GSNv2 Forwarder', + version: '0.0.1', + chainId: await web3.eth.getChainId(), + verifyingContract: this.forwarder.address, + }, + primaryType: 'ForwardRequest', + }; + }); + + describe('msgSender', function () { + it('returns the relayed transaction original sender', async function () { + const data = this.recipient.contract.methods.msgSender().encodeABI(); + + const req = { + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data, + }; + + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); + + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); + }); + }); + + describe('msgData', function () { + it('returns the relayed transaction original data', async function () { + const integerValue = '42'; + const stringValue = 'OpenZeppelin'; + const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); + + const req = { + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data, + }; + + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); + + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); + }); + }); + }); +}); From 3a1fed9d193a3e614116bc083e423cf24fa7468b Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 11 Feb 2021 17:43:14 +0100 Subject: [PATCH 12/24] remove gsnv2 mention from minimalforwarder domain --- contracts/metatx/MinimalForwarder.sol | 2 +- test/metatx/MinimalForwarder.test.js | 40 +++++++++++++++------------ 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 79060b34ac1..0407eec7346 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -24,7 +24,7 @@ contract MinimalForwarder is EIP712 { mapping(address => uint256) private _nonces; - constructor() EIP712("GSNv2 Forwarder", "0.0.1") {} + constructor() EIP712("MinimalForwarder", "0.0.1") {} function getNonce(address from) public view returns (uint256) { return _nonces[from]; diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index a70990b9c2a..4798f0b8a40 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -11,10 +11,31 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); +const name = 'MinimalForwarder'; +const version = '0.0.1'; + contract('GSNRecipient', function (accounts) { beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + + this.domain = { + name, + version, + chainId: await web3.eth.getChainId(), + verifyingContract: this.forwarder.address, + }; + this.types = { + EIP712Domain, + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'data', type: 'bytes' }, + ], + }; }); it('recognize trusted forwarder', async function () { @@ -35,23 +56,8 @@ contract('GSNRecipient', function (accounts) { this.wallet = Wallet.generate(); this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); this.data = { - types: { - EIP712Domain, - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'data', type: 'bytes' }, - ], - }, - domain: { - name: 'GSNv2 Forwarder', - version: '0.0.1', - chainId: await web3.eth.getChainId(), - verifyingContract: this.forwarder.address, - }, + types: this.types, + domain: this.domain, primaryType: 'ForwardRequest', }; }); From df6d730ee2ecc6cc54681499a877c4d304d9d909 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 12 Feb 2021 09:32:19 +0100 Subject: [PATCH 13/24] make _typehashes & _domains private --- contracts/metatx/Forwarder.sol | 4 ++-- test/metatx/Forwarder.test.js | 10 ---------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/contracts/metatx/Forwarder.sol b/contracts/metatx/Forwarder.sol index 6800a5da75b..0d6a1649840 100644 --- a/contracts/metatx/Forwarder.sol +++ b/contracts/metatx/Forwarder.sol @@ -16,8 +16,8 @@ contract Forwarder is IForwarder { bytes32 private constant _EIP721DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); mapping(address => Counters.Counter) private _nonces; - mapping(bytes32 => bool) public _typeHashes; - mapping(bytes32 => bool) public _domains; + mapping(bytes32 => bool) private _typeHashes; + mapping(bytes32 => bool) private _domains; event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); diff --git a/test/metatx/Forwarder.test.js b/test/metatx/Forwarder.test.js index 2e9018218eb..c035d83c0f3 100644 --- a/test/metatx/Forwarder.test.js +++ b/test/metatx/Forwarder.test.js @@ -44,16 +44,6 @@ contract('GSNRecipient', function (accounts) { expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); }); - context('configuration', function () { - it('domainSeparator whitelisted', async function () { - expect(await this.forwarder._domains(this.domainSeparator)).to.be.equal(true); - }); - - it('typeHash whitelisted', async function () { - expect(await this.forwarder._typeHashes(this.requestTypeHash)).to.be.equal(true); - }); - }); - context('when called directly', function () { beforeEach(async function () { this.context = this.recipient; // The Context behavior expects the contract in this.context From 1ad190a93c4488a5c476ad05447433b72e24efa2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 15 Feb 2021 14:17:23 +0100 Subject: [PATCH 14/24] Remove ERC2770 forwarder for now --- contracts/metatx/Forwarder.sol | 126 ----------------------------- contracts/metatx/IForwarder.sol | 70 ---------------- test/metatx/Forwarder.test.js | 139 -------------------------------- 3 files changed, 335 deletions(-) delete mode 100644 contracts/metatx/Forwarder.sol delete mode 100644 contracts/metatx/IForwarder.sol delete mode 100644 test/metatx/Forwarder.test.js diff --git a/contracts/metatx/Forwarder.sol b/contracts/metatx/Forwarder.sol deleted file mode 100644 index 0d6a1649840..00000000000 --- a/contracts/metatx/Forwarder.sol +++ /dev/null @@ -1,126 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -import "./IForwarder.sol"; -import "../cryptography/ECDSA.sol"; -import "../utils/Counters.sol"; - -/* - * @dev Minimal forwarder for GSNv2 - */ -contract Forwarder is IForwarder { - using ECDSA for bytes32; - using Counters for Counters.Counter; - - bytes32 private constant _EIP721DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - - mapping(address => Counters.Counter) private _nonces; - mapping(bytes32 => bool) private _typeHashes; - mapping(bytes32 => bool) private _domains; - - event RequestTypeRegistered(bytes32 indexed typeHash, string typeStr); - event DomainRegistered(bytes32 indexed domainSeparator, bytes domainValue); - - constructor(string memory name, string memory version) { - registerDomainSeparator(name, version); - registerRequestType("ForwardRequest", "", ""); - } - - function getNonce(address from) public view override returns (uint256) { - return _nonces[from].current(); - } - - function verify( - ForwardRequest calldata req, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) - public view override - { - require(_domains[domainSeparator], "Forwarder: invalid domainSeparator"); - require(_typeHashes[requestTypeHash], "Forwarder: invalid requestTypeHash"); - bytes32 structhash = ECDSA.toTypedDataHash( - domainSeparator, - keccak256(abi.encodePacked( - requestTypeHash, - abi.encode( - req.from, - req.to, - req.value, - req.gas, - req.nonce, - keccak256(req.data) - ), - suffixData - )) - ); - require(_nonces[req.from].current() == req.nonce, "Forwarder: invalid nonce"); - require(structhash.recover(signature) == req.from, "Forwarder: invalid signature"); - } - - function execute( - ForwardRequest calldata req, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) - public payable override returns (bool, bytes memory) - { - verify( - req, - domainSeparator, - requestTypeHash, - suffixData, - signature - ); - _nonces[req.from].increment(); - - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); - // Check gas: https://ronan.eth.link/blog/ethereum-gas-dangers/ - assert(gasleft() > req.gas / 63); - - return (success, returndata); - } - - function registerDomainSeparator( - string memory name, - string memory version - ) - public - { - bytes memory domainValue = abi.encode( - _EIP721DOMAIN_TYPEHASH, - keccak256(bytes(name)), - keccak256(bytes(version)), - block.chainid, - address(this) - ); - bytes32 domainSeparator = keccak256(domainValue); - _domains[domainSeparator] = true; - emit DomainRegistered(domainSeparator, domainValue); - } - - function registerRequestType( - string memory typeName, - string memory extraFields, - string memory extraTypes - ) - public - { - bytes memory requestType = abi.encodePacked( - typeName, - "(address from,address to,uint256 value,uint256 gas,uint256 nonce,bytes data", - extraFields, - ")", - extraTypes - ); - bytes32 requestTypehash = keccak256(requestType); - _typeHashes[requestTypehash] = true; - emit RequestTypeRegistered(requestTypehash, string(requestType)); - } -} diff --git a/contracts/metatx/IForwarder.sol b/contracts/metatx/IForwarder.sol deleted file mode 100644 index 8c64c6ab291..00000000000 --- a/contracts/metatx/IForwarder.sol +++ /dev/null @@ -1,70 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity ^0.8.0; - -interface IForwarder { - - struct ForwardRequest { - address from; - address to; - uint256 value; - uint256 gas; - uint256 nonce; - bytes data; - } - - function getNonce(address from) external view returns(uint256); - - /** - * verify the transaction would execute. - * validate the signature and the nonce of the request. - * revert if either signature or nonce are incorrect. - */ - function verify( - ForwardRequest calldata forwardRequest, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) external view; - - /** - * execute a transaction - * @param forwardRequest - all transaction parameters - * @param domainSeparator - domain used when signing this request - * @param requestTypeHash - request type used when signing this request. - * @param suffixData - the extension data used when signing this request. - * @param signature - signature to validate. - * - * the transaction is verified, and then executed. - * the success and ret of "call" are returned. - * This method would revert only verification errors. target errors - * are reported using the returned "success" and ret string - */ - function execute( - ForwardRequest calldata forwardRequest, - bytes32 domainSeparator, - bytes32 requestTypeHash, - bytes calldata suffixData, - bytes calldata signature - ) - external payable returns (bool success, bytes memory ret); - - /** - * Register a new Request typehash. - * @param typeName - the name of the request type. - * @param typeSuffix - anything after the generic params can be empty string (if no extra fields are needed) - * if it does contain a value, then a comma is added first. - */ - // function registerRequestType(string calldata typeName, string calldata typeSuffix) external; - - /** - * Register a new domain separator. - * The domain separator must have the following fields: name,version,chainId, verifyingContract. - * the chainId is the current network's chainId, and the verifyingContract is this forwarder. - * This method is given the domain name and version to create and register the domain separator value. - * @param name the domain's display name - * @param version the domain/protocol version - */ - // function registerDomainSeparator(string calldata name, string calldata version) external; -} diff --git a/test/metatx/Forwarder.test.js b/test/metatx/Forwarder.test.js deleted file mode 100644 index c035d83c0f3..00000000000 --- a/test/metatx/Forwarder.test.js +++ /dev/null @@ -1,139 +0,0 @@ -const ethSigUtil = require('eth-sig-util'); -const Wallet = require('ethereumjs-wallet').default; -const { EIP712Domain } = require('../helpers/eip712'); - -const { expectEvent } = require('@openzeppelin/test-helpers'); -const { expect } = require('chai'); - -const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); -const MinimalForwarder = artifacts.require('Forwarder'); -const ContextMockCaller = artifacts.require('ContextMockCaller'); - -const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); - -const name = 'GSNv2 Forwarder'; -const version = '0.0.1'; - -contract('GSNRecipient', function (accounts) { - beforeEach(async function () { - this.forwarder = await MinimalForwarder.new(name, version); - this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); - - this.domain = { - name, - version, - chainId: await web3.eth.getChainId(), - verifyingContract: this.forwarder.address, - }; - this.types = { - EIP712Domain, - ForwardRequest: [ - { name: 'from', type: 'address' }, - { name: 'to', type: 'address' }, - { name: 'value', type: 'uint256' }, - { name: 'gas', type: 'uint256' }, - { name: 'nonce', type: 'uint256' }, - { name: 'data', type: 'bytes' }, - ], - }; - this.domainSeparator = ethSigUtil.TypedDataUtils.hashStruct('EIP712Domain', this.domain, this.types); - this.requestTypeHash = ethSigUtil.TypedDataUtils.hashType('ForwardRequest', this.types); - }); - - it('recognize trusted forwarder', async function () { - expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); - }); - - context('when called directly', function () { - beforeEach(async function () { - this.context = this.recipient; // The Context behavior expects the contract in this.context - this.caller = await ContextMockCaller.new(); - }); - - shouldBehaveLikeRegularContext(...accounts); - }); - - context('when receiving a relayed call', function () { - beforeEach(async function () { - this.wallet = Wallet.generate(); - this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.data = { - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', - }; - }); - - describe('msgSender', function () { - it('returns the relayed transaction original sender', async function () { - const data = this.recipient.contract.methods.msgSender().encodeABI(); - - const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data, - }; - - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - - // expect not throw - await this.forwarder.verify( - req, - this.domainSeparator, - this.requestTypeHash, - '0x', - sign, - ); - - const { tx } = await this.forwarder.execute( - req, - this.domainSeparator, - this.requestTypeHash, - '0x', - sign, - ); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); - }); - }); - - describe('msgData', function () { - it('returns the relayed transaction original data', async function () { - const integerValue = '42'; - const stringValue = 'OpenZeppelin'; - const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); - - const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data, - }; - - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - - // expect not throw - await this.forwarder.verify( - req, - this.domainSeparator, - this.requestTypeHash, - '0x', - sign, - ); - - const { tx } = await this.forwarder.execute( - req, - this.domainSeparator, - this.requestTypeHash, - '0x', - sign, - ); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); - }); - }); - }); -}); From eebff1bfade0d5e737900782f98eb3c2f5a8a184 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 Feb 2021 18:53:37 +0100 Subject: [PATCH 15/24] improve coverage of metatx --- test/metatx/BaseRelayRecipient.test.js | 113 +++++++++++++++ test/metatx/MinimalForwarder.test.js | 185 ++++++++++++++++--------- 2 files changed, 232 insertions(+), 66 deletions(-) create mode 100644 test/metatx/BaseRelayRecipient.test.js diff --git a/test/metatx/BaseRelayRecipient.test.js b/test/metatx/BaseRelayRecipient.test.js new file mode 100644 index 00000000000..14950db9dd9 --- /dev/null +++ b/test/metatx/BaseRelayRecipient.test.js @@ -0,0 +1,113 @@ +const ethSigUtil = require('eth-sig-util'); +const Wallet = require('ethereumjs-wallet').default; +const { EIP712Domain } = require('../helpers/eip712'); + +const { expectEvent } = require('@openzeppelin/test-helpers'); +const { expect } = require('chai'); + +const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); +const MinimalForwarder = artifacts.require('MinimalForwarder'); +const ContextMockCaller = artifacts.require('ContextMockCaller'); + +const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); + +const name = 'MinimalForwarder'; +const version = '0.0.1'; + +contract('BaseRelayRecipient', function (accounts) { + beforeEach(async function () { + this.forwarder = await MinimalForwarder.new(); + this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + + this.domain = { + name, + version, + chainId: await web3.eth.getChainId(), + verifyingContract: this.forwarder.address, + }; + this.types = { + EIP712Domain, + ForwardRequest: [ + { name: 'from', type: 'address' }, + { name: 'to', type: 'address' }, + { name: 'value', type: 'uint256' }, + { name: 'gas', type: 'uint256' }, + { name: 'nonce', type: 'uint256' }, + { name: 'data', type: 'bytes' }, + ], + }; + }); + + it('recognize trusted forwarder', async function () { + expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); + }); + + context('when called directly', function () { + beforeEach(async function () { + this.context = this.recipient; // The Context behavior expects the contract in this.context + this.caller = await ContextMockCaller.new(); + }); + + shouldBehaveLikeRegularContext(...accounts); + }); + + context('when receiving a relayed call', function () { + beforeEach(async function () { + this.wallet = Wallet.generate(); + this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); + this.data = { + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + }; + }); + + describe('msgSender', function () { + it('returns the relayed transaction original sender', async function () { + const data = this.recipient.contract.methods.msgSender().encodeABI(); + + const req = { + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data, + }; + + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); + + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); + }); + }); + + describe('msgData', function () { + it('returns the relayed transaction original data', async function () { + const integerValue = '42'; + const stringValue = 'OpenZeppelin'; + const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); + + const req = { + from: this.sender, + to: this.recipient.address, + value: '0', + gas: '100000', + nonce: (await this.forwarder.getNonce(this.sender)).toString(), + data, + }; + + const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); + + // rejected by lint :/ + // expect(await this.forwarder.verify(req, sign)).to.be.true; + + const { tx } = await this.forwarder.execute(req, sign); + await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); + }); + }); + }); +}); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 4798f0b8a40..6151f97ecf7 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -2,23 +2,17 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { EIP712Domain } = require('../helpers/eip712'); -const { expectEvent } = require('@openzeppelin/test-helpers'); +const { expectRevert, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); const MinimalForwarder = artifacts.require('MinimalForwarder'); -const ContextMockCaller = artifacts.require('ContextMockCaller'); - -const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); const name = 'MinimalForwarder'; const version = '0.0.1'; -contract('GSNRecipient', function (accounts) { +contract('MinimalForwarder', function (accounts) { beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); - this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); - this.domain = { name, version, @@ -38,75 +32,134 @@ contract('GSNRecipient', function (accounts) { }; }); - it('recognize trusted forwarder', async function () { - expect(await this.recipient.isTrustedForwarder(this.forwarder.address)); - }); - - context('when called directly', function () { - beforeEach(async function () { - this.context = this.recipient; // The Context behavior expects the contract in this.context - this.caller = await ContextMockCaller.new(); - }); - - shouldBehaveLikeRegularContext(...accounts); - }); - - context('when receiving a relayed call', function () { + context('with message', function () { beforeEach(async function () { this.wallet = Wallet.generate(); this.sender = web3.utils.toChecksumAddress(this.wallet.getAddressString()); - this.data = { - types: this.types, - domain: this.domain, - primaryType: 'ForwardRequest', + this.req = { + from: this.sender, + to: constants.ZERO_ADDRESS, + value: '0', + gas: '100000', + nonce: Number(await this.forwarder.getNonce(this.sender)), + data: '0x', }; + this.sign = ethSigUtil.signTypedMessage( + this.wallet.getPrivateKey(), + { + data: { + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + message: this.req, + }, + }, + ); }); - describe('msgSender', function () { - it('returns the relayed transaction original sender', async function () { - const data = this.recipient.contract.methods.msgSender().encodeABI(); - - const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data, - }; - - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - - // rejected by lint :/ - // expect(await this.forwarder.verify(req, sign)).to.be.true; + context('verify', function () { + context('valid signature', function () { + beforeEach(async function () { + expect(await this.forwarder.getNonce(this.req.from)) + .to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + }); + + it('success', async function () { + expect(await this.forwarder.verify(this.req, this.sign)).to.be.equal(true); + }); + + afterEach(async function () { + expect(await this.forwarder.getNonce(this.req.from)) + .to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + }); + }); - const { tx } = await this.forwarder.execute(req, sign); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); + context('invalid signature', function () { + it('tampered from', async function () { + expect(await this.forwarder.verify({ ...this.req, from: accounts[0] }, this.sign)) + .to.be.equal(false); + }); + it('tampered to', async function () { + expect(await this.forwarder.verify({ ...this.req, to: accounts[0] }, this.sign)) + .to.be.equal(false); + }); + it('tampered value', async function () { + expect(await this.forwarder.verify({ ...this.req, value: web3.utils.toWei('1') }, this.sign)) + .to.be.equal(false); + }); + it('tampered nonce', async function () { + expect(await this.forwarder.verify({ ...this.req, nonce: this.req.nonce + 1 }, this.sign)) + .to.be.equal(false); + }); + it('tampered data', async function () { + expect(await this.forwarder.verify({ ...this.req, data: '0x1742' }, this.sign)) + .to.be.equal(false); + }); + it('tampered signature', async function () { + const tamperedsign = web3.utils.hexToBytes(this.sign); + tamperedsign[42] ^= 0xff; + expect(await this.forwarder.verify(this.req, web3.utils.bytesToHex(tamperedsign))) + .to.be.equal(false); + }); }); }); - describe('msgData', function () { - it('returns the relayed transaction original data', async function () { - const integerValue = '42'; - const stringValue = 'OpenZeppelin'; - const data = this.recipient.contract.methods.msgData(integerValue, stringValue).encodeABI(); - - const req = { - from: this.sender, - to: this.recipient.address, - value: '0', - gas: '100000', - nonce: (await this.forwarder.getNonce(this.sender)).toString(), - data, - }; - - const sign = ethSigUtil.signTypedMessage(this.wallet.getPrivateKey(), { data: { ...this.data, message: req } }); - - // rejected by lint :/ - // expect(await this.forwarder.verify(req, sign)).to.be.true; + context('execute', function () { + context('valid signature', function () { + beforeEach(async function () { + expect(await this.forwarder.getNonce(this.req.from)) + .to.be.bignumber.equal(web3.utils.toBN(this.req.nonce)); + }); + + it('success', async function () { + await this.forwarder.execute(this.req, this.sign); // expect to not revert + }); + + afterEach(async function () { + expect(await this.forwarder.getNonce(this.req.from)) + .to.be.bignumber.equal(web3.utils.toBN(this.req.nonce + 1)); + }); + }); - const { tx } = await this.forwarder.execute(req, sign); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); + context('invalid signature', function () { + it('tampered from', async function () { + await expectRevert( + this.forwarder.execute({ ...this.req, from: accounts[0] }, this.sign), + 'MinimalForwarder: signature does not match request', + ); + }); + it('tampered to', async function () { + await expectRevert( + this.forwarder.execute({ ...this.req, to: accounts[0] }, this.sign), + 'MinimalForwarder: signature does not match request', + ); + }); + it('tampered value', async function () { + await expectRevert( + this.forwarder.execute({ ...this.req, value: web3.utils.toWei('1') }, this.sign), + 'MinimalForwarder: signature does not match request', + ); + }); + it('tampered nonce', async function () { + await expectRevert( + this.forwarder.execute({ ...this.req, nonce: this.req.nonce + 1 }, this.sign), + 'MinimalForwarder: signature does not match request', + ); + }); + it('tampered data', async function () { + await expectRevert( + this.forwarder.execute({ ...this.req, data: '0x1742' }, this.sign), + 'MinimalForwarder: signature does not match request', + ); + }); + it('tampered signature', async function () { + const tamperedsign = web3.utils.hexToBytes(this.sign); + tamperedsign[42] ^= 0xff; + await expectRevert( + this.forwarder.execute(this.req, web3.utils.bytesToHex(tamperedsign)), + 'MinimalForwarder: signature does not match request', + ); + }); }); }); }); From c9a9c2d5b1d483e2971fd8a33db255dfcdb0250a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Feb 2021 10:35:45 +0100 Subject: [PATCH 16/24] Update contracts/metatx/MinimalForwarder.sol Co-authored-by: Francisco Giordano --- contracts/metatx/MinimalForwarder.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index 0407eec7346..f0b07bbb808 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -49,7 +49,8 @@ contract MinimalForwarder is EIP712 { // solhint-disable-next-line avoid-low-level-calls (bool success, bytes memory returndata) = req.to.call{gas: req.gas, value: req.value}(abi.encodePacked(req.data, req.from)); - // Check gas: https://ronan.eth.link/blog/ethereum-gas-dangers/ + // Validate that the relayer has sent enough gas for the call. + // See https://ronan.eth.link/blog/ethereum-gas-dangers/ assert(gasleft() > req.gas / 63); return (success, returndata); From c3b180e5761f5c327727d3fa77db9b44cc34c947 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Feb 2021 14:24:35 +0100 Subject: [PATCH 17/24] refactor EIP721 draft to use ECDSA tooling --- contracts/drafts/EIP712.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contracts/drafts/EIP712.sol b/contracts/drafts/EIP712.sol index 68ee9de4a22..c3be3ea7630 100644 --- a/contracts/drafts/EIP712.sol +++ b/contracts/drafts/EIP712.sol @@ -2,6 +2,8 @@ pragma solidity ^0.8.0; +import "../cryptography/ECDSA.sol"; + /** * @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. * @@ -95,6 +97,6 @@ abstract contract EIP712 { * ``` */ function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) { - return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorV4(), structHash)); + return ECDSA.toTypedDataHash(_domainSeparatorV4(), structHash); } } From e88b839894716393d50aa77a29ef554b8ec48b08 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Feb 2021 15:31:00 +0100 Subject: [PATCH 18/24] remove mention to gsn in changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baa09c760fa..78cc28ecd08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ * `Context`: making `_msgData` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) * `ERC20`: Removed the `_setDecimals` function and the storage slot associated to decimals. ([#2502](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2502)) * `Strings`: addition of a `toHexString` function. ([#2504](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2504)) - * `GSNv2`: add a `BaseRelayRecipient` and a `MinimalForwarder` for experimenting with GSNv2 ([#2508](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2508)) + * `metatx`: add a `BaseRelayRecipient` and a `MinimalForwarder` for meta-transactions ([#2508](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2508)) ## 3.4.0 (2021-02-02) From 63341e982ee0f95f9cb4c670d600bcc885660acb Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 18 Feb 2021 17:04:20 +0100 Subject: [PATCH 19/24] improve inline documentation --- contracts/cryptography/ECDSA.sol | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/cryptography/ECDSA.sol b/contracts/cryptography/ECDSA.sol index a877d9c8370..76dab5d2113 100644 --- a/contracts/cryptography/ECDSA.sol +++ b/contracts/cryptography/ECDSA.sol @@ -72,9 +72,9 @@ library ECDSA { /** * @dev Returns an Ethereum Signed Message, created from a `hash`. This - * replicates the behavior of the - * https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign[`eth_sign`] - * JSON-RPC method. + * produces hash corresponding to the one signed with the + * https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] + * JSON-RPC method as part of EIP-191. * * See {recover}. */ @@ -85,7 +85,11 @@ library ECDSA { } /** - * @dev TODO. + * @dev Returns an Ethereum Signed Typed Data, created from a + * `domainSeparator` and a `structHash`. This produces hash corresponding + * to the one signed with the + * https://eips.ethereum.org/EIPS/eip-712[`eth_signTypedData`] + * JSON-RPC method as part of EIP-712. * * See {recover}. */ From a3870fff6fdb23f1d7aa75764cb66d7098e8167c Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Feb 2021 14:43:46 +0100 Subject: [PATCH 20/24] fix import path --- test/metatx/BaseRelayRecipient.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metatx/BaseRelayRecipient.test.js b/test/metatx/BaseRelayRecipient.test.js index 14950db9dd9..9297458bb53 100644 --- a/test/metatx/BaseRelayRecipient.test.js +++ b/test/metatx/BaseRelayRecipient.test.js @@ -9,7 +9,7 @@ const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); const MinimalForwarder = artifacts.require('MinimalForwarder'); const ContextMockCaller = artifacts.require('ContextMockCaller'); -const { shouldBehaveLikeRegularContext } = require('../GSN/Context.behavior'); +const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); const name = 'MinimalForwarder'; const version = '0.0.1'; From 338e64019347a5dac31ec434131bdfd5ede147c2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 19 Feb 2021 15:59:31 +0100 Subject: [PATCH 21/24] =?UTF-8?q?rename=20BaseRelayRecipient=20=E2=86=92?= =?UTF-8?q?=20ERC2771Context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...BaseRelayRecipient.sol => ERC2771Context.sol} | 2 +- contracts/mocks/BaseRelayRecipientMock.sol | 16 ++++++++-------- ...yRecipient.test.js => ERC2771Context.test.js} | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-) rename contracts/metatx/{BaseRelayRecipient.sol => ERC2771Context.sol} (95%) rename test/metatx/{BaseRelayRecipient.test.js => ERC2771Context.test.js} (89%) diff --git a/contracts/metatx/BaseRelayRecipient.sol b/contracts/metatx/ERC2771Context.sol similarity index 95% rename from contracts/metatx/BaseRelayRecipient.sol rename to contracts/metatx/ERC2771Context.sol index 199079e9794..5ab22f5d9e7 100644 --- a/contracts/metatx/BaseRelayRecipient.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -7,7 +7,7 @@ import "../utils/Context.sol"; /* * @dev Context variant with ERC2771 support. */ -abstract contract BaseRelayRecipient is Context { +abstract contract ERC2771Context is Context { address immutable _trustedForwarder; constructor(address trustedForwarder) { diff --git a/contracts/mocks/BaseRelayRecipientMock.sol b/contracts/mocks/BaseRelayRecipientMock.sol index b1b60c40c6c..14c5b104c4c 100644 --- a/contracts/mocks/BaseRelayRecipientMock.sol +++ b/contracts/mocks/BaseRelayRecipientMock.sol @@ -3,17 +3,17 @@ pragma solidity ^0.8.0; import "./ContextMock.sol"; -import "../metatx/BaseRelayRecipient.sol"; +import "../metatx/ERC2771Context.sol"; -// By inheriting from BaseRelayRecipient, Context's internal functions are overridden automatically -contract BaseRelayRecipientMock is ContextMock, BaseRelayRecipient { - constructor(address trustedForwarder) BaseRelayRecipient(trustedForwarder) {} +// By inheriting from ERC2771Context, Context's internal functions are overridden automatically +contract ERC2771ContextMock is ContextMock, ERC2771Context { + constructor(address trustedForwarder) ERC2771Context(trustedForwarder) {} - function _msgSender() internal override(Context, BaseRelayRecipient) view virtual returns (address) { - return BaseRelayRecipient._msgSender(); + function _msgSender() internal override(Context, ERC2771Context) view virtual returns (address) { + return ERC2771Context._msgSender(); } - function _msgData() internal override(Context, BaseRelayRecipient) view virtual returns (bytes calldata) { - return BaseRelayRecipient._msgData(); + function _msgData() internal override(Context, ERC2771Context) view virtual returns (bytes calldata) { + return ERC2771Context._msgData(); } } diff --git a/test/metatx/BaseRelayRecipient.test.js b/test/metatx/ERC2771Context.test.js similarity index 89% rename from test/metatx/BaseRelayRecipient.test.js rename to test/metatx/ERC2771Context.test.js index 9297458bb53..50a94c25cc4 100644 --- a/test/metatx/BaseRelayRecipient.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -5,7 +5,7 @@ const { EIP712Domain } = require('../helpers/eip712'); const { expectEvent } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); -const BaseRelayRecipientMock = artifacts.require('BaseRelayRecipientMock'); +const ERC2771ContextMock = artifacts.require('ERC2771ContextMock'); const MinimalForwarder = artifacts.require('MinimalForwarder'); const ContextMockCaller = artifacts.require('ContextMockCaller'); @@ -14,10 +14,10 @@ const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); const name = 'MinimalForwarder'; const version = '0.0.1'; -contract('BaseRelayRecipient', function (accounts) { +contract('ERC2771Context', function (accounts) { beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); - this.recipient = await BaseRelayRecipientMock.new(this.forwarder.address); + this.recipient = await ERC2771ContextMock.new(this.forwarder.address); this.domain = { name, @@ -81,7 +81,7 @@ contract('BaseRelayRecipient', function (accounts) { // expect(await this.forwarder.verify(req, sign)).to.be.true; const { tx } = await this.forwarder.execute(req, sign); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Sender', { sender: this.sender }); + await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Sender', { sender: this.sender }); }); }); @@ -106,7 +106,7 @@ contract('BaseRelayRecipient', function (accounts) { // expect(await this.forwarder.verify(req, sign)).to.be.true; const { tx } = await this.forwarder.execute(req, sign); - await expectEvent.inTransaction(tx, BaseRelayRecipientMock, 'Data', { data, integerValue, stringValue }); + await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); }); }); }); From 99914ef24b76444a5d3f00ea0ca0c4fc7538614a Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 19 Feb 2021 14:51:34 -0300 Subject: [PATCH 22/24] rename mock file --- .../mocks/{BaseRelayRecipientMock.sol => ERC2771ContextMock.sol} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename contracts/mocks/{BaseRelayRecipientMock.sol => ERC2771ContextMock.sol} (100%) diff --git a/contracts/mocks/BaseRelayRecipientMock.sol b/contracts/mocks/ERC2771ContextMock.sol similarity index 100% rename from contracts/mocks/BaseRelayRecipientMock.sol rename to contracts/mocks/ERC2771ContextMock.sol From fd309c347341ad045a72211063cd106fbdb6af73 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 19 Feb 2021 14:54:34 -0300 Subject: [PATCH 23/24] fix naming in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b388213081..6f73365a5ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ * `GSN`: Deprecate GSNv1 support in favor of upcomming support for GSNv2. ([#2521](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2521)) * `ERC165`: Remove uses of storage in the base ERC165 implementation. ERC165 based contracts now use storage-less virtual functions. Old behaviour remains available in the `ERC165Storage` extension. ([#2505](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2505)) * `Initializable`: Make initializer check stricter during construction. ([#2531](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2531)) - * `metatx`: add a `BaseRelayRecipient` and a `MinimalForwarder` for meta-transactions ([#2508](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2508)) + * Meta Transactions: add `ERC2771Context` and a `MinimalForwarder` for meta-transactions. ([#2508](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2508)) ## 3.4.0 (2021-02-02) From d06cd393ffd02ae452f68dde17ce321732459b6d Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 19 Feb 2021 14:58:25 -0300 Subject: [PATCH 24/24] improve docs --- contracts/metatx/MinimalForwarder.sol | 2 +- contracts/metatx/README.adoc | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 contracts/metatx/README.adoc diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index f0b07bbb808..6c028ec049a 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -6,7 +6,7 @@ import "../cryptography/ECDSA.sol"; import "../drafts/EIP712.sol"; /* - * @dev Minimal forwarder for GSNv2 + * @dev Simple minimal forwarder to be used together with an ERC2771 compatible contract. See {ERC2771Context}. */ contract MinimalForwarder is EIP712 { using ECDSA for bytes32; diff --git a/contracts/metatx/README.adoc b/contracts/metatx/README.adoc new file mode 100644 index 00000000000..51a97913fa4 --- /dev/null +++ b/contracts/metatx/README.adoc @@ -0,0 +1,12 @@ += Meta Transactions + +[.readme-notice] +NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/api/math + +== Core + +{{ERC2771Context}} + +== Utils + +{{MinimalForwarder}}