Array signed int access

Severity

Medium Risk

Summary

Arrays can be keyed by a signed integer, while they are defined for unsigned integers only.

Vulnerability Details

Let's take a toy example:

arr: public(uint256[MAX_UINT256])

@external
def set(idx: int256, num: uint256):
    self.arr[idx] = num

This compile, and works!

If we generate the ir using vyper src/array.vy -f ir, we get this:

[iszero, [xor, _calldata_method_id, 2720814400 <0xa22c5540: set(int256,uint256)>]],
[seq,
  [assert, [iszero, [or, callvalue, [lt, calldatasize, 68]]]],
  [seq,
    [goto, external_set__int256_uint256__common],
    # Line 4
    [seq,
      [label,
        external_set__int256_uint256__common,
        var_list,
        [seq,
          [seq,
            [unique_symbol, sstore_2],
            /* store the value at index */
            [sstore,
              [with,
                clamp_arg,
                /* load the array index */
                [calldataload, 4 <idx (4+0)>],
                /* make sure that the int is not 2**255, max is 2**255 - 1 */
                [seq,
                  [assert,
                    [ne,
                      clamp_arg,
                      115792089237316195423570985008687907853269984665640564039457584007913129639935]],
                  clamp_arg]],
              /* load the array value */
              [calldataload, 36 <num (4+32)>]]],
          [exit_to, external_set__int256_uint256__cleanup],
          pass]],
      [label, external_set__int256_uint256__cleanup, var_list, stop]]]]

There is a warning when compiling that says UserWarning: Use of large arrays can be unsafe!, but please note that this will bail out for any array length that is less than 64 bits long. The reason this warning is up is because an arbitrary long array could give the opportunity to write already used storage slots.

We could write a code that doesn't trigger this warning, such as

arr: public(uint256[max_value(uint32)])

@external
def set(idx: int16, num: uint256):
    self.arr[idx] = num

One could assume in a more tailored smart contract that any array access that is out of bound or at least less than 0 will revert but signed integer can also have an unsigned bitwise equivalent which could cause some collisions in the storage. For instance, 0 in the signed integer representation can be expressed with either 0x000..000, or 0x800..000 (-0). These two indexes will be different, so allowing to key an array from a signed integer doesn't seems to be something that we wouldn't restrict.

Impact

This could cause sne(a)ky accesses to forbidden storage slots.

Tools Used

Manual review

Recommendations

Add a type check for the Subscriptable node and make sure that types matches.