Description

The Bitcoin client is used to watch for cross-chain transactions as well as to relay transactions to and from the Bitcoin chain. There are numerous functions in the client, but the relevant functions are described below:

1. IsSendOutTxProcessed() - Checks the ob.submittedTx[outTxID] to see whether the transaction in question has already been submitted for relaying.

2. startSendScheduler() - Runs every three seconds. This function gets all pending CCTX and checks if they have already been submitted with IsSendOutTxProcessed(). If the CCTX has not been submitted, it will call TryProcessOutTx().

3. TryProcessOutTx() - Signs and broadcasts a CCTX, then adds it to a tracker in the x/crosschain module with AddTxHashToOutTxTracker().

4. observeOutTx() - Runs every two seconds. It queries for all transactions that have been added to the tracker in the x/crosschain module and adds them to ob.submittedTx[outTxID].

Impact

The bug here occurs due to the racy check in IsSendOutTxProcessed(). More specifically, the following scenario would lead to the bug:

1. First, startSendScheduler() runs and gets a pending CCTX. It checks that the CCTX has not been processed (i.e., has not been added to ob.submittedTx[], so IsSendOutTxProcessed() returns false), and thus calls TryProcessOutTx().

2. Then, TryProcessOutTx() signs the CCTX and broadcasts it, then adds it to the tracker in the x/crosschain module.

3. After, startSendScheduler() runs again before observeOutTx() is able to run. The CCTX is in the x/crosschain module tracker but not yet in ob.submittedTx[] since observeOutTx() has not run yet. Therefore, TryProcessOutTx() is called again.

4. Then TryProcessOutTx() runs, signs, broadcasts, and adds the same CCTX to the tracker in the x/crosschain module.

5. Finally, observeOutTx() runs and adds (or in this case, overwrites) the CCTX to ob.submittedTx[].

The bug occurs in step 3. Since observeOutTx() is responsible for adding the CCTX to the ob.submittedTx[] map, the intention is for observeOutTx() to run before startSendScheduler() runs again. Due to the racy nature of the code though, this does not happen, and thus the bug is triggered.

The bug triggers with the current smoke tests by modifying the following line of code in bitcoin_client.go to make observeOutTx() run every 30 seconds.

func (ob *BitcoinChainClient) observeOutTx() {
	ticker := time.NewTicker(30 * time.Second)

    // [ ... ]
}

Recommendations

A naive fix for this bug is to modify IsSendOutTxProcessed() to make it query for pending CCTXs in the x/crosschain module's tracker instead. This will prevent this issue from occurring, as startSendScheduler() and TryProcessOutTx() run synchronously.

Although the above fix is sufficient for this specific issue, we find it important to note that the code here is multithreaded and accesses ob.submittedTx[] asynchronously without any locking involved. Additionally, ob.submittedTx[] is often out of sync with the tracker in the x/crosschain module. Code like this is prone to similar bugs, and it is especially prone to bugs being introduced in the future. Because of this, it is our recommendation that the ZetaChain team do a thorough refactoring of the code to introduce synchronization between the functions. This would eliminate the racy nature of the code and make it less likely for bugs to be introduced in the future.

Remediation

This issue has been acknowledged by ZetaChain.