Review of PRs to fix BSC Bridge exploitation issue on BSC and Tendermint

[redragonvn]

[WIP] Preliminary Security review

Review of four PRs aims to fix security issues on BSC and Tendermint

Oct 8th, 2022
Author: Verichains - https://verichains.io

Overview

Following the BSC Bridge hack and our analysis on BSC Bridge hack, BNB Chain dev team has proposed four PRs on Tendermint and BSC as below:

• vm: add two proof verifier to fix the vulnerability in range proof bnb-chain/bsc#1121
• fix: add ProofOp verifier to enhancement verification bnb-chain/tendermint#1
• close register for bsc relayers bnb-chain/bsc-genesis-contract#193
• fix: add height check for cross chain package bnb-chain/bsc-genesis-contract#192

The four PRs aims to solve the following issues:

a. Security issue in IAVL RangeProof of Cosmos codebase

The bug was exploited in the recent BSC Bridge hack. More detailed analysis could be found at Binance Chain Bridge Exploitation Writeup - Part 1 and Binance Chain Bridge Exploitation Writeup - Part 2

b. Critical security issue in multistore proof verification of BSC light-client (new)

The PRs also fix another new finding issue, lying in the multistore proof verification logic. It's about a serialized key-value data struct that is encoded as a list of (key, value) pairs. There are pre-assumption that the keys should be unique but that constraint has not been verified, thus causing the bug.

Basically, a multistore proof contains a list of name-hash pairs of the substores. When computing the multistore hash, this list is converted into a map whose keys are the substores' names: https://github.com/bnb-chain/bsc/blob/master/core/vm/lightclient/rootmultistore.go#L41. Therefore, if the proof contains multiple entries with duplicate names, earlier hashes for the same name will be ignored. However, the input root hash of a substore is checked against the first encountered entry in the proof matching its name: https://github.com/bnb-chain/bsc/blob/master/core/vm/lightclient/multistoreproof.go#L114, thus a proof looks like this: [..., (“ibc”, evilHash), (“ibc”, validHash), ...] will be successfully verified although it should not be.

c. Another critical security issue in Cosmos codebase (new)

Shared privately with BNB/Binance team. To be disclosed in details later.

Our Review

This is still a preliminary review of Verichains team on the four Pull requests. In summary, we think that the PRs could stop the exploit conditions by adding verifier/validator logic for Merkle proofs:

• Added 3 verifiers:

  • forbiddenAbsenceOpVerifier: raise error for IAVLAbsenceOp (https://github.com/bnb-chain/bsc/pull/1121/files#diff-bdd31bdae9170f8fd8c03100808fdb227f27266635f4cb14d42edb91296d60d8R189-R196).
  • singleValueOpVerifier: ensure that the number of leaves is 1 and no node of LeftPath has both non-empty left and right (https://github.com/bnb-chain/bsc/pull/1121/files#diff-bdd31bdae9170f8fd8c03100808fdb227f27266635f4cb14d42edb91296d60d8R199-R213).
    This verifier effectively blocks the previous hacker exploit.
  • multiStoreOpVerifier: to prevent multiple entries having the same name in a multistore proof (https://github.com/bnb-chain/bsc/pull/1121/files#diff-bdd31bdae9170f8fd8c03100808fdb227f27266635f4cb14d42edb91296d60d8R200).

• These verifiers are passed into the Proof structure right after decoding (https://github.com/bnb-chain/bsc/blob/c5cc1d5b0d482894980c76e60b39634d9e7034e2/core/vm/contracts_lightclient.go#L178) and again passes down to actual verifiers (https://github.com/bnb-chain/bsc/blob/c5cc1d5b0d482894980c76e60b39634d9e7034e2/core/vm/lightclient/types.go#L234,L238) and finally run for each proof to check for errors (https://github.com/bnb-chain/tendermint/blob/6fa739f145d5b80cc845b390229bc8c50689029d/crypto/merkle/proof.go#L46-L50).

• A package together with its proof now must pass a height check. Specifically, if package1.sequence < package2.sequence, then package2.proof.BC_height must be greater than or equal package1.proof.BC_height (https://github.com/bnb-chain/bsc-genesis-contract/pull/192/files), i.e. the BC heights at which proofs are generated must be in increasing order. Although this check was intended to block the origin hack attacker’s payload, it does not. However, it helps prevent attack vector leveraging the new finding issue (c) above.

The 3rd PR closes registration for new BSC relayers. This is a temporary fix and will only be kept until BC to BSC cross-chain communication going through more in-depth security review.

Potential issues from PRs

The height check introduced in the 4th PR is a little bit too tight and might cause some troubles.

Consider the situation in the following figure:

Say, let t100 and t101 denote the times at which the blocks of height 100 and 101 are proposed on BSC. Between these, 10 blocks at height 1001, 1002, … 1010 are proposed on BC. At each BC block, there’s a few cross-chain asset transfers from BC to BSC resulting in packages, created with proofs at that BC height, are relayed to BSC. For example, packages with sequence number 1337, 1338, 1339 come with proofs at BC height 1001. Package 1340 comes with proof at BC 1002 and so on.

Now, before t101, let’s say, for some reason (there’s might be a malicious actor or relayers are just not in sync), a new proof for package seq.1337 is regenerated at BC height 1002 (or higher) and submitted in a transaction to BSC with high fee/tip. When this transaction is included in BSC block 101 and sorted at the front, all other transactions containing cross-chain packages included in this block automatically become invalid due to the new BC height check.

If the action is malicious and repeated, the relaying throughput can be reduced down to 1 packages/BSC block, which is some form of a DoS attack.

To mitigate the issue, we recommend allowing a short distance or window size d between the BC height at which a proof is generated and the highest BC block ever seen in a proof of a package tracked by the Crosschain.sol smart contract for a specific channel.

Let b be the maximum number of blocks that can be proposed on BC between 2 consecutive blocks on BSC, choosing d = b is fine. If BSC reorg is taken into account and we don’t want proofs to be regenerated during reorgs, let r denote the largest reorg depth than can possibly occur, d can chosen to be b*r.

For example, BSC currently has 21 validators, the theoretically maximum reorg depth r is 14 (7 malicious + 7 honest, assuming malicious/total < 2/3). If the maximum number of blocks that can be proposed on BC during a BSC block interval is, say, b = 10; then we have d = b*r = 10 * 14 = 140 and the BC height check should now be:

require(height + 140 >= channelSyncedHeaderMap[channelId], "too old header");
if (height > channelSyncedHeaderMap[channelId]) {
 channelSyncedHeaderMap[channelId] = height;
}

Other Comments

With new critical security issues in just a few days, we are really concerned there could be more potential security issues in BSC Bridge and Cosmos codebase. Getting hacked once again due to security issues shall greatly impact the BNB Chain ecosystem and users trust. In our opinion, a full in-depth security review of BSC cross-chain bridge and Cosmos related codebase should be done ASAP to find other potential security issues (if any) before fully reopening the BSC bridge/relayers.

We recommend practicing secure coding, where a sanity check is done right before the input is consumed.