Slice bounds check can be overflowed to access unrelated data

Severity

High Risk

Relevant GitHub Links

https://github.com/vyperlang/vyper/blob/b01cd686aa567b32498fefd76bd96b0597c6f099/vyper/builtins/functions.py#L404-L457

https://github.com/vyperlang/vyper/blob/b01cd686aa567b32498fefd76bd96b0597c6f099/vyper/builtins/functions.py#L319-L331

Summary

The bounds check for slices does not account for the ability for start + length to overflow when the start value is not a literal. This creates the ability for an attacker to overflow the bounds check to use the slice() built-in to access either (a) an unrelated storage slot or (b) the previous word of memory.

Vulnerability Details

When calling slice() there are compile time bounds checks if the start and length values are literals, but of course this cannot happen if they are passed values:

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)

At runtime, we perform the following equivalent check, but the runtime check does not account for overflows:

["assert", ["le", ["add", start, length], src_len]],  # bounds check

This same issue exists if the bytestring being sliced is in memory or storage:

The storage slice() function copies bytes directly from storage into memory and returns the memory value of the resulting slice. This means that, if a user is able to input the start value, they can force an overflow and access an unrelated storage slot. In most cases, this will mean they have the ability to forceably return 0 for the slice, even if this shouldn't be possible. In extreme cases, it will mean they can return another unrelated value from storage.

The memory slice() function returns the memory value of the resulting slice. There is a check as part of the process that start + 32 < length, which means that for the overflow to be possible start must be greater than max uint256 - 31. As a result, the returned slice can be any slice starting up to 32 bytes before the variable that is being sliced.

Proof of Concept

For simplicity, take the following Vyper contract, which takes an argument to determine where in a Bytes[64] bytestring should be sliced. It should only accept a value of zero, and should revert in all other cases.

# @version ^0.3.9

x: public(Bytes[64])
secret: uint256

@external
def __init__():
    self.x = empty(Bytes[64])
    self.secret = 42

@external
def slice_it(start: uint256) -> Bytes[64]:
    return slice(self.x, start, 64)

We can use the following manual storage to demonstrate the vulnerability:

{"x": {"type": "bytes32", "slot": 0}, "secret": {"type": "uint256", "slot": 3618502788666131106986593281521497120414687020801267626233049500247285301248}}

If we run the following test, passing max - 63 as the start value, we will overflow the bounds check, but access the storage slot at 1 + (2**256 - 63) / 32, which is what was set in the above storage layout:

function test__slice_error() public {
    c = SuperContract(deployer.deploy_with_custom_storage("src/loose/", "slice_error", "slice_error_storage"));
    bytes memory result = c.slice_it(115792089237316195423570985008687907853269984665640564039457584007913129639872); // max - 63
    console.logBytes(result);
}

The result is that we return the secret value from storage:

Logs:
0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002a

For a memory slice, see the following contract:

# @version ^0.3.9

@external
def slice_it_mem(start: uint256) -> Bytes[32]:
    x: uint256 = 2345908340958
    y: Bytes[32] = b"\x05\x05"
    return slice(y, start, 32)

If we pass max uint256 - 31 as the start, we will be returned 2 (the length of the bytestring). If we pass max uint256 - 30, we will be returned 205 (the length plus the first element of the left aligned bytestring). If we pass max uint256 - 29, we will be returned 20505, etc.

Impact

The built-in slice() method can be used to read unrelated storage slots or memory locations by abusing a bounds check overflow.

Tools Used

Manual Review, Foundry

Recommendations

Update the bounds check to also include a check that start + length > start to ensure no overflow is possible.