Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vm: add two proof verifier to fix the vulnerability in range proof #1121

Merged
merged 14 commits into from
Oct 11, 2022

Conversation

unclezoro
Copy link
Collaborator

Description

This PR aims to solve the BSC Bridge exploitation issue.

Rationale

BC to BSC cross-chain communication is based on the light client technique. A smart contract is deployed on BSC to track the consensus state of BC including the appHash (analogous to stateRoot of Ethereum), which is essentially a root of a verifiable key-value store implemented as an AVL tree. Messages coming from BC will be verified against the appHash so their integrity can be guaranteed.

The store allows consecutive key-value pairs to be proved in batch (better in performance than proving each one individually). However its range proof verification logic contains a critical bug that can be exploited to prove membership of arbitrary key-value pairs chosen by the attacker.

https://github.com/cosmos/iavl/blob/6c1300ae54a9bb851e77dbcc4ba4b21832279027/proof_path.go#L70

https://github.com/cosmos/iavl/blob/6c1300ae54a9bb851e77dbcc4ba4b21832279027/proof.go#L79

The root is calculated by hashing repeatedly (just like in Merkle proof) as in the pseudocode below:
image

If both left and right exist, the digest will ignore the right field. Because of this, if the verification path contains all items with both left and right, the root is unchanged with any arbitrary right values (using the same leaf). This is one factor to be exploited.

This PR in tendermint will introduce ProofOpVerifier to disable following ProofOperation:

  1. The absence proof operator;
  2. The value proof operator with more than one leaves.

Example

Refer to the UT.

Changes

NA

@unclezoro unclezoro marked this pull request as draft October 8, 2022 09:00
@unclezoro unclezoro changed the title enhancement: add two proof operator verifier to fix the vulnerability… enhance: add two proof verifier to fix the vulnerability in range prooof Oct 8, 2022
@unclezoro unclezoro changed the title enhance: add two proof verifier to fix the vulnerability in range prooof enhance: add two proof verifier to fix the vulnerability in range proof Oct 8, 2022
@zb3
Copy link

zb3 commented Oct 8, 2022

Out of curiosity (as an outsider), wouldn't the right package to change be the iavl library? The fix could then be merged upstream. Or I'm missing something?

@unclezoro
Copy link
Collaborator Author

unclezoro commented Oct 8, 2022

Out of curiosity (as an outsider), wouldn't the right package to change be the iavl library? The fix could then be merged upstream. Or I'm missing something?

We can not change iavl library to the right logic directly, because BSC should support replay block from genesis to latest, it means even the exploitation transaction should be compatibly executed. That is why we did not change the iavl lib, but added two verifiers here.

@unclezoro unclezoro added r4r ready for review enhancement New feature or request labels Oct 8, 2022
@unclezoro unclezoro changed the title enhance: add two proof verifier to fix the vulnerability in range proof vm: add two proof verifier to fix the vulnerability in range proof Oct 8, 2022
@unclezoro unclezoro marked this pull request as ready for review October 8, 2022 13:21
params/config.go Outdated Show resolved Hide resolved
@unclezoro unclezoro merged commit cb131fa into bnb-chain:master Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request r4r ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants