Ill-formed calldata to deposit contract can add invalid deposit data

[daejunpark]

Problem

The calldata decoding process in the deposit contract bytecode does not have a sufficient well-formedness check, which fails to detect certain ill-formed calldata, leading to adding invalid deposit data in the deposit Merkle tree.

This is problematic when clients make mistakes and send deposit transactions with incorrectly encoded calldata, which may result in losing their deposited Ether.

For example, the following ill-formed calldata does not trigger the deposit function to revert:

0xc47e300d    <-- deposit function identifier
  0000000000000000000000000000000000000000000000000000000000000060    <-- 96
  0000000000000000000000000000000000000000000000000000000000000080    <-- 128
  00000000000000000000000000000000000000000000000000000000000000a0    <-- 160
  0000000000000000000000000000000000000000000000000000000000000030    <-- 48
  0000000000000000000000000000000000000000000000000000000000000020    <-- 32
  0000000000000000000000000000000000000000000000000000000000000060    <-- 96

and the deposit function will simply result in adding the following (garbage) deposit data in the Merkle tree:

pubkey:
0x0000000000000000000000000000000000000000000000000000000000000020    <-- 32
  00000000000000000000000000000000

withdrawal_credentials:
0x0000000000000000000000000000000000000000000000000000000000000060    <-- 96

signature:
0x0000000000000000000000000000000000000000000000000000000000000000    <-- 0
  0000000000000000000000000000000000000000000000000000000000000000    <-- 0
  0000000000000000000000000000000000000000000000000000000000000000    <-- 0

Since bls_verify will fail on this, the deposited Ether associated to this will be lost.

A fix suggestion

[After discussion with @CarlBeek]

Add a new parameter to the deposit function, say expected_leaf_value, as a checksum for the deposit data to be added in the tree.

That is, after the following code that computes the leaf value:

    node: bytes32 = sha256(concat(
        sha256(concat(pubkey_root, withdrawal_credentials)),
        sha256(concat(amount, slice(zero_bytes32, start=0, len=32 - AMOUNT_LENGTH), signature_root)),
    ))

add the following assertion:

    assert node == expected_leaf_value

Here, clients need to compute expected_leaf_value off-chain and provide it along with the original deposit data, to ensure that the leaf value is correctly computed on-chain, which implies that any ill-formed calldata will be rejected. Furthermore, clients can even run bls_verify on expected_leaf_value off-chain, to ensure that the deposit data to be added in the tree will be eventually claimable in Eth2.

[daejunpark]

For a future reference, other ideas of fixing this issue are shared below.

1. Incorporate the bls_verify logic in the deposit contract to ensure the validity of the deposit data.

This is an ultimate way to ensure the validity, but currently not feasible due to the huge gas cost, until additional EC-precompiles are added to Eth1.

2. Add explicit well-formedness check.

Recall that the calldata of the deposit function must be the following 388-byte array:

0xc47e300d    <-- function identifier
  0000000000000000000000000000000000000000000000000000000000000060    <-- 96 : pointer to pubkey
  00000000000000000000000000000000000000000000000000000000000000c0    <-- 192 : pointer to withdrawal_credentials
  0000000000000000000000000000000000000000000000000000000000000100    <-- 256 : pointer to signature
  0000000000000000000000000000000000000000000000000000000000000030    <-- 48 : size of pubkey
  pppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppp    <-- pubkey[0:32]
  pppppppppppppppppppppppppppppppp00000000000000000000000000000000    <-- pubkey[32:48] with zero-padding
  0000000000000000000000000000000000000000000000000000000000000020    <-- 32 : size of withdrawal_credentials
  wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww    <-- withdrawal_credentials
  0000000000000000000000000000000000000000000000000000000000000060    <-- 96 : size of signature
  ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss    <-- signature[0:32]
  ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss    <-- signature[32:64]
  ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss    <-- signature[64:96]

where ppp... is the actual content of pubkey, www... is of withdrawal_credentials, and sss... is of signature.

Thus, the explicit well-formedness check can be done as follows:
a. check call data size:

assert calldatasize == 388

b. check header:

assert calldata[4:36] == 96
assert calldata[36:68] == 192
assert calldata[68:100] == 256

c. check pubkey:

assert calldata[100:132] == 48
assert calldata[180:196] == 0

d. check withdrawal_credentials:

assert calldata[196:228] == 32

e. check signature:

assert calldata[260:292] == 96

However, it is not straightforward to implement these checks in Vyper, since Vyper does not support the inline assembly because of its design principle.

[CarlBeek]

Thanks @daejunpark for the very in-depth explanation here and discussion we had over the past few days.

2. Add explicit well-formedness check.
   ...
   However, it is not straightforward to implement these checks in Vyper, since Vyper does not support the inline assembly because of its design principle.

An alternative suggestion is to ask the Vyper team to build out "VIP: msg.data" which would allow for the explicit check of the parameters. I question whether there is sufficient time to implement this though.

[djrtwo]

So this boils down to "how can we better prevent potential validators from shooting themselves in the foot". We've been a bit dismissive of additional checks in the past but primarily just wrt bls verification on chain, but this checksum seems to be a reasonable addition with minimal overhead.

The most important thing is that any deposit that comes through can be validly de-serialized and parsed into an SSZ Deposit. This we handled with the length checks on the incoming params. If there is a route in which the data logged is malformed wrt valid SSZ, then it could cause the eth2 chain to break or at least not be able to process new deposits.

Secondly, we want to have a good user experience -- the top item on that list being not losing all your money. The checksum is a solid path and should prevent most buggy data from being committed. I just checked on the Vyper msg.data issue to see if they can prioritize it. They tend to move quickly on needed features like this when they pop up.

[vbuterin]

I like the checksum. Definitely feels the most non-intrusive of all the fixes.

[JustinDrake]

Closing in favour of #1362—we're going ahead with the suggestion deposit_data_root check :)