diff --git a/contracts/GSN/GSNContext.sol b/contracts/GSN/GSNContext.sol deleted file mode 100644 index e7ef137e0..000000000 --- a/contracts/GSN/GSNContext.sol +++ /dev/null @@ -1,88 +0,0 @@ -pragma solidity ^0.5.0; - -import "./Context.sol"; - -/** - * @dev Enables GSN support on `Context` contracts by recognizing calls from - * RelayHub and extracting the actual sender and call data from the received - * calldata. - * - * > This contract does not perform all required tasks to implement a GSN - * recipient contract: end users should use `GSNRecipient` instead. - */ -contract GSNContext is Context { - address internal _relayHub = 0xD216153c06E857cD7f72665E0aF1d7D82172F494; - - event RelayHubChanged(address indexed oldRelayHub, address indexed newRelayHub); - - constructor() internal { - // solhint-disable-previous-line no-empty-blocks - } - - function _upgradeRelayHub(address newRelayHub) internal { - address currentRelayHub = _relayHub; - require(newRelayHub != address(0), "GSNContext: new RelayHub is the zero address"); - require(newRelayHub != currentRelayHub, "GSNContext: new RelayHub is the current one"); - - emit RelayHubChanged(currentRelayHub, newRelayHub); - - _relayHub = newRelayHub; - } - - // Overrides for Context's functions: when called from RelayHub, sender and - // data require some pre-processing: the actual sender is stored at the end - // of the call data, which in turns means it needs to be removed from it - // when handling said data. - - function _msgSender() internal view returns (address) { - if (msg.sender != _relayHub) { - return msg.sender; - } else { - return _getRelayedCallSender(); - } - } - - function _msgData() internal view returns (bytes memory) { - if (msg.sender != _relayHub) { - return msg.data; - } else { - return _getRelayedCallData(); - } - } - - function _getRelayedCallSender() private pure returns (address result) { - // We need to read 20 bytes (an address) located at array index msg.data.length - 20. In memory, the array - // is prefixed with a 32-byte length value, so we first add 32 to get the memory read index. However, doing - // so would leave the address in the upper 20 bytes of the 32-byte word, which is inconvenient and would - // require bit shifting. We therefore subtract 12 from the read index so the address lands on the lower 20 - // bytes. This can always be done due to the 32-byte prefix. - - // The final memory read index is msg.data.length - 20 + 32 - 12 = msg.data.length. Using inline assembly is the - // easiest/most-efficient way to perform this operation. - - // These fields are not accessible from assembly - bytes memory array = msg.data; - uint256 index = msg.data.length; - - // solhint-disable-next-line no-inline-assembly - assembly { - // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those. - result := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff) - } - return result; - } - - function _getRelayedCallData() private pure returns (bytes memory) { - // RelayHub appends the sender address at the end of the calldata, so in order to retrieve the actual msg.data, - // we must strip the last 20 bytes (length of an address type) from it. - - uint256 actualDataLength = msg.data.length - 20; - bytes memory actualData = new bytes(actualDataLength); - - for (uint256 i = 0; i < actualDataLength; ++i) { - actualData[i] = msg.data[i]; - } - - return actualData; - } -} diff --git a/contracts/GSN/GSNRecipient.sol b/contracts/GSN/GSNRecipient.sol index 5c14cb77d..3f11170ad 100644 --- a/contracts/GSN/GSNRecipient.sol +++ b/contracts/GSN/GSNRecipient.sol @@ -1,9 +1,9 @@ pragma solidity ^0.5.0; import "./IRelayRecipient.sol"; -import "./GSNContext.sol"; -import "./bouncers/GSNBouncerBase.sol"; import "./IRelayHub.sol"; +import "./Context.sol"; +import "./bouncers/GSNBouncerBase.sol"; /** * @dev Base GSN recipient contract: includes the {IRelayRecipient} interface and enables GSN support on all contracts @@ -11,16 +11,42 @@ import "./IRelayHub.sol"; * * Not all interface methods are implemented (e.g. {acceptRelayedCall}, derived contracts must provide one themselves. */ -contract GSNRecipient is IRelayRecipient, GSNContext, GSNBouncerBase { +contract GSNRecipient is IRelayRecipient, Context, GSNBouncerBase { + // Default RelayHub address, deployed on mainnet and all testnets at the same address + address private _relayHub = 0xD216153c06E857cD7f72665E0aF1d7D82172F494; + /** - * @dev Returns the `RelayHub` address for this recipient contract. + * @dev Emitted when a contract changes its {IRelayHub} contract to a new one. + */ + event RelayHubChanged(address indexed oldRelayHub, address indexed newRelayHub); + + /** + * @dev Returns the address of the {IRelayHub} contract for this recipient. */ function getHubAddr() public view returns (address) { return _relayHub; } /** - * @dev Returns the version string of the `RelayHub` for which this recipient implementation was built. + * @dev Switches to a new {IRelayHub} instance. This method is added for future-proofing: there's no reason to not + * use the default instance. + * + * IMPORTANT: After upgrading, the {GSNRecipient} will no longer be able to receive relayed calls from the old + * {IRelayHub} instance. Additionally, all funds should be previously withdrawn via {_withdrawDeposits}. + */ + function _upgradeRelayHub(address newRelayHub) internal { + address currentRelayHub = _relayHub; + require(newRelayHub != address(0), "GSNRecipient: new RelayHub is the zero address"); + require(newRelayHub != currentRelayHub, "GSNRecipient: new RelayHub is the current one"); + + emit RelayHubChanged(currentRelayHub, newRelayHub); + + _relayHub = newRelayHub; + } + + /** + * @dev Returns the version string of the {IRelayHub} for which this recipient implementation was built. If + * {_upgradeRelayHub} is used, the new {IRelayHub} instance should be compatible with this version. */ // This function is view for future-proofing, it may require reading from // storage in the future. @@ -37,4 +63,73 @@ contract GSNRecipient is IRelayRecipient, GSNContext, GSNBouncerBase { function _withdrawDeposits(uint256 amount, address payable payee) internal { IRelayHub(_relayHub).withdraw(amount, payee); } + + // Overrides for Context's functions: when called from RelayHub, sender and + // data require some pre-processing: the actual sender is stored at the end + // of the call data, which in turns means it needs to be removed from it + // when handling said data. + + /** + * @dev Replacement for msg.sender. Returns the actual sender of a transaction: msg.sender for regular transactions, + * and the end-user for GSN relayed calls (where msg.sender is actually `RelayHub`). + * + * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.sender`, and use {_msgSender} instead. + */ + function _msgSender() internal view returns (address) { + if (msg.sender != _relayHub) { + return msg.sender; + } else { + return _getRelayedCallSender(); + } + } + + /** + * @dev Replacement for msg.data. Returns the actual calldata of a transaction: msg.data for regular transactions, + * and a reduced version for GSN relayed calls (where msg.data contains additional information). + * + * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.data`, and use {_msgData} instead. + */ + function _msgData() internal view returns (bytes memory) { + if (msg.sender != _relayHub) { + return msg.data; + } else { + return _getRelayedCallData(); + } + } + + function _getRelayedCallSender() private pure returns (address result) { + // We need to read 20 bytes (an address) located at array index msg.data.length - 20. In memory, the array + // is prefixed with a 32-byte length value, so we first add 32 to get the memory read index. However, doing + // so would leave the address in the upper 20 bytes of the 32-byte word, which is inconvenient and would + // require bit shifting. We therefore subtract 12 from the read index so the address lands on the lower 20 + // bytes. This can always be done due to the 32-byte prefix. + + // The final memory read index is msg.data.length - 20 + 32 - 12 = msg.data.length. Using inline assembly is the + // easiest/most-efficient way to perform this operation. + + // These fields are not accessible from assembly + bytes memory array = msg.data; + uint256 index = msg.data.length; + + // solhint-disable-next-line no-inline-assembly + assembly { + // Load the 32 bytes word from memory with the address on the lower 20 bytes, and mask those. + result := and(mload(add(array, index)), 0xffffffffffffffffffffffffffffffffffffffff) + } + return result; + } + + function _getRelayedCallData() private pure returns (bytes memory) { + // RelayHub appends the sender address at the end of the calldata, so in order to retrieve the actual msg.data, + // we must strip the last 20 bytes (length of an address type) from it. + + uint256 actualDataLength = msg.data.length - 20; + bytes memory actualData = new bytes(actualDataLength); + + for (uint256 i = 0; i < actualDataLength; ++i) { + actualData[i] = msg.data[i]; + } + + return actualData; + } } diff --git a/contracts/mocks/GSNContextMock.sol b/contracts/mocks/GSNContextMock.sol deleted file mode 100644 index bf1d688b1..000000000 --- a/contracts/mocks/GSNContextMock.sol +++ /dev/null @@ -1,46 +0,0 @@ -pragma solidity ^0.5.0; - -import "./ContextMock.sol"; -import "../GSN/GSNContext.sol"; -import "../GSN/IRelayRecipient.sol"; - -// By inheriting from GSNContext, Context's internal functions are overridden automatically -contract GSNContextMock is ContextMock, GSNContext, IRelayRecipient { - function getHubAddr() public view returns (address) { - return _relayHub; - } - - function acceptRelayedCall( - address, - address, - bytes calldata, - uint256, - uint256, - uint256, - uint256, - bytes calldata, - uint256 - ) - external - view - returns (uint256, bytes memory) - { - return (0, ""); - } - - function preRelayedCall(bytes calldata) external returns (bytes32) { - // solhint-disable-previous-line no-empty-blocks - } - - function postRelayedCall(bytes calldata, bool, uint256, bytes32) external { - // solhint-disable-previous-line no-empty-blocks - } - - function getRelayHub() public view returns (address) { - return _relayHub; - } - - function upgradeRelayHub(address newRelayHub) public { - return _upgradeRelayHub(newRelayHub); - } -} diff --git a/contracts/mocks/GSNRecipientMock.sol b/contracts/mocks/GSNRecipientMock.sol index 1ab11492b..f80e8bc6f 100644 --- a/contracts/mocks/GSNRecipientMock.sol +++ b/contracts/mocks/GSNRecipientMock.sol @@ -1,8 +1,10 @@ pragma solidity ^0.5.0; +import "./ContextMock.sol"; import "../GSN/GSNRecipient.sol"; -contract GSNRecipientMock is GSNRecipient { +// By inheriting from GSNRecipient, Context's internal functions are overridden automatically +contract GSNRecipientMock is ContextMock, GSNRecipient { function withdrawDeposits(uint256 amount, address payable payee) public { _withdrawDeposits(amount, payee); } @@ -22,4 +24,8 @@ contract GSNRecipientMock is GSNRecipient { function postRelayedCall(bytes calldata, bool, uint256, bytes32) external { // solhint-disable-previous-line no-empty-blocks } + + function upgradeRelayHub(address newRelayHub) public { + return _upgradeRelayHub(newRelayHub); + } } diff --git a/test/GSN/GSNContext.test.js b/test/GSN/GSNContext.test.js deleted file mode 100644 index f50828899..000000000 --- a/test/GSN/GSNContext.test.js +++ /dev/null @@ -1,78 +0,0 @@ -const { BN, constants, expectEvent, expectRevert } = require('openzeppelin-test-helpers'); -const { ZERO_ADDRESS } = constants; -const gsn = require('@openzeppelin/gsn-helpers'); - -const GSNContextMock = artifacts.require('GSNContextMock'); -const ContextMockCaller = artifacts.require('ContextMockCaller'); - -const { shouldBehaveLikeRegularContext } = require('./Context.behavior'); - -contract('GSNContext', function ([_, deployer, sender, newRelayHub]) { - beforeEach(async function () { - this.context = await GSNContextMock.new(); - this.caller = await ContextMockCaller.new(); - }); - - describe('get/set RelayHub', function () { - const singletonRelayHub = '0xD216153c06E857cD7f72665E0aF1d7D82172F494'; - - it('initially returns the singleton instance address', async function () { - expect(await this.context.getRelayHub()).to.equal(singletonRelayHub); - }); - - it('can be upgraded to a new RelayHub', async function () { - const { logs } = await this.context.upgradeRelayHub(newRelayHub); - expectEvent.inLogs(logs, 'RelayHubChanged', { oldRelayHub: singletonRelayHub, newRelayHub }); - }); - - it('cannot upgrade to the same RelayHub', async function () { - await expectRevert( - this.context.upgradeRelayHub(singletonRelayHub), - 'GSNContext: new RelayHub is the current one' - ); - }); - - it('cannot upgrade to the zero address', async function () { - await expectRevert(this.context.upgradeRelayHub(ZERO_ADDRESS), 'GSNContext: new RelayHub is the zero address'); - }); - - context('with new RelayHub', function () { - beforeEach(async function () { - await this.context.upgradeRelayHub(newRelayHub); - }); - - it('returns the new instance address', async function () { - expect(await this.context.getRelayHub()).to.equal(newRelayHub); - }); - }); - }); - - context('when called directly', function () { - shouldBehaveLikeRegularContext(sender); - }); - - context('when receiving a relayed call', function () { - beforeEach(async function () { - await gsn.fundRecipient(web3, { recipient: this.context.address }); - }); - - describe('msgSender', function () { - it('returns the relayed transaction original sender', async function () { - const { tx } = await this.context.msgSender({ from: sender, useGSN: true }); - await expectEvent.inTransaction(tx, GSNContextMock, 'Sender', { sender }); - }); - }); - - describe('msgData', function () { - it('returns the relayed transaction original data', async function () { - const integerValue = new BN('42'); - const stringValue = 'OpenZeppelin'; - const callData = this.context.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI(); - - // The provider doesn't properly estimate gas for a relayed call, so we need to manually set a higher value - const { tx } = await this.context.msgData(integerValue, stringValue, { gas: 1000000, useGSN: true }); - await expectEvent.inTransaction(tx, GSNContextMock, 'Data', { data: callData, integerValue, stringValue }); - }); - }); - }); -}); diff --git a/test/GSN/GSNRecipient.test.js b/test/GSN/GSNRecipient.test.js index 36bbc5478..d9f43f1f7 100644 --- a/test/GSN/GSNRecipient.test.js +++ b/test/GSN/GSNRecipient.test.js @@ -1,23 +1,94 @@ -const { balance, ether, expectRevert } = require('openzeppelin-test-helpers'); +const { balance, BN, constants, ether, expectEvent, expectRevert } = require('openzeppelin-test-helpers'); +const { ZERO_ADDRESS } = constants; + const gsn = require('@openzeppelin/gsn-helpers'); const { expect } = require('chai'); const GSNRecipientMock = artifacts.require('GSNRecipientMock'); +const ContextMockCaller = artifacts.require('ContextMockCaller'); -contract('GSNRecipient', function ([_, payee]) { +const { shouldBehaveLikeRegularContext } = require('./Context.behavior'); + +contract('GSNRecipient', function ([_, payee, sender, newRelayHub]) { beforeEach(async function () { this.recipient = await GSNRecipientMock.new(); }); - it('returns the RelayHub address address', async function () { - expect(await this.recipient.getHubAddr()).to.equal('0xD216153c06E857cD7f72665E0aF1d7D82172F494'); - }); - it('returns the compatible RelayHub version', async function () { expect(await this.recipient.relayHubVersion()).to.equal('1.0.0'); }); + describe('get/set RelayHub', function () { + const singletonRelayHub = '0xD216153c06E857cD7f72665E0aF1d7D82172F494'; + + it('initially returns the singleton instance address', async function () { + expect(await this.recipient.getHubAddr()).to.equal(singletonRelayHub); + }); + + it('can be upgraded to a new RelayHub', async function () { + const { logs } = await this.recipient.upgradeRelayHub(newRelayHub); + expectEvent.inLogs(logs, 'RelayHubChanged', { oldRelayHub: singletonRelayHub, newRelayHub }); + }); + + it('cannot upgrade to the same RelayHub', async function () { + await expectRevert( + this.recipient.upgradeRelayHub(singletonRelayHub), + 'GSNRecipient: new RelayHub is the current one' + ); + }); + + it('cannot upgrade to the zero address', async function () { + await expectRevert( + this.recipient.upgradeRelayHub(ZERO_ADDRESS), 'GSNRecipient: new RelayHub is the zero address' + ); + }); + + context('with new RelayHub', function () { + beforeEach(async function () { + await this.recipient.upgradeRelayHub(newRelayHub); + }); + + it('returns the new instance address', async function () { + expect(await this.recipient.getHubAddr()).to.equal(newRelayHub); + }); + }); + }); + + 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(sender); + }); + + context('when receiving a relayed call', function () { + beforeEach(async function () { + await gsn.fundRecipient(web3, { recipient: this.recipient.address }); + }); + + describe('msgSender', function () { + it('returns the relayed transaction original sender', async function () { + const { tx } = await this.recipient.msgSender({ from: sender, useGSN: true }); + await expectEvent.inTransaction(tx, GSNRecipientMock, 'Sender', { sender }); + }); + }); + + describe('msgData', function () { + it('returns the relayed transaction original data', async function () { + const integerValue = new BN('42'); + const stringValue = 'OpenZeppelin'; + const callData = this.recipient.contract.methods.msgData(integerValue.toString(), stringValue).encodeABI(); + + // The provider doesn't properly estimate gas for a relayed call, so we need to manually set a higher value + const { tx } = await this.recipient.msgData(integerValue, stringValue, { gas: 1000000, useGSN: true }); + await expectEvent.inTransaction(tx, GSNRecipientMock, 'Data', { data: callData, integerValue, stringValue }); + }); + }); + }); + context('with deposited funds', async function () { const amount = ether('1');