From 9788d7300e6933577aa54154d49c0728c74569d6 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jan 2021 13:57:19 +0100 Subject: [PATCH 1/4] making Context._msgSender return "bytes calldata" --- contracts/GSN/GSNRecipient.sol | 52 +++++----------------------- contracts/mocks/GSNRecipientMock.sol | 2 +- contracts/utils/Context.sol | 2 +- 3 files changed, 10 insertions(+), 46 deletions(-) diff --git a/contracts/GSN/GSNRecipient.sol b/contracts/GSN/GSNRecipient.sol index cfde4763b2c..8393056f115 100644 --- a/contracts/GSN/GSNRecipient.sol +++ b/contracts/GSN/GSNRecipient.sol @@ -87,11 +87,11 @@ abstract contract GSNRecipient is IRelayRecipient, Context { * * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.sender`, and use {_msgSender} instead. */ - function _msgSender() internal view virtual override returns (address) { - if (msg.sender != getHubAddr()) { - return msg.sender; + function _msgSender() internal view virtual override returns (address msgSender) { + if (msg.sender == getHubAddr()) { + assembly { msgSender := shr(96, calldataload(sub(calldatasize(), 20))) } } else { - return _getRelayedCallSender(); + return msg.sender; } } @@ -101,11 +101,11 @@ abstract contract GSNRecipient is IRelayRecipient, Context { * * IMPORTANT: Contracts derived from {GSNRecipient} should never use `msg.data`, and use {_msgData} instead. */ - function _msgData() internal view virtual override returns (bytes memory) { - if (msg.sender != getHubAddr()) { - return msg.data; + function _msgData() internal view virtual override returns (bytes calldata) { + if (msg.sender == getHubAddr()) { + return msg.data[:msg.data.length - 20]; } else { - return _getRelayedCallData(); + return msg.data; } } @@ -191,40 +191,4 @@ abstract contract GSNRecipient is IRelayRecipient, Context { // charged for 1.4 times the spent amount. return (gas * gasPrice * (100 + serviceFee)) / 100; } - - 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/GSNRecipientMock.sol b/contracts/mocks/GSNRecipientMock.sol index 2b9c839640b..f0c70b79363 100644 --- a/contracts/mocks/GSNRecipientMock.sol +++ b/contracts/mocks/GSNRecipientMock.sol @@ -32,7 +32,7 @@ contract GSNRecipientMock is ContextMock, GSNRecipient { return GSNRecipient._msgSender(); } - function _msgData() internal override(Context, GSNRecipient) view virtual returns (bytes memory) { + function _msgData() internal override(Context, GSNRecipient) view virtual returns (bytes calldata) { return GSNRecipient._msgData(); } } diff --git a/contracts/utils/Context.sol b/contracts/utils/Context.sol index b8fca57b048..11e886f1080 100644 --- a/contracts/utils/Context.sol +++ b/contracts/utils/Context.sol @@ -17,7 +17,7 @@ abstract contract Context { return msg.sender; } - function _msgData() internal view virtual returns (bytes memory) { + function _msgData() internal view virtual returns (bytes calldata) { this; // silence state mutability warning without generating bytecode - see https://github.com/ethereum/solidity/issues/2691 return msg.data; } From 10e5aa4a056561b8e818e11598c05699aeb29308 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jan 2021 14:00:28 +0100 Subject: [PATCH 2/4] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b6f831f445..beaefb2a6d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ * `ERC165Checker`: added batch `getSupportedInterfaces`. ([#2469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2469)) * `RefundEscrow`: `beneficiaryWithdraw` will forward all available gas to the beneficiary. ([#2480](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2480)) * Many view and pure functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. + * `Context`: making `_msgSender` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) ### Security Fixes From 18e46d64d191b3d7260b01f29ea08c406efca610 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jan 2021 16:13:42 +0100 Subject: [PATCH 3/4] Context._msgData: fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index beaefb2a6d9..26df9a04ed4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ * `ERC165Checker`: added batch `getSupportedInterfaces`. ([#2469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2469)) * `RefundEscrow`: `beneficiaryWithdraw` will forward all available gas to the beneficiary. ([#2480](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2480)) * Many view and pure functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. - * `Context`: making `_msgSender` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) + * `Context`: making `_msgData` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) ### Security Fixes From ba7e4f8c2ead0b534ca0bfdbaf7bb83b8e773fce Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 29 Jan 2021 16:47:27 +0100 Subject: [PATCH 4/4] reorganise changelog to differenciate 3.4 and 4.0 changes --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26df9a04ed4..d32313a200f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +* `Context`: making `_msgData` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) + ## Unreleased * `BeaconProxy`: added new kind of proxy that allows simultaneous atomic upgrades. ([#2411](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2411)) @@ -16,7 +20,6 @@ * `ERC165Checker`: added batch `getSupportedInterfaces`. ([#2469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2469)) * `RefundEscrow`: `beneficiaryWithdraw` will forward all available gas to the beneficiary. ([#2480](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2480)) * Many view and pure functions have been made virtual to customize them via overrides. In many cases this will not imply that other functions in the contract will automatically adapt to the overridden definitions. People who wish to override should consult the source code to understand the impact and if they need to override any additional functions to achieve the desired behavior. - * `Context`: making `_msgData` return `bytes calldata` instead of `bytes memory` ([#2492](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2492)) ### Security Fixes