Submitted by ChristiansWhoHack
https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/proto/crosschain/tx.proto#L141
https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/proto/crosschain/tx.proto#L117
The voting process works as follows:
The index of an observed transaction MsgVoteOnObservedInboundTx is calculated by taking a hash of the incoming message. For every observer that votes on a given event, the index should be the same. The protection in place for stopping duplicate transactions is simply checking if this index has occurred already. Since the ballots are never deleted, this works well.
However, the index is too granular. Many aspects of an event are guaranteed to not change: sender/receiver change, tx hash, amount, cointype, etc. This is NOT the case with several of the fields that are hardcoded into the Zetaclient but may change in the future. Gaslimit (hardcoded in app) and asset (which is currently blank) are unused fields that may change in the future. On top of this, a change in encoding, what the zetaclient signs or anything else would result in a different hash as well.
Additionally, any newly added fields would change the index of previous ballots as well. If this sounds farfetched, there is already a case of this happening since deployment. The field eventIndex was added very recently to the repository, since multiple events can happen within a single transaction. If the current Zetachain deployment had the AddToInTxTracker then it would be possible to exploit the new eventIndex field changes the hash to retrigger the CCTX.
If any of these values change in the future, then the ballot index would change. Since this index is the only security protection for duplicate submissions, the same event could be submitted once again. To make matters worse, there is nothing within the TxInTracker on the zetachain or zetaclient that checks that an event has already occurred. This allows for trivial exploitation when the client or parts of the message are updated.
Relying on this index to never change is an unspoken variant now. Since this is not mentioned anywhere, a tiny change made by a developer would result in every CCTX previously created to be valid in the voting process once again. Although there is some waiting involved, a malicious adversary could send CCTXs and simply wait for them to be valid again once a change to the Zetachain or Zetaclient is made.
Attack strategy:
This proof of concept demonstrates the issue from the Cosmos SDK tests. It sends two events that only differ by the GasLimit and checks if they get approved or not. To make the proof of concept more viable, you could simulate a change on the zetaclient parameters and send a proof through the TxInTracker afterwards. Since this requires a change to the zetaclient live, we felt that a Cosmos SDK PoC was clearer to reproduce and easier to understand.
The obvious solution would be to remove fields that can change over time from the message. This way, small changes do not change the voting index and compromise the duplicate event submission check. However, this does NOT work because it would allow the finalizing vote to set many of the fields, which would be bad.
So, it is recommended to have a separate structure for keeping track of events that have already occurred besides the voting process that is specific index for an event. For instance, the tx hash, event id and chain id are a great way to verify that an event occurred already to guarantee that the same event is not trying to be added again.
Defense in depth measures can be put in place to secure this more.
An obvious one is having a timeout. For instance, maximum blocks between events or a week of actual time. This would limit the scope of the exploit to only recent changes.
Another one would be introducing a version scheme for each change the message. On the event voting side, only allowing for specific versions past a specific point would ensure that old ballots could not be resubmitted.
Rigorous testing to ensure that old transactions are the same compared to new ones. If a library is upgraded, a byte is dropped or anything else, then a different index will be created, resulting in lost funds.
lumtis (ZetaChain) confirmed
