Add _msgSender() trick

[danfinlay]

Fixes #132 (If you'd want it!)

A more full description of the motivation is on that issue. Here is a quick pass achieving that proposal.

I did not change msg.sender instances in constructors, since the benefit would never apply in those scenarios.

Enables MetaTransaction facets and Delegatable.

[ItsNickBarry]

This seems useful. Same concept as GSN in the OZ contracts, correct? I didn't re-implement that feature because I've never used it myself.

Before merge, needs to be applied to a few more files: SafeOwnableInternal, ERC20ExtendedInternal, ERC20ImplicitApprovalInternal, ERC4626BaseInternal

[ItsNickBarry]

Can you describe these steps in more detail?

[ItsNickBarry]

I think it would be better to define this in a library in utils/, to improve re-usability and avoid code duplication.

[danfinlay]

That makes sense, I was thinking similarly.

[danfinlay]

Hi, thanks for the review!

Same concept as GSN in the OZ contracts, correct?

That's right. They should be completely compatible. I'll verify as much and get back here. And there are a few implementations going around that should all be compatible, so we can probably choose the most readable one. I'll gather the options.

Can you describe these steps in more detail?

I stole this code from someone who understood it better, but it is almost identical to GSN's version (formatted differently). Their comments are this:

    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;
    }

I'll add to the remaining files, while moving the implementation to a util soon, thank you! Feel free to close until I make those changes if you'd like to keep the PR queue neat.

[danfinlay]

Ok, I've made a few changes:

• Moved the _msgSender() implementations into a single util file.
• Copied the better-documented version of the _msgSender() that I linked to above.

Before merge, needs to be applied to a few more files: SafeOwnableInternal, ERC20ExtendedInternal, ERC20ImplicitApprovalInternal, ERC4626BaseInternal

• SafeOwnableInternal imports OwnableInternal, which now includes the change, so it is inherited.
• ERC20ExtendedInternal imports ERC20BaseInternal, which now includes the change, so it is inherited.
• ERC20ImplicitApprovalInternal imports ERC20BaseInternal, which now includes the change, so it is inherited.
• ERC4626BaseInternal imports ERC20BaseInternal, which now includes the change, so it is inherited.

If I were to block merge myself, I might suggest I add at least one test first to demonstrate reasonable usage of this method.

[zlace0x]

This is similar to OZ's Context.sol, but they don't implement _getRelayedCallSender by default since not all contracts would require meta tx. One can override the default with their desired metatx standard (e.g. ERC2771Context or ContextMixin)

I have not tested it, but this could affect existing contracts using multicall function since it would change the sender.

[danfinlay]

Oh that's interesting, does the multicall functinality rely on similarly appending to tx.data?

[zlace0x]

I'm looking for cases where msg.sender == address(this) might return true and return an unexpected sender.
If a contract calls itself with address(this).call without appending sender, _msgSender() will return an invalid sender address.

In the case of SS's Multicall, there won't be an issue since it uses delegatecall (msg.sender doesn't change to the contract address).

But other versions of Multicall use call for read-only queries; this can cause _msgSender() to report an unexpected value.