Unsafe MASP Transaction Validation

[murisi]

Some of the code executed during MASP transaction validation is unsafe and could be exploited to crash a node. More specifically, the following lines may be problematic:

• Spend description zero-knowledge proof reading at:

  namada/shared/src/ledger/masp.rs

  Line 101 in 8e830ef

  bellman::groth16::Proof::read(spend.zkproof.as_slice()).unwrap();

  . A client may inadvertently or maliciously place an invalid spend description ZK-proof into a transaction causing a node to crash. More specifically, placing invalid group elements into the ZK-proof could trigger crashing.
• Analogous issue to above, but for output descriptions at:

  namada/shared/src/ledger/masp.rs

  Line 121 in 8e830ef

  bellman::groth16::Proof::read(output.zkproof.as_slice()).unwrap();

• Analogous issue to above, but for convert descriptions at:

  namada/shared/src/ledger/masp.rs

  Line 138 in 8e830ef

  bellman::groth16::Proof::read(convert.zkproof.as_slice()).unwrap();

• Spend authorization signature extraction at:

  namada/shared/src/ledger/masp.rs

  Line 108 in 8e830ef

  spend.spend_auth_sig.unwrap(),

  . A missing spend authorization signature in a transaction spend description may cause a node to crash.

Additionally, while the type conversion of sighashes (at

namada/shared/src/ledger/masp.rs

Line 156 in 8e830ef

.unwrap();

) is probably valid, it will crash a node if the MASP crate returns a vector with unexpected length. Maybe it would be better to just log an error and reject a transaction triggering this (hopefully impossible) condition?

These are the unsafe operations (acting on untrusted inputs) I've seen so far. cc @gijswijs @juped

[grarco]

@murisi I actually think this has already been solved and can be closed