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

Function selector collision is not detected with the "0" selector #1603

Closed
wadeAlexC opened this issue Sep 8, 2019 · 7 comments · Fixed by #1606

Comments

@wadeAlexC
Copy link

commented Sep 8, 2019

Version Information

  • vyper Version (output of vyper --version): 0.1.0-beta.12

What's your issue about?

Vyper has an edge case in function selector collision detection. If a function with selector 0x00000000 exists, the __default__ function is overwritten in the resulting contract. Here's an example:

owner: public(address)

@public
def __init__():
    self.owner = msg.sender

@public
@payable
def blockHashAskewLimitary(v: uint256): # function selector: 0x00000000
    send(self.owner, self.balance)

@public
@payable
def __default__():
    assert msg.value != 0
    send(msg.sender, self.balance)

Calls made to the above contract with no calldata trigger blockHashAskewLimitary(uint), rather than the default function. It seems that v is also successfully decoded, even without calldata.

How can it be fixed?

Solidity uses CALLDATASIZE to differentiate between functions with the "0" selector, and the fallback function. As for the variable unpacking from empty calldata, a check should be in place that each function has some minimum CALLDATASIZE.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

We should definitely check for collisions between function selectors and reject programs with collisions at compile time!

@charles-cooper charles-cooper added the bug label Sep 8, 2019
@wadeAlexC

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

It seems Vyper does this -- the only edge case I've found is the "0" selector. Note, though, that Solidity does allow a function with a "0" selector along with a fallback function. It differentiates between the two by checking whether CALLDATASIZE is less than 4.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

It seems Vyper does this -- the only edge case I've found is the "0" selector. Note, though, that Solidity does allow a function with a "0" selector along with a fallback function. It differentiates between the two by checking whether CALLDATASIZE is less than 4.

Thanks - I think we will just reject programs with name that maps to the "0" selector

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

Actually I see the issue. Consider the IR of the following program:

@public
@payable
def blockHashAskewLimitary(v: uint256): # function selector: 0x00000000
  send(msg.sender, self.balance)

@public
def __default__() -> uint256:
  return 1
[seq,
  [return,
    0,
    [lll,
      [seq,
        [mstore, 28, [calldataload, 0]],
        [mstore, 32, 1461501637330902918203684832716283019655932542976],
        [mstore, 64, 170141183460469231731687303715884105727],
        [mstore, 96, -170141183460469231731687303715884105728],
        [mstore, 128, 1701411834604692317316873037158841057270000000000],
        [mstore, 160, -1701411834604692317316873037158841057280000000000],
        # Line 1
        [if,
          [iszero, [mload, 0]],
          [seq,
            # Line 4
            [assert, [call, 0, caller, [balance, address], 0, 0, 0, 0]],
            # Line 1
            stop]],
        # Line 6
        [assert, [iszero, callvalue]],
        # Line 8
        [mstore, 0, 1],
        [seq_unchecked, pass, [return, 0, 32]]],
      0]]]

The fallback function doesn't have a selector. Actually the blockHashAskewLimitary is getting selected because the result of (mload 0) (which in turn contains the result of (calldataload 0)) is 0. This is because of the semantics of CALLDATALOAD:
Screenshot from 2019-09-08 13-19-31

If the argument to CALLDATALOAD refers to a location larger than (or equal to) CALLDATASIZE then CALLDATALOAD returns 0 (rather than reverting). So as you suggested, we need to check that CALLDATASIZE is at least equal to 4, otherwise jump to the fallback clause.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

As an aside, this provides us a clean and gas-efficient way to perform memory zeroing without loops. We can just use (calldatacopy calldatasize mem_dest len). This is a base cost of 3 gas + 3 gas per word copied.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

@wadeAlexC I think this should fix it: #1606

@wadeAlexC

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

Looks good to me!

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