CVE-2023-31146: Out-of-bounds Write (CWE-787)
value: Vyper is a Pythonic smart contract language for the Ethereum virtual machine. Prior to version 0.3.8, during codegen, the length word of a dynarray is written before the data, which can result in out-of-bounds array access in the case where the dynarray is on both the lhs and rhs of an assignment. The issue can cause data corruption across call frames. The expected behavior is to revert due to out-of-bounds array access. Version 0.3.8 contains a patch for this issue.
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.1
Published: 2023-05-11T21:15Z
Repository: https://github.com/vyperlang/vyper
Language: Python
Files changed: tests/parser/types/test_dynamic_array.py, vyper/codegen/core.py
Commit date: 2023-05-11 13:30:00 -0700
Commit message: Merge pull request from GHSA-3p37-3636-q8wv

in dynarray_make_setter, the length is copied before the data. when the
dst and src arrays do not overlap, this is not a problem. however, when
the dst and src are the same dynarray, this can lead to a
store-before-load, leading any array bounds checks on the right hand
side to function incorrectly. here is an example:

```vyper
@external
def should_revert() -> DynArray[uint256,3]:
    a: DynArray[uint256, 3] = [1, 2, 3]
    a = empty(DynArray[uint256, 3])
    a = [self.a[0], self.a[1], self.a[2]]
    return a  # if bug: returns [1,2,3]
```

