Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply the msgSender trick #132

Open
danfinlay opened this issue Aug 22, 2022 · 0 comments · May be fixed by #133
Open

Apply the msgSender trick #132

danfinlay opened this issue Aug 22, 2022 · 0 comments · May be fixed by #133

Comments

@danfinlay
Copy link

Hi, I'm a developer interested in enabling new authorization patterns, like metaTransactions and delegation.

Those approaches require authority-checking facets on the same diamond not use msg.sender directly, but instead call an internal method called _msgSender(), which allows internal methods to implement custom authorization code. This is sometimes called the msgSender() trick.

It looks like this:

    /*
     * @notice Overrides the msgSender to enable delegation message signing.
     * @returns address - The account whose authority is being acted on.
     */
    function _msgSender()
        internal
        view
        virtual
        override(DelegatableCore)
        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;
    }

Adding this change should not add any insecurity to any facets, other than any issues brought on by facets that perform some custom authorization logic to append the intended sender to the calldata:

    function _execute(
        address to,
        bytes memory data,
        uint256 gasLimit,
        address sender
    ) internal returns (bool success) {
        bytes memory full = abi.encodePacked(data, sender);
        assembly {
            success := call(gasLimit, to, 0, add(full, 0x20), mload(full), 0, 0)
        }
    }

Anyways, I know this is probably a sensitive change to propose, and I'd be willing to go to some measures to help it get security audited, but I wanted to know if you'd be interested in merging such a change, I think it would make these facets even more versatile.

If not, no hard feelings, I am happy to just start by making a simple fork for my own usage. I will PR my proposed changes either way so you can see what I am talking about.

danfinlay added a commit to delegatable/solidstate-solidity that referenced this issue Aug 22, 2022
@danfinlay danfinlay linked a pull request Aug 22, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant