Structural hash implementaion allows for collisions

[pnowosie]

To simplify implementation of EIP-712 structural hashing when transaction does not contain metadata 32 zero-bytes is used for EIP-712 hash computation.
This seems to be good idea since it is aligned with how solidity handles unassigned variables and simplifies further processing

As we discovered this approach could open an attack vector, because transaction without metadata and the same transaction with zero-metadata have different txhash-es, which are used as transaction identifier. But both have same struct hash and therefore a signature.

It will require changes in elixir & cntract's code.

[pnowosie]

Interesting observation was made elsewhere.

@pgebal: The questions is what we do with deposits and EIP format. If we want to keep that Input0, Input 1, ... in EIP format then it looks like we should require deposit transaction to provide 4 inputs (all zeros) and 4 outputs (one non-zero, rest zeros)

@pnowosie: I can see an option to dynamically specified type. E.g. for deposit it would be (type spec)
Transaction(Output output0)
However this cost gas - because contract need to compute hash(type_spec) on demand or precompute all possible types (:question:)

[pnowosie]

Note: This collision can be used to prove non-canonicality of the transaction providing analogical transaction with empty metadata.

[pdobacz]

@pnowosie

can be used to prove non-canonicality

great find. I think this deserves a separate bug tracked in plasma-contracts :(.

It is also more severe than just non-canonicality - it also can be used to challenge input piggybacking in the IFE, so no funds can be exited from neither transaction, if it can't be proven to be included in some block.

[boolafish]

So I was looking at the contract code, I have a proposal here, how about let not use keccak256(txBytes) as txHash anymore, and use the EIP712 one. The obvious downside is that this cost more gas to hash and it needs more changes in code base (I guess both elixir and contract). But I think it simplifies our system and make us consistently use only one hash and it makes more sense that the user signs the hash same as the txHash instead of another eip712Hash.

How do you think? @pnowosie @pdobacz

[pnowosie]

Yes that was initial plan, which was reduced once we realized the complexity & risk (there was no known test vectors).
I'm fully behind this - one suggestion from my side (probably proposed in plasma-contract twin issue) - lets fix first collision issue and start enhancements in separate issue. It will be easier to track and decide on details

[boolafish]

yeah, in general I like that approach. Only downside is audit and then we will probably never fix that due to not want to re-audit lol. But I guess we will just fix the existing collision on empty meta first then since if that's complex to fix the root cause totally.

[pnowosie]

NOTE: we fixed this issue by making metadata field mandatory (default 32-zero bytes) in f535faa.

Integrating with new plasma_framework we implement no metadata means 32-zero bytes in f535faa. This also reflects how plasma_framework decodes a transaction (metadata if missing is zero-bytes32). Mentioned commit always puts empty 32-bytes metadata to rlp-serialized form and rejects rlp-encoded transaction without metadata (metadata is no longer optional).

We need corresponding code on contract side - which strictly require metadata in rlp-encoded form.

Also as suggested in ☝️ comments we need more flexible implementation of EIP-712 struct hash e.g. to support dynamicly sized arrays (no inputs, inputs & outputs without padding & more...)
This will be addressed by another :issue:

This can be closed when 948-ald-deposit-tx-integration will land in master.

[pdobacz]

I think the collisions are not possible anymore. metadata is bytes32 always now, elixir-omg conforms to it AFAIK. Close this @pnowosie or did I miss sth?