Insufficient zero-padding bug for functions returning byte arrays of size < 16

[daejunpark]

Version Information

• vyper Version (output of vyper --version): 0.1.0b11+commit.c775cda (the latest master)
  • The buggy behavior happens for both recent releases, v0.1.0-beta.10 and v0.1.0-beta.11, as well.

What's your issue about?

Summary:

A function whose return type is bytes[n] where n < 16, returns a value that does not conform to the ABI encoding, having incorrect zero-padding.

Details:

For example, the get_deposit_count function of the deposit contract, whose return type is bytes[8], returns the following 96 bytes (in hexadecimal notation):

0x0000000000000000000000000000000000000000000000000000000000000020
  0000000000000000000000000000000000000000000000000000000000000008
  deadbeefdeadbeef000000000000000000000000000000000000000000000020
                                                                ^^  <-- problem!

where the first 32 bytes (the first line) denotes the header (the offset 32 = 0x20), the second 32 bytes (the second line) denotes the length of the byte array (8), and the "deadbeefdeadbeef" denotes the content of the byte array (bytes[8]). Here, the problem is that the last byte is 32 (= 0x20) while it should be 0 (= 0x00) according to the ABI specification. This means that any contract (written in either Solidity or Vyper) that calls to this type of functions expecting the correct zero-padding may misbehave.

What happens is that the compiled bytecode of get_deposit_count fails to sufficiently pad zeros when it prepares for the return value. In the following return statement,

return self.to_little_endian_64(self.deposit_count)

it first copies the returned value (of 8 bytes) to some specific region of the memory, and puts only 8 bytes of zero-padding after that, instead of 24 bytes of zeros, which results in including some garbage values in the last 16 bytes. Indeed, in this particular case, the last 16 bytes (000000000000000000000000020) came from the side-effect of the sub-function call to self.to_little_endian_64().

To be more specific, in the following zero-padding LLL code of get_deposit_count (generated by the zero_pad function of vyper/parser/stmt.py):

            /* Zero pad */
            [with,
              _ceil32_end,
              [ceil32, [mload, 640]],
              [repeat,
                736,
                [mload, 640],
    ??? -->     8,
                [seq,
                  [if, [gt, [mload, 736], _ceil32_end], break],
                  [mstore8, [add, 672, [mload, 736]], 0]]]],
            [mstore, 608, 32],
            [return, 608, [ceil32, [add, [mload, 640], 64]]],
            # Line 58
            stop]],

the third argument of the repeat loop is 8, where it should be at least 24. In a quick examination of the Vyper compiler code, it seems that the number 8 comes from the maxlen of the type bytes[8], but I couldn't have a chance to further investigate the root cause and a quick fix.

Indeed, the same problem happens in the to_little_endian_64 function as well, which has less effect because it is a private function, though.

[This bug was privately reported on July 22, 2019, confirmed as not a security vulnerability, and made public here for the transparency.]

[daejunpark]

To quickly reproduce the buggy behavior:

$ git clone https://github.com/ethereum/eth2.0-specs.git
$ cd eth2.0-specs
[change tests as described in https://github.com/ethereum/eth2.0-specs/issues/1341]
$ make install_deposit_contract_test
$ ( . deposit_contract/venv/bin/activate
    && pip3 uninstall vyper
    && cd /path/to/vyper
    && make )
$ make compile_deposit_contract
$ make test_deposit_contract

Also see: ethereum/consensus-specs#1341

[davesque]

Thanks for the heads up. I'll try and figure this out.