search

penumbra-zone / penumbra

Penumbra is a fully private proof-of-stake network and decentralized exchange for the Cosmos ecosystem.
https://penumbra.zone
Apache License 2.0
361 stars 289 forks source link

Audit use of panicky-macros #4446

Open zbuc opened 1 month ago

zbuc commented 1 month ago

In #4444 @cratelyn noted that the use of the unimplemented!() macro in RPCs can allow a malicious client to crash the node software.

I performed a brief search through the codebase and saw several other calls to unimplemented elsewhere. We should audit the codebase for panicky macros (panic, unimplemented, todo, etc.) and ensure they're unable to crash node software.

erwanor commented 1 month ago

It should only interrupt the connection right? if it makes the full node panic we need to do this immediately

cratelyn commented 1 month ago

i'm not sure if there is comet behavior involved when you say "full node panic" but yes, unimplemented!, todo!, and co. will cause the current thread to panic. if panics are exposed in any publicly accessible endpoints, they almost always end up being DDoS-shaped bugs.

zbuc commented 1 month ago

I just tested this by adding a panic to the batch_swap_output_data RPC. Good news is that the entire node does not crash; I think Tonic handles recovery from the task's panic. Even running a client calling the crashy RPC in a loop didn't prevent other services on the Dex RPC from being called simultaneously.

This work may not need to be completed after all, or is at least lower-priority than we initially thought.