# #37985 \[SC-Low] Incorrectly Eliminate Code With Side Effect In Slice Args

**Submitted on Dec 20th 2024 at 19:11:37 UTC by @anatomist for** [**Attackathon | Ethereum Protocol**](https://immunefi.com/audit-competition/ethereum-protocol-attackathon)

* **Report ID:** #37985
* **Report Type:** Smart Contract
* **Report severity:** Low
* **Target:** <https://github.com/vyperlang/vyper>
* **Impacts:**
  * (Compiler) Incorrect bytecode generation leading to incorrect behavior
  * (Compiler) Unexpected behavior

## Description

## Brief/Intro

The `slice` precompile skips `length != 0` check when the first argument is an `is_adhoc_slice`. However, if `length == 0`, the `make_setter` function will discard all operations except `mstore dst_size_field 0` regardless of any potential side effect, leading to incorrect compilation.

## Vulnerability Details

The slice buildin skips a lot of checks when the src to be slices is an `adhoc_slice` (`msg.data` or `(address).code`). Notably, the `length_literal < 1` check is skipped.

```
class Slice(BuiltinFunctionT):
    ...
    def fetch_call_return(self, node):
        ...
        arg = node.args[0]
        ...
        length_expr = node.args[2].reduced()

        # CMC 2022-03-22 NOTE slight code duplication with semantics/analysis/local
        is_adhoc_slice = arg.get("attr") == "code" or (
            arg.get("value.id") == "msg" and arg.get("attr") == "data"
        )

        start_literal = start_expr.value if isinstance(start_expr, vy_ast.Int) else None
        length_literal = length_expr.value if isinstance(length_expr, vy_ast.Int) else None

        if not is_adhoc_slice:
            if length_literal is not None:
                if length_literal < 1:
                    raise ArgumentException("Length cannot be less than 1", length_expr)

                if length_literal > arg_type.length:
                    raise ArgumentException(f"slice out of bounds for {arg_type}", length_expr)

            if start_literal is not None:
                if start_literal > arg_type.length:
                    raise ArgumentException(f"slice out of bounds for {arg_type}", start_expr)
                if length_literal is not None and start_literal + length_literal > arg_type.length:
                    raise ArgumentException(f"slice out of bounds for {arg_type}", node)
        ...
```

Thus the following code will compile

```
a: public(uint256)

@external
def test():
    b: Bytes[10]= slice(msg.data, self.test(), 0)

def side_effect() -> uint256:
    self.a += 1
    return 0 
```

During code generation, the IR for the slice builtin is created by the following snippet

```
def _build_adhoc_slice_node(sub: IRnode, start: IRnode, length: IRnode, context: Context) -> IRnode:
    assert length.is_literal, "typechecker failed"
    assert isinstance(length.value, int)  # mypy hint

    dst_typ = BytesT(length.value)
    # allocate a buffer for the return value
    buf = context.new_internal_variable(dst_typ)

    with scope_multi((start, length), ("start", "length")) as (b1, (start, length)):
        # `msg.data` by `calldatacopy`
        if sub.value == "~calldata":
            node = [
                "seq",
                check_buffer_overflow_ir(start, length, "calldatasize"),
                ["mstore", buf, length],
                ["calldatacopy", add_ofst(buf, 32), start, length],
                buf,
            ]
        elif sub.value == "~selfcode":
            ...
        else:
            ...
        ret = IRnode.from_list(node, typ=BytesT(length.value), location=MEMORY)
        return b1.resolve(ret)
```

And then returned to `parse_AnnAssign` as `rhs`

```
class Stmt:
    ...
    def parse_AnnAssign(self):
        ltyp = self.stmt.target._metadata["type"]
        varname = self.stmt.target.id
        lhs = self.context.new_variable(varname, ltyp)

        assert self.stmt.value is not None
        rhs = Expr(self.stmt.value, self.context).ir_node

        return make_setter(lhs, rhs)
    ...
```

Then `make_setter` is called to generate the final ir for this statement.

```
def make_setter(left, right, hi=None):
    check_assign(left, right)

    if potential_overlap(left, right):
        raise CompilerPanic("overlap between src and dst!")

    assert (hi is not None) == _dirty_read_risk(right)

    if left.typ._is_prim_word:
        ...

    elif isinstance(left.typ, _BytestringT):
        # TODO rethink/streamline the clamp_basetype logic
        if needs_clamp(right.typ, right.encoding):
            ...
        else:
            ret = make_byte_array_copier(left, right)

        return IRnode.from_list(ret)

    elif isinstance(left.typ, DArrayT):
        ...

    ...

def make_byte_array_copier(dst, src):
    assert isinstance(src.typ, _BytestringT)
    assert isinstance(dst.typ, _BytestringT)

    _check_assign_bytes(dst, src)

    if src.value == "~empty" or src.typ.maxlen == 0:
        # set length word to 0.
        return STORE(dst, 0)                #drops all rhs (src) ir if src.typ.maxlen == 0

    ...
```

Unfortunately, `rhs` (`slice` result) ir is completely dropped when its `src.typ.maxlen == 0`, along with any side effects that it may have. This in turn leads to incorrect compilation.

While we use `slice` as an entry point for this issue, we consider the actual root cause to be `make_setter` and `make_byte_array_copier` which eliminates ir without checking whether it contains side effect. There are potentially other entry points that we haven't explored yet, but can lead to the same issue.

Additionally, the `is_adhoc_slice` checker is also not precise, since it doesn't check whether the `code` attribute comes from an `address`, so if user defined a struct that contains a field calls `code`, then it'll also be treated as an `is_adhoc_slice` in `Slice.fetch_call_return`.

```
        is_adhoc_slice = arg.get("attr") == "code" or (
            arg.get("value.id") == "msg" and arg.get("attr") == "data"
        )
```

## Impact Details

Incorrectly optimizing out side effects can result in all kinds of unexpected behaviors in a contract, such as skipping critical checks performed in the incorrectly eliminated code, skipping important business logic, and more. This exposes contract developers to substantial risks.

## References

* `https://github.com/vyperlang/vyper/blob/12ab4919cc4618fcac4f5d24d45a0e7fdbc4a48c/vyper/builtins/functions.py#L320`
* `https://github.com/vyperlang/vyper/blob/12ab4919cc4618fcac4f5d24d45a0e7fdbc4a48c/vyper/builtins/functions.py#L247`
* `https://github.com/vyperlang/vyper/blob/12ab4919cc4618fcac4f5d24d45a0e7fdbc4a48c/vyper/codegen/stmt.py#L63`
* `https://github.com/vyperlang/vyper/blob/12ab4919cc4618fcac4f5d24d45a0e7fdbc4a48c/vyper/codegen/core.py#L983`
* `https://github.com/vyperlang/vyper/blob/12ab4919cc4618fcac4f5d24d45a0e7fdbc4a48c/vyper/codegen/core.py#L160`

## Proof of Concept

## Proof of Concept

Already shown in Vulnerability Details section.


---

# Agent Instructions: Querying This Documentation

If you need additional information that is not directly available in this page, you can query the documentation dynamically by asking a question.

Perform an HTTP GET request on the current page URL with the `ask` query parameter:

```
GET https://reports.immunefi.com/ethereum-protocol-or-attackathon/37985-sc-low-incorrectly-eliminate-code-with-side-effect-in-slice-args.md?ask=<question>
```

The question should be specific, self-contained, and written in natural language.
The response will contain a direct answer to the question and relevant excerpts and sources from the documentation.

Use this mechanism when the answer is not explicitly present in the current page, you need clarification or additional context, or you want to retrieve related documentation sections.
