fix: calldatasize < 4 reverting instead of going to fallback (#3408)

in the case that a selector matches calldata with less than 4 bytes, it
currently will revert instead of going to the fallback. this can happen
if the selector has trailing zeroes. this commit fixes the behavior for
selectors with trailing zeroes, and improves code size and gas for those
without trailing zeroes.

tests/parser/functions/test_default_function.py

@@ -100,7 +100,9 @@ def __default__():
     assert_compile_failed(lambda: get_contract_with_gas_estimation(code))
 
 
-def test_zero_method_id(w3, get_logs, get_contract_with_gas_estimation):
+def test_zero_method_id(w3, get_logs, get_contract, assert_tx_failed):
+    # test a method with 0x00000000 selector,
+    # expects at least 36 bytes of calldata.
     code = """
 event Sent:
     sig: uint256
@@ -116,18 +118,108 @@ def blockHashAskewLimitary(v: uint256) -> uint256:
 def __default__():
     log Sent(1)
     """
-
-    c = get_contract_with_gas_estimation(code)
+    c = get_contract(code)
 
     assert c.blockHashAskewLimitary(0) == 7
 
-    logs = get_logs(w3.eth.send_transaction({"to": c.address, "value": 0}), c, "Sent")
-    assert 1 == logs[0].args.sig
+    def _call_with_bytes(hexstr):
+        # call our special contract and return the logged value
+        logs = get_logs(
+            w3.eth.send_transaction({"to": c.address, "value": 0, "data": hexstr}), c, "Sent"
+        )
+        return logs[0].args.sig
 
-    logs = get_logs(
-        # call blockHashAskewLimitary
-        w3.eth.send_transaction({"to": c.address, "value": 0, "data": "0x" + "00" * 36}),
-        c,
-        "Sent",
-    )
-    assert 2 == logs[0].args.sig
+    assert 1 == _call_with_bytes("0x")
+
+    # call blockHashAskewLimitary with proper calldata
+    assert 2 == _call_with_bytes("0x" + "00" * 36)
+
+    # call blockHashAskewLimitary with extra trailing bytes in calldata
+    assert 2 == _call_with_bytes("0x" + "00" * 37)
+
+    for i in range(4):
+        # less than 4 bytes of calldata doesn't match the 0 selector and goes to default
+        assert 1 == _call_with_bytes("0x" + "00" * i)
+
+    for i in range(4, 36):
+        # match the full 4 selector bytes, but revert due to malformed (short) calldata
+        assert_tx_failed(lambda: _call_with_bytes("0x" + "00" * i))
+
+
+def test_another_zero_method_id(w3, get_logs, get_contract, assert_tx_failed):
+    # test another zero method id but which only expects 4 bytes of calldata
+    code = """
+event Sent:
+    sig: uint256
+
+@external
+@payable
+# function selector: 0x00000000
+def wycpnbqcyf() -> uint256:
+    log Sent(2)
+    return 7
+
+@external
+def __default__():
+    log Sent(1)
+    """
+    c = get_contract(code)
+
+    assert c.wycpnbqcyf() == 7
+
+    def _call_with_bytes(hexstr):
+        # call our special contract and return the logged value
+        logs = get_logs(
+            w3.eth.send_transaction({"to": c.address, "value": 0, "data": hexstr}), c, "Sent"
+        )
+        return logs[0].args.sig
+
+    assert 1 == _call_with_bytes("0x")
+
+    # call wycpnbqcyf
+    assert 2 == _call_with_bytes("0x" + "00" * 4)
+
+    # too many bytes ok
+    assert 2 == _call_with_bytes("0x" + "00" * 5)
+
+    # "right" method id but by accident - not enough bytes.
+    for i in range(4):
+        assert 1 == _call_with_bytes("0x" + "00" * i)
+
+
+def test_partial_selector_match_trailing_zeroes(w3, get_logs, get_contract):
+    code = """
+event Sent:
+    sig: uint256
+
+@external
+@payable
+# function selector: 0xd88e0b00
+def fow() -> uint256:
+    log Sent(2)
+    return 7
+
+@external
+def __default__():
+    log Sent(1)
+    """
+    c = get_contract(code)
+
+    # sanity check - we can call c.fow()
+    assert c.fow() == 7
+
+    def _call_with_bytes(hexstr):
+        # call our special contract and return the logged value
+        logs = get_logs(
+            w3.eth.send_transaction({"to": c.address, "value": 0, "data": hexstr}), c, "Sent"
+        )
+        return logs[0].args.sig
+
+    # check we can call default function
+    assert 1 == _call_with_bytes("0x")
+
+    # check fow() selector is 0xd88e0b00
+    assert 2 == _call_with_bytes("0xd88e0b00")
+
+    # check calling d88e0b with no trailing zero goes to fallback instead of reverting
+    assert 1 == _call_with_bytes("0xd88e0b")

vyper/codegen/function_definitions/external_function.py

@@ -88,7 +88,12 @@ def handler_for(calldata_kwargs, default_kwargs):
         # ensure calldata is at least of minimum length
         args_abi_t = calldata_args_t.abi_type
         calldata_min_size = args_abi_t.min_size() + 4
-        ret.append(["assert", ["ge", "calldatasize", calldata_min_size]])
+
+        # note we don't need the check if calldata_min_size == 4,
+        # because the selector checks later in this routine ensure
+        # that calldatasize >= 4.
+        if calldata_min_size > 4:
+            ret.append(["assert", ["ge", "calldatasize", calldata_min_size]])
 
         # TODO optimize make_setter by using
         # TupleT(list(arg.typ for arg in calldata_kwargs + default_kwargs))
@@ -124,20 +129,26 @@ def handler_for(calldata_kwargs, default_kwargs):
 
         method_id_check = ["eq", "_calldata_method_id", method_id]
 
-        # if there is a function whose selector is 0, it won't be distinguished
-        # from the case where nil calldata is supplied, b/c calldataload loads
-        # 0s past the end of physical calldata (cf. yellow paper).
-        # since supplying 0 calldata is expected to trigger the fallback fn,
-        # we check that calldatasize > 0, which distinguishes the 0 selector
-        # from the fallback function "selector"
-        # (equiv. to "all selectors not in the selector table").
-
-        # note: cases where not enough calldata is supplied (besides
-        # calldatasize==0) are not addressed here b/c a calldatasize
-        # well-formedness check is already present in the function body
-        # as part of abi validation
-        if method_id.value == 0:
-            method_id_check = ["and", ["gt", "calldatasize", 0], method_id_check]
+        # if there is a function whose selector is 0 or has trailing 0s, it
+        # might not be distinguished from the case where insufficient calldata
+        # is supplied, b/c calldataload loads 0s past the end of physical
+        # calldata (cf. yellow paper).
+        # since the expected behavior of supplying insufficient calldata
+        # is to trigger the fallback fn, we add to the selector check that
+        # calldatasize >= 4, which distinguishes any selector with trailing
+        # 0 bytes from the fallback function "selector" (equiv. to "all
+        # selectors not in the selector table").
+        #
+        # note that the inclusion of this check means that, we are always
+        # guaranteed that the calldata is at least 4 bytes - either we have
+        # the explicit `calldatasize >= 4` condition in the selector check,
+        # or there are no trailing zeroes in the selector, (so the selector
+        # is impossible to match without calldatasize being at least 4).
+        method_id_bytes = util.method_id(abi_sig)
+        assert len(method_id_bytes) == 4
+        has_trailing_zeroes = method_id_bytes.endswith(b"\x00")
+        if has_trailing_zeroes:
+            method_id_check = ["and", ["ge", "calldatasize", 4], method_id_check]
 
         ret = ["if", method_id_check, ret]
         return ret