this commit moves the length store to after the data copy in
dynarray_make_setter. for hygiene, it also moves the length store to
after the data copy in several other routines. I left pop_dyn_array()
unchanged, because moving the routine does not actually perform any data
copy, it just writes the new length (and optionally returns a pointer to
the popped item).
Diff: diff --git a/tests/parser/types/test_dynamic_array.py b/tests/parser/types/test_dynamic_array.py
index 04c068824..cb55c4287 100644
--- a/tests/parser/types/test_dynamic_array.py
+++ b/tests/parser/types/test_dynamic_array.py
@@ -1746,5 +1746,97 @@ MY_CONSTANT: constant(DynArray[{storage_type}, 3]) = [1, 2, 3]
 @external
 def foo(i: uint256) -> {return_type}:
     return MY_CONSTANT[i]
     """
     assert_compile_failed(lambda: get_contract(code), TypeMismatch)
+
+
+dynarray_length_no_clobber_cases = [
+    # GHSA-3p37-3636-q8wv cases
+    """
+a: DynArray[uint256,3]
+
+@external
+def should_revert() -> DynArray[uint256,3]:
+    self.a = [1,2,3]
+    self.a = empty(DynArray[uint256,3])
+    self.a = [self.a[0], self.a[1], self.a[2]]
+
+    return self.a  # if bug: returns [1,2,3]
+    """,
+    """
+@external
+def should_revert() -> DynArray[uint256,3]:
+    self.a()
+    return self.b() # if bug: returns [1,2,3]
+
+@internal
+def a():
+    a: uint256 = 0
+    b: uint256 = 1
+    c: uint256 = 2
+    d: uint256 = 3
+
+@internal
+def b() -> DynArray[uint256,3]:
+    a: DynArray[uint256,3] = empty(DynArray[uint256,3])
+    a = [a[0],a[1],a[2]]
+    return a
+    """,
+    """
+a: DynArray[uint256,4]
+
+@external
+def should_revert() -> DynArray[uint256,4]:
+    self.a = [1,2,3]
+    self.a = empty(DynArray[uint256,4])
+    self.a = [4, self.a[0]]
+
+    return self.a  # if bug: return [4, 4]
+    """,
+    """
+@external
+def should_revert() -> DynArray[uint256,4]:
+    a: DynArray[uint256, 4] = [1,2,3]
+    a = []
+
+    a = [a.pop()]  # if bug: return [1]
+
+    return a
+    """,
+    """
+@external
+def should_revert():
+    c: DynArray[uint256, 1] = []
+    c.append(c[0])
+    """,
+    """
+@external
+def should_revert():
+    c: DynArray[uint256, 1] = [1]
+    c[0] = c.pop()
+    """,
+    """
+@external
+def should_revert():
+    c: DynArray[DynArray[uint256, 1], 2] = [[]]
+    c[0] = c.pop()
+    """,
+    """
+a: DynArray[String[65],2]
+
+@external
+def should_revert() -> DynArray[String[65], 2]:
+    self.a = ["hello", "world"]
+    self.a = []
+    self.a = [self.a[0], self.a[1]]
+
+    return self.a  # if bug: return ["hello", "world"]
+    """,
+]
+
+
+@pytest.mark.parametrize("code", dynarray_length_no_clobber_cases)
+def test_dynarray_length_no_clobber(get_contract, assert_tx_failed, code):
+    # check that length is not clobbered before dynarray data copy happens
+    c = get_contract(code)
+    assert_tx_failed(lambda: c.should_revert())
diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py
index 86fe19818..a9a91ec9d 100644
--- a/vyper/codegen/core.py
+++ b/vyper/codegen/core.py
@@ -115,17 +115,19 @@ def make_byte_array_copier(dst, src):
     with src.cache_when_complex("src") as (b1, src):
         with get_bytearray_length(src).cache_when_complex("len") as (b2, len_):
             max_bytes = src.typ.maxlen
 
             ret = ["seq"]
+
+            dst_ = bytes_data_ptr(dst)
+            src_ = bytes_data_ptr(src)
+
+            ret.append(copy_bytes(dst_, src_, len_, max_bytes))
+
             # store length
             ret.append(STORE(dst, len_))
 
-            dst = bytes_data_ptr(dst)
-            src = bytes_data_ptr(src)
-
-            ret.append(copy_bytes(dst, src, len_, max_bytes))
             return b1.resolve(b2.resolve(ret))
 
 
 def bytes_data_ptr(ptr):
     if ptr.location is None:
@@ -146,29 +148,38 @@ def _dynarray_make_setter(dst, src):
     assert isinstance(dst.typ, DArrayT)
 
     if src.value == "~empty":
         return IRnode.from_list(STORE(dst, 0))
 
+    # copy contents of src dynarray to dst.
+    # note that in case src and dst refer to the same dynarray,
+    # in order for get_element_ptr oob checks on the src dynarray
+    # to work, we need to wait until after the data is copied
+    # before we clobber the length word.
+
     if src.value == "multi":
         ret = ["seq"]
         # handle literals
 
-        # write the length word
-        store_length = STORE(dst, len(src.args))
-        ann = None
-        if src.annotation is not None:
-            ann = f"len({src.annotation})"
-        store_length = IRnode.from_list(store_length, annotation=ann)
-        ret.append(store_length)
-
+        # copy each item
         n_items = len(src.args)
+
         for i in range(n_items):
             k = IRnode.from_list(i, typ=UINT256_T)
             dst_i = get_element_ptr(dst, k, array_bounds_check=False)
             src_i = get_element_ptr(src, k, array_bounds_check=False)
             ret.append(make_setter(dst_i, src_i))
 
+        # write the length word after data is copied
+        store_length = STORE(dst, n_items)
+        ann = None
+        if src.annotation is not None:
+            ann = f"len({src.annotation})"
+        store_length = IRnode.from_list(store_length, annotation=ann)
+
+        ret.append(store_length)
+
         return ret
 
     with src.cache_when_complex("darray_src") as (b1, src):
         # for ABI-encoded dynamic data, we must loop to unpack, since
         # the layout does not match our memory layout
@@ -188,12 +199,10 @@ def _dynarray_make_setter(dst, src):
         should_loop |= src.typ.value_type.abi_type.is_dynamic()
 
         with get_dyn_array_count(src).cache_when_complex("darray_count") as (b2, count):
             ret = ["seq"]
 
-            ret.append(STORE(dst, count))
-
             if should_loop:
                 i = IRnode.from_list(_freshname("copy_darray_ix"), typ=UINT256_T)
 
                 loop_body = make_setter(
                     get_element_ptr(dst, i, array_bounds_check=False),
@@ -211,10 +220,13 @@ def _dynarray_make_setter(dst, src):
 
                 src_ = dynarray_data_ptr(src)
                 dst_ = dynarray_data_ptr(dst)
                 ret.append(copy_bytes(dst_, src_, n_bytes, max_bytes))
 
+            # write the length word after data is copied
+            ret.append(STORE(dst, count))
+
             return b1.resolve(b2.resolve(ret))
 
 
 # Copy bytes
 # Accepts 4 arguments:
@@ -334,16 +346,18 @@ def append_dyn_array(darray_node, elem_node):
     with darray_node.cache_when_complex("darray") as (b1, darray_node):
         len_ = get_dyn_array_count(darray_node)
         with len_.cache_when_complex("old_darray_len") as (b2, len_):
             assertion = ["assert", ["lt", len_, darray_node.typ.count]]
             ret.append(IRnode.from_list(assertion, error_msg=f"{darray_node.typ} bounds check"))
-            ret.append(STORE(darray_node, ["add", len_, 1]))
             # NOTE: typechecks elem_node
             # NOTE skip array bounds check bc we already asserted len two lines up
             ret.append(
                 make_setter(get_element_ptr(darray_node, len_, array_bounds_check=False), elem_node)
             )
+
+            # store new length
+            ret.append(STORE(darray_node, ["add", len_, 1]))
             return IRnode.from_list(b1.resolve(b2.resolve(ret)))
 
 
 def pop_dyn_array(darray_node, return_popped_item):
     assert isinstance(darray_node.typ, DArrayT)
@@ -352,20 +366,22 @@ def pop_dyn_array(darray_node, return_popped_item):
     with darray_node.cache_when_complex("darray") as (b1, darray_node):
         old_len = clamp("gt", get_dyn_array_count(darray_node), 0)
         new_len = IRnode.from_list(["sub", old_len, 1], typ=UINT256_T)
 
         with new_len.cache_when_complex("new_len") as (b2, new_len):
+            # store new length
             ret.append(STORE(darray_node, new_len))
 
             # NOTE skip array bounds check bc we already asserted len two lines up
             if return_popped_item:
                 popped_item = get_element_ptr(darray_node, new_len, array_bounds_check=False)
                 ret.append(popped_item)
                 typ = popped_item.typ
                 location = popped_item.location
             else:
                 typ, location = None, None
+
             return IRnode.from_list(b1.resolve(b2.resolve(ret)), typ=typ, location=location)
 
 
 def getpos(node):
     return (

Vulnerable code:             dst = bytes_data_ptr(dst)
            src = bytes_data_ptr(src)

            ret.append(copy_bytes(dst, src, len_, max_bytes))
        # write the length word
        store_length = STORE(dst, len(src.args))
        ann = None
        if src.annotation is not None:
            ann = f"len({src.annotation})"
        store_length = IRnode.from_list(store_length, annotation=ann)
        ret.append(store_length)

            ret.append(STORE(dst, count))

            ret.append(STORE(darray_node, ["add", len_, 1]))
Fixed code: 

dynarray_length_no_clobber_cases = [
    # GHSA-3p37-3636-q8wv cases
    """
