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

[wadeAlexC]

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]

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

[wadeAlexC]

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]

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]

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:

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]

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]

@wadeAlexC I think this should fix it: #1606

[wadeAlexC]

Looks good to me!