#38682 [SC-Medium] AugAssign evaluation order causing OOB write within the object

Submitted on Jan 10th 2025 at 01:15:10 UTC by @anatomist for Attackathon | Ethereum Protocol

• Report ID: #38682

• Report Type: Smart Contract

• Report severity: Medium

• Target: https://github.com/vyperlang/vyper

• Impacts:

  • (Compiler) Incorrect bytecode generation leading to incorrect behavior

  • (Compiler) Unexpected behavior

Description

Brief/Intro

AugAssign (e.g. +=, -=, ...) statements does not consider the side effect caused by rhs, allowing out-of-bounds index write within the array.

Vulnerability Details

Vyper parse AugAssign statements by first cache the target location to avoid double evaluation. However, in the case when target is an access to a DynArray and the right value modify the array, the cached target will evaluate first, following the modifications like DynArray.pop(), and eventually the STORE operation could store the computed output in an index that falls outside the array’s current length bounds.

def parse_AugAssign(self):
	target = self._get_target(self.stmt.target)
	right = Expr.parse_value_expr(self.stmt.value, self.context)

	if not target.typ._is_prim_word:
		# because of this check, we do not need to check for
		# make_setter references lhs<->rhs as in parse_Assign -
		# single word load/stores are atomic.
		raise TypeCheckFailure("unreachable")

	with target.cache_when_complex("_loc") as (b, target):
		left = IRnode.from_list(LOAD(target), typ=target.typ)
		new_val = Expr.handle_binop(self.stmt.op, left, right, self.context)
		return b.resolve(STORE(target, new_val))

This is alike this high severity issue where it discusses similar issues for Assign. However, it is not appropriate to apply the same mitigations in parse_Assign to parse_AugAssign, since the evaluation order of Assign is rhs -> lhs, while evaluation of AugAssign should be dereference lhs -> rhs -> assign to lhs, so assigning rhs to a temporary variable will actually change the evaluation order. We currently don't have a simple patch in mind, aside from reimplementing the entire AugAssign. Notably, care must be taken to not double evaluate lhs when fixing the bug.

Impact Details

The direct impact is out-of-bounds index access within the array. But wrong IR code emission could also lead to various consequences.

References

https://github.com/vyperlang/vyper/blob/a29b49d422f6979be2b9c6c80aa583a60b1ccb7f/vyper/codegen/stmt.py#L291

Proof of Concept

Proof of Concept

@deploy
def __init__():
    a: DynArray[uint256, 2] = [1, 2]
    a[1] += a.pop() # should revert here, but doesn't. rhs doesn't have to be pop, and can be other complex functions