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

VIP: Restrict usage of `msg` fields in public functions #1199

Closed
charles-cooper opened this issue Jan 15, 2019 · 11 comments · Fixed by #1582
Closed

VIP: Restrict usage of `msg` fields in public functions #1199

charles-cooper opened this issue Jan 15, 2019 · 11 comments · Fixed by #1582

Comments

@charles-cooper
Copy link
Collaborator

@charles-cooper charles-cooper commented Jan 15, 2019

Simple Summary

Disallow using the msg context in public functions which are called internally.

Abstract

This is kind of a follow-up to the discussion in #901. The currently implemented behavior is to disallow msg.sender in private functions, forcing the caller to pass it as an explicit function argument. The behavior proposed here is to extend this restriction to public functions which are called internally, and also to extend it to the entire msg context. If a user really wants to call an internal @public function which accesses the msg context, they are forced to refactor the function to pass msg.sender explicitly (and optionally change it to @private).

Motivation

msg.sender can have different values depending on if the function call is generated from an external caller or an internal caller. For instance,

@public
def call1() -> address:
  return msg.sender
@private
def call2(addr: address) -> address:
  return addr  # Can't access `msg.sender` here, must pass as arg
@public
def call3() -> address:
  return call1()
@public
def call4() -> address:
  return call2(msg.sender)

In this example, call1 and call4 return the address of the caller, but call3 returns the address of this contract. This goes against Vyper's principle of auditability since extra context needs to be reasoned about when reading code. This proposal would force the above implementation of call1 to be changed to pass msg.sender as an argument, mimicking call2.

Additionally, the current behavior is an extra point of confusion for users coming from Solidity because Vyper's @public is more analogous to Solidity's external keyword than Solidity's public keyword (Solidity has a fine-grained distinction between public and external - public uses a jump which does not change the msg context, while external uses a message call).

Specification

In addition to disallowing msg.sender in private functions, additionally disallow any msg values (off the top of my head: msg.sender, msg.value, proposed msg.data) in internal calls to public functions by raising an exception.

Backwards Compatibility

msg.sender and friends are no longer allowed in public functions which are called internally.

Copyright

Copyright and related rights waived via CC0

@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented Jan 15, 2019

An alternative to this proposal would be to only allow access to these variables in @external (similar to Solidity) as that context is meant to exclude access to other contract functions.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator Author

@charles-cooper charles-cooper commented Jan 24, 2019

Tentative conclusion from gitter the other day was to disallow self calls to public functions entirely

@fubuloubu fubuloubu added this to To do in Release Candidate! via automation Jan 24, 2019
@fubuloubu fubuloubu removed this from To do in Release Candidate! Jan 28, 2019
@jacqueswww jacqueswww added post beta Approved and removed for rc labels Jan 28, 2019
@jacqueswww

This comment has been minimized.

Copy link
Collaborator

@jacqueswww jacqueswww commented Jan 28, 2019

Calls to self if functions are public, will be prohibited.

@iamdefinitelyahuman

This comment has been minimized.

Copy link
Contributor

@iamdefinitelyahuman iamdefinitelyahuman commented Aug 4, 2019

What about the following pattern for public => public calls in the same contract?

contract FooBar:
    def foo() -> uint256: constant

@public
def foo() -> uint256:
    return 42

@public
def bar() -> uint256:
    return FooBar(self).foo()

Currently this code compiles, but calls to bar revert.

To me it feels much more explicit than simply using self, I would fix the revert and document this as the proper way to access public functions within the same contract. On the other hand, if this behavior shouldn't be allowed, I feel this code should fail to compile with a "cannot create an interface over self" error message.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator Author

@charles-cooper charles-cooper commented Aug 4, 2019

Calls to bar shouldn't really revert there .. that seems like a bug

@jacqueswww

This comment has been minimized.

Copy link
Collaborator

@jacqueswww jacqueswww commented Aug 4, 2019

@CharlesCooper do we know why this reverts ? 🤔

@charles-cooper

This comment has been minimized.

Copy link
Collaborator Author

@charles-cooper charles-cooper commented Aug 5, 2019

@jacqueswww

        # Line 8
        [if,
          [eq, [mload, 0], '4273672062' <bar()>],
          [seq,
            [assert, [iszero, callvalue]],
            # Line 10
            [mstore,
              0,
              [mload,
                [seq,
                  [assert, [extcodesize, address]],
# probably this test:
                  [assert, [xor, address, address]],
                  [assert, [staticcall, gas, address, [seq, [mstore, 320, 3264763256], 348], 4, 416, 32]],
                  0,
                  416]]],
            [seq_unchecked, pass, [return, 0, 32]],
            # Line 8
            stop]],
@davesque

This comment has been minimized.

Copy link
Contributor

@davesque davesque commented Aug 5, 2019

@charles-cooper @jacqueswww Looking at where that LLL is generated makes it a bit more clear that it's specifically preventing an "external" call to the current address: https://github.com/ethereum/vyper/blob/76afcb1f599704b748b16272924149a472623a74/vyper/parser/external_call.py#L70

There may be a good reason to prevent that, but I can't think of one off the top of my head. Anyone have any ideas? It's also not clear from the commit history why that LLL is part of the output.

@davesque

This comment has been minimized.

Copy link
Contributor

@davesque davesque commented Aug 5, 2019

Actually, here's the original commit that introduced that restriction: 9220715

@davesque

This comment has been minimized.

Copy link
Contributor

@davesque davesque commented Aug 5, 2019

Actually, I may as well ping @DavidKnott here to see if he has any recollection of why this was introduced.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator Author

@charles-cooper charles-cooper commented Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.