a: DynArray[uint256,3]

@external
def should_revert() -> DynArray[uint256,3]:
    self.a = [1,2,3]
    self.a = empty(DynArray[uint256,3])
    self.a = [self.a[0], self.a[1], self.a[2]]

    return self.a  # if bug: returns [1,2,3]
    """,
    """
@external
def should_revert() -> DynArray[uint256,3]:
    self.a()
    return self.b() # if bug: returns [1,2,3]

@internal
def a():
    a: uint256 = 0
    b: uint256 = 1
    c: uint256 = 2
    d: uint256 = 3

@internal
def b() -> DynArray[uint256,3]:
    a: DynArray[uint256,3] = empty(DynArray[uint256,3])
    a = [a[0],a[1],a[2]]
    return a
    """,
    """
a: DynArray[uint256,4]

@external
def should_revert() -> DynArray[uint256,4]:
    self.a = [1,2,3]
    self.a = empty(DynArray[uint256,4])
    self.a = [4, self.a[0]]

    return self.a  # if bug: return [4, 4]
    """,
    """
@external
def should_revert() -> DynArray[uint256,4]:
    a: DynArray[uint256, 4] = [1,2,3]
    a = []

    a = [a.pop()]  # if bug: return [1]

    return a
    """,
    """
@external
def should_revert():
    c: DynArray[uint256, 1] = []
    c.append(c[0])
    """,
    """
@external
def should_revert():
    c: DynArray[uint256, 1] = [1]
    c[0] = c.pop()
    """,
    """
@external
def should_revert():
    c: DynArray[DynArray[uint256, 1], 2] = [[]]
    c[0] = c.pop()
    """,
    """
a: DynArray[String[65],2]

@external
def should_revert() -> DynArray[String[65], 2]:
    self.a = ["hello", "world"]
    self.a = []
    self.a = [self.a[0], self.a[1]]

    return self.a  # if bug: return ["hello", "world"]
    """,
]


@pytest.mark.parametrize("code", dynarray_length_no_clobber_cases)
def test_dynarray_length_no_clobber(get_contract, assert_tx_failed, code):
    # check that length is not clobbered before dynarray data copy happens
    c = get_contract(code)
    assert_tx_failed(lambda: c.should_revert())

            dst_ = bytes_data_ptr(dst)
            src_ = bytes_data_ptr(src)

            ret.append(copy_bytes(dst_, src_, len_, max_bytes))

    # copy contents of src dynarray to dst.
    # note that in case src and dst refer to the same dynarray,
    # in order for get_element_ptr oob checks on the src dynarray
    # to work, we need to wait until after the data is copied
    # before we clobber the length word.

        # copy each item

        # write the length word after data is copied
        store_length = STORE(dst, n_items)
        ann = None
        if src.annotation is not None:
            ann = f"len({src.annotation})"
        store_length = IRnode.from_list(store_length, annotation=ann)

        ret.append(store_length)

            # write the length word after data is copied
            ret.append(STORE(dst, count))


            # store new length
            ret.append(STORE(darray_node, ["add", len_, 1]))
            # store new length