GetBtcEvent should get FromAddress from tx.outputs or handle inputs more than p2wpkh

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/zetaclient/bitcoin_client.go#L582
https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/zetaclient/bitcoin_client.go#L667

Vulnerability details

Impact

On Bitcoin chain, If users send assets to the TSS address with the first tx.input UTXO types other than p2wpkh (i.e. don't have witness or the second field is not public key), zetachain client will ignore their sends or fail to refund the assets, leading to losses. Please note that this is not a user error and likely to happen, because:

1. Both in the documentation Deposit and call zEVM contracts from Bitcoin and the code comments, we only tell users ensure the send's outputs to be: p2wpkh to TSS + OP_RETURN PUSH_x [DATA], not the inputs (UTXO) they use.
2. It's impractical to force users to use p2wpkh UTXO for first tx.input and can harm zetachain's tokenomics (more info below).

Proof of Concept

We filter and parse send events from btc in zetaclient/bitcoin_client.go:

func FilterAndParseIncomingTx(txs []btcjson.TxRawResult, blockNumber uint64, targetAddress string, logger *zerolog.Logger) []*BTCInTxEvnet {
	inTxs := make([]*BTCInTxEvnet, 0)
	for idx, tx := range txs {
		if idx == 0 {
			continue // the first tx is coinbase; we do not process coinbase tx
		}
		inTx, err := GetBtcEvent(tx, targetAddress, blockNumber, logger)
		if err != nil {
			logger.Error().Err(err).Msg("error getting btc event")
			continue  // <===
		}
		if inTx != nil {
			inTxs = append(inTxs, inTx)
		}
	}
	return inTxs
}

As we can see, if we fail to get the btc event in GetBtcEvent, we will ignore this transaction and continue to handle next one. Even more, after we get all the usable events, the last scanned block will be set, so the failed get events won't be handled in the future:

func (ob *BitcoinChainClient) observeInTx() error {
	// ......
		inTxs := FilterAndParseIncomingTx(res.Block.Tx, uint64(res.Block.Height), tssAddress, &ob.logger.WatchInTx)

		for _, inTx := range inTxs {
			//... handle every tx event
		}

		// Save LastBlockHeight
		ob.SetLastBlockHeightScanned(bn)  // <===
}

The problem is in GetBtcEvent(): As show below, after we get the target address and calling parameters (memo), we need to get the FromAddress which will be used as the sender, txOrigin and receiver in GetInBoundVoteMessage to send to the zetacore:

func GetBtcEvent(tx btcjson.TxRawResult, targetAddress string, blockNumber uint64, logger *zerolog.Logger) (*BTCInTxEvnet, error) {
	if len(tx.Vout) >= 2 {
		// ... check the outpus formats and get target address + parameters
		if len(script) == 44 && script[:4] == "0014" { // segwit output: 0x00 + 20 bytes of pubkey hash
			// ...
			if len(script) >= 4 && script[:2] == "6a" { // OP_RETURN
				// ...
				memo = memoBytes
				found = true
			}
	}
	if found {
		fmt.Println("found tx: ", tx.Txid)
		var fromAddress string
		if len(tx.Vin) > 0 {
			vin := tx.Vin[0]
			//log.Info().Msgf("vin: %v", vin.Witness)
			if len(vin.Witness) == 2 {               // <===
				pk := vin.Witness[1]
				pkBytes, err := hex.DecodeString(pk)
				if err != nil {
					return nil, errors.Wrapf(err, "error decoding pubkey")
				}
				hash := btcutil.Hash160(pkBytes)
				addr, err := btcutil.NewAddressWitnessPubKeyHash(hash, config.BitconNetParams)
				if err != nil {
					return nil, errors.Wrapf(err, "error decoding pubkey hash") // <===
				}
				fromAddress = addr.EncodeAddress()   // <===
			}
		}
		return &BTCInTxEvnet{
			FromAddress: fromAddress,
			ToAddress:   targetAddress,
			Value:       value,
			MemoBytes:   memo,
			BlockNumber: blockNumber,
			TxHash:      tx.Txid,
		}, nil
	}
	return nil, nil
}

To get the FromAddress, we hash the second witness field (public key if the corresponding output in UTXO is p2wpkh) in the first input to an address, which is error-prone:

1. There is no requirements in Doc/Code or Bitcoin's spec to enforce user to put the p2wpkh input as the first input in tx.Vin. In reality, it depends on the user's bitcoin wallet to send the transaction. If it doesn't meet if len(vin.Witness) == 2, fromAddress will be unassigned "", this will make zetachain unable to refund the assets if the call on zEVM revert.
2. There are UTXO types contain two witness fields and the second field is not a public key. Like "p2wsh", "p2tr" below:

In these cases, we will failed to decode/convert the second field to an address and return an error, making the transaction be ignored.

3. There are many outputs in UTXO that are not p2wpkh currently, so these users are blocked out of bitcoin to zetachain movement, or they have to move their UTXO to p2wpkh outputs manually and pay fees. Below is a chart about current UTXO types on bitcoin, as we can see, there are many other UTXO can't be spent as p2wpkh:

Tools Used

Manual Review.

Recommended Mitigation Steps

1. Since there could be tx.inputs where we can't get an address (e.g. on witness script and hash), we can require the user set a refund bitcoin address in one of the tx.outputs.
2. (or) Add supports for some largely used UTXO types, and add warnings in the documentation about the UTXO types we can't handle.
3. (and) Add a check to ensure fromAddress is not empty.

Assessed type

Context

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

QA might be more appropriate. Docs explicitly say to use p2wpkh.

[c4-judge]

0xean marked the issue as unsatisfactory:
Overinflated severity

[QiuhaoLi]

@DadeKuma @0xean Hi, thanks for the review and comments. Please note the doc and code explicitly say to use p2wpkh, but that is for the Bitcoin tx's second output, not for the inputs (i.e. the UTXO belongs to users) used. So users are free to use any UTXO, which is likely to happen as shown in the description, and can lead to users' assets lost. Since it's triggered by the users themself, Medium might be right.

[0xean]

I think this is mainly an issue with documentation and certainly think H is overinflated. I will ask for sponsor comment before making a final decision

[brewmaster012]

thanks for the report
this is a valid issue; we will update the doc so that it's clear only p2wpkh inputs/outputs are supported.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b