informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
588 stars 215 forks source link

Serialization for `validator::Set` and related proto structs is inconsistent, lacks validation #1348

Closed mzabaluev closed 9 months ago

mzabaluev commented 9 months ago

1340 attempted to fix an issue with deserializing ValidatorSet data without the total_voting_power field being present in the serialized message.

The field is actually redundant, its value is computable as the sum of voting powers of the validators listed in the message. If the field is not present in the deserialized JSON or protobuf, its value should be recomputed. There is also an issue of trust with impications on protocol security: if we accept the total voting power value without checking it against the sum of listed validators' powers, this means validator::Set cannot be assumed correct by construction.

In the serialization bolted onto the tendermint-proto types generated for ValidatorSet, we skipped the field to match the serialization schema of misbehavior evidence, where it is not present in at least one place, in #1292.

Unfortunately, validator::Set has also had Serialize and Deserialize derived, the total_voting_power field included with no fallback, and this stricter deserialization schema has probably been in use by parties unknown. That serialization is simply derived, with no validations.

Definition of "done"

The following needs to be implemented and backed by tests:

Originally posted by @mzabaluev in https://github.com/informalsystems/tendermint-rs/issues/1340#issuecomment-1708508934

mzabaluev commented 9 months ago

Corrected in the DoD: the tendermint-proto structs are raw protocol data, they should not normally be validated.