From ccffe671054676b65a5d7f3434eb75b3237ff3dc Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Sun, 21 Aug 2022 22:26:03 -0700 Subject: [PATCH 1/3] Add _msgSender() trick Fixes #132 Enables MetaTransaction facets and [Delegatable](https://github.com/delegatable/delegatable-sol/blob/main/contracts/diamond/README.md). --- contracts/access/ownable/OwnableInternal.sol | 26 ++++++++-- contracts/security/PausableInternal.sol | 24 +++++++++- contracts/token/ERC1155/base/ERC1155Base.sol | 14 +++--- .../ERC1155/base/ERC1155BaseInternal.sol | 47 +++++++++++++++---- contracts/token/ERC20/base/ERC20Base.sol | 4 +- .../token/ERC20/base/ERC20BaseInternal.sol | 24 +++++++++- contracts/token/ERC721/base/ERC721Base.sol | 12 ++--- .../token/ERC721/base/ERC721BaseInternal.sol | 22 ++++++++- 8 files changed, 140 insertions(+), 33 deletions(-) diff --git a/contracts/access/ownable/OwnableInternal.sol b/contracts/access/ownable/OwnableInternal.sol index 49366e81..1b3bca8a 100644 --- a/contracts/access/ownable/OwnableInternal.sol +++ b/contracts/access/ownable/OwnableInternal.sol @@ -12,13 +12,13 @@ abstract contract OwnableInternal is IOwnableInternal { using OwnableStorage for OwnableStorage.Layout; modifier onlyOwner() { - require(msg.sender == _owner(), 'Ownable: sender must be owner'); + require(_msgSender() == _owner(), 'Ownable: sender must be owner'); _; } modifier onlyTransitiveOwner() { require( - msg.sender == _transitiveOwner(), + _msgSender() == _transitiveOwner(), 'Ownable: sender must be transitive owner' ); _; @@ -44,6 +44,26 @@ abstract contract OwnableInternal is IOwnableInternal { function _transferOwnership(address account) internal virtual { OwnableStorage.layout().setOwner(account); - emit OwnershipTransferred(msg.sender, account); + emit OwnershipTransferred(_msgSender(), account); + } + + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; } } diff --git a/contracts/security/PausableInternal.sol b/contracts/security/PausableInternal.sol index 3aec4f66..50e9077b 100644 --- a/contracts/security/PausableInternal.sol +++ b/contracts/security/PausableInternal.sol @@ -37,7 +37,7 @@ abstract contract PausableInternal { */ function _pause() internal virtual whenNotPaused { PausableStorage.layout().paused = true; - emit Paused(msg.sender); + emit Paused(_msgSender()); } /** @@ -45,6 +45,26 @@ abstract contract PausableInternal { */ function _unpause() internal virtual whenPaused { PausableStorage.layout().paused = false; - emit Unpaused(msg.sender); + emit Unpaused(_msgSender()); + } + + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; } } diff --git a/contracts/token/ERC1155/base/ERC1155Base.sol b/contracts/token/ERC1155/base/ERC1155Base.sol index 1df879a1..7356db72 100644 --- a/contracts/token/ERC1155/base/ERC1155Base.sol +++ b/contracts/token/ERC1155/base/ERC1155Base.sol @@ -73,13 +73,13 @@ abstract contract ERC1155Base is IERC1155Base, ERC1155BaseInternal { */ function setApprovalForAll(address operator, bool status) public virtual { require( - msg.sender != operator, + _msgSender() != operator, 'ERC1155: setting approval status for self' ); - ERC1155BaseStorage.layout().operatorApprovals[msg.sender][ + ERC1155BaseStorage.layout().operatorApprovals[_msgSender()][ operator ] = status; - emit ApprovalForAll(msg.sender, operator, status); + emit ApprovalForAll(_msgSender(), operator, status); } /** @@ -93,10 +93,10 @@ abstract contract ERC1155Base is IERC1155Base, ERC1155BaseInternal { bytes memory data ) public virtual { require( - from == msg.sender || isApprovedForAll(from, msg.sender), + from == _msgSender() || isApprovedForAll(from, _msgSender()), 'ERC1155: caller is not owner nor approved' ); - _safeTransfer(msg.sender, from, to, id, amount, data); + _safeTransfer(_msgSender(), from, to, id, amount, data); } /** @@ -110,9 +110,9 @@ abstract contract ERC1155Base is IERC1155Base, ERC1155BaseInternal { bytes memory data ) public virtual { require( - from == msg.sender || isApprovedForAll(from, msg.sender), + from == _msgSender() || isApprovedForAll(from, _msgSender()), 'ERC1155: caller is not owner nor approved' ); - _safeTransferBatch(msg.sender, from, to, ids, amounts, data); + _safeTransferBatch(_msgSender(), from, to, ids, amounts, data); } } diff --git a/contracts/token/ERC1155/base/ERC1155BaseInternal.sol b/contracts/token/ERC1155/base/ERC1155BaseInternal.sol index 63944275..71e9bee0 100644 --- a/contracts/token/ERC1155/base/ERC1155BaseInternal.sol +++ b/contracts/token/ERC1155/base/ERC1155BaseInternal.sol @@ -50,7 +50,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { require(account != address(0), 'ERC1155: mint to the zero address'); _beforeTokenTransfer( - msg.sender, + _msgSender(), address(0), account, _asSingletonArray(id), @@ -60,7 +60,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { ERC1155BaseStorage.layout().balances[id][account] += amount; - emit TransferSingle(msg.sender, address(0), account, id, amount); + emit TransferSingle(_msgSender(), address(0), account, id, amount); } /** @@ -79,7 +79,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { _mint(account, id, amount, data); _doSafeTransferAcceptanceCheck( - msg.sender, + _msgSender(), address(0), account, id, @@ -109,7 +109,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { ); _beforeTokenTransfer( - msg.sender, + _msgSender(), address(0), account, ids, @@ -127,7 +127,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { } } - emit TransferBatch(msg.sender, address(0), account, ids, amounts); + emit TransferBatch(_msgSender(), address(0), account, ids, amounts); } /** @@ -146,7 +146,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { _mintBatch(account, ids, amounts, data); _doSafeBatchTransferAcceptanceCheck( - msg.sender, + _msgSender(), address(0), account, ids, @@ -169,7 +169,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { require(account != address(0), 'ERC1155: burn from the zero address'); _beforeTokenTransfer( - msg.sender, + _msgSender(), account, address(0), _asSingletonArray(id), @@ -189,7 +189,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { balances[account] -= amount; } - emit TransferSingle(msg.sender, account, address(0), id, amount); + emit TransferSingle(_msgSender(), account, address(0), id, amount); } /** @@ -209,7 +209,14 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { 'ERC1155: ids and amounts length mismatch' ); - _beforeTokenTransfer(msg.sender, account, address(0), ids, amounts, ''); + _beforeTokenTransfer( + _msgSender(), + account, + address(0), + ids, + amounts, + '' + ); mapping(uint256 => mapping(address => uint256)) storage balances = ERC1155BaseStorage.layout().balances; @@ -225,7 +232,7 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { } } - emit TransferBatch(msg.sender, account, address(0), ids, amounts); + emit TransferBatch(_msgSender(), account, address(0), ids, amounts); } /** @@ -504,4 +511,24 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { uint256[] memory amounts, bytes memory data ) internal virtual {} + + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; + } } diff --git a/contracts/token/ERC20/base/ERC20Base.sol b/contracts/token/ERC20/base/ERC20Base.sol index 35d00328..b17d88cb 100644 --- a/contracts/token/ERC20/base/ERC20Base.sol +++ b/contracts/token/ERC20/base/ERC20Base.sol @@ -45,7 +45,7 @@ abstract contract ERC20Base is IERC20Base, ERC20BaseInternal { virtual returns (bool) { - return _approve(msg.sender, spender, amount); + return _approve(_msgSender(), spender, amount); } /** @@ -56,7 +56,7 @@ abstract contract ERC20Base is IERC20Base, ERC20BaseInternal { virtual returns (bool) { - return _transfer(msg.sender, recipient, amount); + return _transfer(_msgSender(), recipient, amount); } /** diff --git a/contracts/token/ERC20/base/ERC20BaseInternal.sol b/contracts/token/ERC20/base/ERC20BaseInternal.sol index e3aace4d..0f38c5ce 100644 --- a/contracts/token/ERC20/base/ERC20BaseInternal.sol +++ b/contracts/token/ERC20/base/ERC20BaseInternal.sol @@ -151,7 +151,7 @@ abstract contract ERC20BaseInternal is IERC20BaseInternal { address recipient, uint256 amount ) internal virtual returns (bool) { - uint256 currentAllowance = _allowance(holder, msg.sender); + uint256 currentAllowance = _allowance(holder, _msgSender()); require( currentAllowance >= amount, @@ -159,7 +159,7 @@ abstract contract ERC20BaseInternal is IERC20BaseInternal { ); unchecked { - _approve(holder, msg.sender, currentAllowance - amount); + _approve(holder, _msgSender(), currentAllowance - amount); } _transfer(holder, recipient, amount); @@ -179,4 +179,24 @@ abstract contract ERC20BaseInternal is IERC20BaseInternal { address to, uint256 amount ) internal virtual {} + + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; + } } diff --git a/contracts/token/ERC721/base/ERC721Base.sol b/contracts/token/ERC721/base/ERC721Base.sol index 22fe7342..343c61c9 100644 --- a/contracts/token/ERC721/base/ERC721Base.sol +++ b/contracts/token/ERC721/base/ERC721Base.sol @@ -61,7 +61,7 @@ abstract contract ERC721Base is IERC721Base, ERC721BaseInternal { ) public payable { _handleTransferMessageValue(from, to, tokenId, msg.value); require( - _isApprovedOrOwner(msg.sender, tokenId), + _isApprovedOrOwner(_msgSender(), tokenId), 'ERC721: transfer caller is not owner or approved' ); _transfer(from, to, tokenId); @@ -89,7 +89,7 @@ abstract contract ERC721Base is IERC721Base, ERC721BaseInternal { ) public payable { _handleTransferMessageValue(from, to, tokenId, msg.value); require( - _isApprovedOrOwner(msg.sender, tokenId), + _isApprovedOrOwner(_msgSender(), tokenId), 'ERC721: transfer caller is not owner or approved' ); _safeTransfer(from, to, tokenId, data); @@ -103,7 +103,7 @@ abstract contract ERC721Base is IERC721Base, ERC721BaseInternal { address owner = ownerOf(tokenId); require(operator != owner, 'ERC721: approval to current owner'); require( - msg.sender == owner || isApprovedForAll(owner, msg.sender), + _msgSender() == owner || isApprovedForAll(owner, _msgSender()), 'ERC721: approve caller is not owner nor approved for all' ); _approve(operator, tokenId); @@ -113,10 +113,10 @@ abstract contract ERC721Base is IERC721Base, ERC721BaseInternal { * @inheritdoc IERC721 */ function setApprovalForAll(address operator, bool status) public { - require(operator != msg.sender, 'ERC721: approve to caller'); - ERC721BaseStorage.layout().operatorApprovals[msg.sender][ + require(operator != _msgSender(), 'ERC721: approve to caller'); + ERC721BaseStorage.layout().operatorApprovals[_msgSender()][ operator ] = status; - emit ApprovalForAll(msg.sender, operator, status); + emit ApprovalForAll(_msgSender(), operator, status); } } diff --git a/contracts/token/ERC721/base/ERC721BaseInternal.sol b/contracts/token/ERC721/base/ERC721BaseInternal.sol index 8c1d7b62..810ed08d 100644 --- a/contracts/token/ERC721/base/ERC721BaseInternal.sol +++ b/contracts/token/ERC721/base/ERC721BaseInternal.sol @@ -179,7 +179,7 @@ abstract contract ERC721BaseInternal is IERC721Internal { bytes memory returnData = to.functionCall( abi.encodeWithSelector( IERC721Receiver(to).onERC721Received.selector, - msg.sender, + _msgSender(), from, tokenId, data @@ -229,4 +229,24 @@ abstract contract ERC721BaseInternal is IERC721Internal { address to, uint256 tokenId ) internal virtual {} + + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; + } } From a0b566ebeb3b7d4998dd4349ae822e72c3318700 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 25 Aug 2022 18:44:44 -0700 Subject: [PATCH 2/3] Move msgSender trick to utils file --- contracts/access/ownable/OwnableInternal.sol | 23 ++------------- contracts/security/PausableInternal.sol | 23 ++------------- .../ERC1155/base/ERC1155BaseInternal.sol | 23 ++------------- .../token/ERC20/base/ERC20BaseInternal.sol | 23 ++------------- .../token/ERC721/base/ERC721BaseInternal.sol | 23 ++------------- contracts/utils/MsgSenderTrick.sol | 28 +++++++++++++++++++ 6 files changed, 38 insertions(+), 105 deletions(-) create mode 100644 contracts/utils/MsgSenderTrick.sol diff --git a/contracts/access/ownable/OwnableInternal.sol b/contracts/access/ownable/OwnableInternal.sol index 1b3bca8a..b3596661 100644 --- a/contracts/access/ownable/OwnableInternal.sol +++ b/contracts/access/ownable/OwnableInternal.sol @@ -6,8 +6,9 @@ import { AddressUtils } from '../../utils/AddressUtils.sol'; import { IERC173 } from '../IERC173.sol'; import { IOwnableInternal } from './IOwnableInternal.sol'; import { OwnableStorage } from './OwnableStorage.sol'; +import { MsgSenderTrick } from '../../utils/MsgSenderTrick.sol'; -abstract contract OwnableInternal is IOwnableInternal { +abstract contract OwnableInternal is IOwnableInternal, MsgSenderTrick { using AddressUtils for address; using OwnableStorage for OwnableStorage.Layout; @@ -46,24 +47,4 @@ abstract contract OwnableInternal is IOwnableInternal { OwnableStorage.layout().setOwner(account); emit OwnershipTransferred(_msgSender(), account); } - - /* - * @notice Overrides the msgSender to enable delegation message signing. - * @returns address - The account whose authority is being acted on. - */ - function _msgSender() internal view virtual returns (address sender) { - if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } - } else { - sender = msg.sender; - } - return sender; - } } diff --git a/contracts/security/PausableInternal.sol b/contracts/security/PausableInternal.sol index 50e9077b..d696e43c 100644 --- a/contracts/security/PausableInternal.sol +++ b/contracts/security/PausableInternal.sol @@ -3,11 +3,12 @@ pragma solidity ^0.8.8; import { PausableStorage } from './PausableStorage.sol'; +import { MsgSenderTrick } from '../utils/MsgSenderTrick.sol'; /** * @title Internal functions for Pausable security control module. */ -abstract contract PausableInternal { +abstract contract PausableInternal is MsgSenderTrick { using PausableStorage for PausableStorage.Layout; event Paused(address account); @@ -47,24 +48,4 @@ abstract contract PausableInternal { PausableStorage.layout().paused = false; emit Unpaused(_msgSender()); } - - /* - * @notice Overrides the msgSender to enable delegation message signing. - * @returns address - The account whose authority is being acted on. - */ - function _msgSender() internal view virtual returns (address sender) { - if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } - } else { - sender = msg.sender; - } - return sender; - } } diff --git a/contracts/token/ERC1155/base/ERC1155BaseInternal.sol b/contracts/token/ERC1155/base/ERC1155BaseInternal.sol index 71e9bee0..4193809c 100644 --- a/contracts/token/ERC1155/base/ERC1155BaseInternal.sol +++ b/contracts/token/ERC1155/base/ERC1155BaseInternal.sol @@ -6,12 +6,13 @@ import { AddressUtils } from '../../../utils/AddressUtils.sol'; import { IERC1155Internal } from '../IERC1155Internal.sol'; import { IERC1155Receiver } from '../IERC1155Receiver.sol'; import { ERC1155BaseStorage } from './ERC1155BaseStorage.sol'; +import { MsgSenderTrick } from '../../../utils/MsgSenderTrick.sol'; /** * @title Base ERC1155 internal functions * @dev derived from https://github.com/OpenZeppelin/openzeppelin-contracts/ (MIT license) */ -abstract contract ERC1155BaseInternal is IERC1155Internal { +abstract contract ERC1155BaseInternal is IERC1155Internal, MsgSenderTrick { using AddressUtils for address; /** @@ -511,24 +512,4 @@ abstract contract ERC1155BaseInternal is IERC1155Internal { uint256[] memory amounts, bytes memory data ) internal virtual {} - - /* - * @notice Overrides the msgSender to enable delegation message signing. - * @returns address - The account whose authority is being acted on. - */ - function _msgSender() internal view virtual returns (address sender) { - if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } - } else { - sender = msg.sender; - } - return sender; - } } diff --git a/contracts/token/ERC20/base/ERC20BaseInternal.sol b/contracts/token/ERC20/base/ERC20BaseInternal.sol index 0f38c5ce..c64527bd 100644 --- a/contracts/token/ERC20/base/ERC20BaseInternal.sol +++ b/contracts/token/ERC20/base/ERC20BaseInternal.sol @@ -4,11 +4,12 @@ pragma solidity ^0.8.8; import { IERC20BaseInternal } from './IERC20BaseInternal.sol'; import { ERC20BaseStorage } from './ERC20BaseStorage.sol'; +import { MsgSenderTrick } from '../../../utils/MsgSenderTrick.sol'; /** * @title Base ERC20 internal functions, excluding optional extensions */ -abstract contract ERC20BaseInternal is IERC20BaseInternal { +abstract contract ERC20BaseInternal is IERC20BaseInternal, MsgSenderTrick { /** * @notice query the total minted token supply * @return token supply @@ -179,24 +180,4 @@ abstract contract ERC20BaseInternal is IERC20BaseInternal { address to, uint256 amount ) internal virtual {} - - /* - * @notice Overrides the msgSender to enable delegation message signing. - * @returns address - The account whose authority is being acted on. - */ - function _msgSender() internal view virtual returns (address sender) { - if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } - } else { - sender = msg.sender; - } - return sender; - } } diff --git a/contracts/token/ERC721/base/ERC721BaseInternal.sol b/contracts/token/ERC721/base/ERC721BaseInternal.sol index 810ed08d..868fc4f1 100644 --- a/contracts/token/ERC721/base/ERC721BaseInternal.sol +++ b/contracts/token/ERC721/base/ERC721BaseInternal.sol @@ -8,11 +8,12 @@ import { EnumerableSet } from '../../../utils/EnumerableSet.sol'; import { IERC721Internal } from '../IERC721Internal.sol'; import { IERC721Receiver } from '../IERC721Receiver.sol'; import { ERC721BaseStorage } from './ERC721BaseStorage.sol'; +import { MsgSenderTrick } from '../../../utils/MsgSenderTrick.sol'; /** * @title Base ERC721 internal functions */ -abstract contract ERC721BaseInternal is IERC721Internal { +abstract contract ERC721BaseInternal is IERC721Internal, MsgSenderTrick { using ERC721BaseStorage for ERC721BaseStorage.Layout; using AddressUtils for address; using EnumerableMap for EnumerableMap.UintToAddressMap; @@ -229,24 +230,4 @@ abstract contract ERC721BaseInternal is IERC721Internal { address to, uint256 tokenId ) internal virtual {} - - /* - * @notice Overrides the msgSender to enable delegation message signing. - * @returns address - The account whose authority is being acted on. - */ - function _msgSender() internal view virtual returns (address sender) { - if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } - } else { - sender = msg.sender; - } - return sender; - } } diff --git a/contracts/utils/MsgSenderTrick.sol b/contracts/utils/MsgSenderTrick.sol new file mode 100644 index 00000000..e9911248 --- /dev/null +++ b/contracts/utils/MsgSenderTrick.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.8; + +/** + * @title Utility contract for supporting alternative authorization schemes + */ +abstract contract MsgSenderTrick { + /* + * @notice Overrides the msgSender to enable delegation message signing. + * @returns address - The account whose authority is being acted on. + */ + function _msgSender() internal view virtual returns (address sender) { + if (msg.sender == address(this)) { + bytes memory array = msg.data; + uint256 index = msg.data.length; + assembly { + sender := and( + mload(add(array, index)), + 0xffffffffffffffffffffffffffffffffffffffff + ) + } + } else { + sender = msg.sender; + } + return sender; + } +} From 3e876b3c35cf9d99761468bbeee19647966d1c3d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 25 Aug 2022 19:25:00 -0700 Subject: [PATCH 3/3] Bring in the better documented version of _msgSender --- contracts/utils/MsgSenderTrick.sol | 44 ++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/contracts/utils/MsgSenderTrick.sol b/contracts/utils/MsgSenderTrick.sol index e9911248..bf84eabe 100644 --- a/contracts/utils/MsgSenderTrick.sol +++ b/contracts/utils/MsgSenderTrick.sol @@ -7,22 +7,48 @@ pragma solidity ^0.8.8; */ abstract contract MsgSenderTrick { /* - * @notice Overrides the msgSender to enable delegation message signing. + * @notice Returns the intended sender of a message. Either msg.sender or the address of the authorizing signer. + * Enables MetaTransactions, since the sender doesn't need to be the tx.origin or even the msg.sender. * @returns address - The account whose authority is being acted on. + * 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 virtual returns (address sender) { if (msg.sender == address(this)) { - bytes memory array = msg.data; - uint256 index = msg.data.length; - assembly { - sender := and( - mload(add(array, index)), - 0xffffffffffffffffffffffffffffffffffffffff - ) - } + return _getRelayedCallSender(); } else { sender = msg.sender; } return sender; } + + function _getRelayedCallSender() + private + pure + returns (address payable 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; + } }