c4-bot-4 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/msg_server_add_to_outtx_tracker.go#L100

Vulnerability details

Impact

The outbound transaction queue is potentially blocked by such "stuck" transactions, preventing other pending cctxs from being sent out.

Proof of Concept

The AddToOutTxTracker function, handling the MsgAddToOutTxTracker message, adds a transaction hash associated with a pending cctx to the outbound transaction tracker, which is subsequently watched by observers to detect the transaction's inclusion and confirmation in a block. As a result, the pending cctx is marked as CctxStatus_OutboundMined.

This MsgAddToOutTxTracker message is sent by observers as soon as the outbound transaction is broadcast. See zetaclient/evm_signer.go#L585 and zetaclient/btc_signer.go#L360.

However, the AddToOutTxTracker function only keeps track of a maximum of two different transaction hashes, as seen in line 100. Otherwise, the msg.TxHash is discarded.

024: func (k msgServer) AddToOutTxTracker(goCtx context.Context, msg *types.MsgAddToOutTxTracker) (*types.MsgAddToOutTxTrackerResponse, error) {
...   // [...]
081:
082:    var isDup = false
083:    for _, hash := range tracker.HashList {
084:        if strings.EqualFold(hash.TxHash, msg.TxHash) {
085:            isDup = true
086:            if isProven {
087:                hash.Proved = true
088:                k.SetOutTxTracker(ctx, tracker)
089:                k.Logger(ctx).Info("Proof'd outbound transaction")
090:                return &types.MsgAddToOutTxTrackerResponse{}, nil
091:            }
092:            break
093:        }
094:    }
095:    if !isDup {
096:        if isProven {
097:            hash.Proved = true
098:            tracker.HashList = append([]*types.TxHashList{&hash}, tracker.HashList...)
099:            k.Logger(ctx).Info("Proof'd outbound transaction")
100: ❌      } else if len(tracker.HashList) < 2 {
101:            tracker.HashList = append(tracker.HashList, &hash)
102:        }
103:        k.SetOutTxTracker(ctx, tracker)
104:    }
105:    return &types.MsgAddToOutTxTrackerResponse{}, nil
106: }

This is problematic as an outbound cctx, uniquely identified by its nonce, can have multiple different transaction hashes caused by repeatedly increasing the gas price every epoch.

As soon as the gas price has been increased and observers broadcast the updated transaction (i.e., replace it in the mempool), the transaction hash will change. As a result, reporting the transaction hash to ZetaChain via the AddTxHashToOutTxTracker function call will not keep track of the new transaction hash.

Consequently, the EVM client's observeOutTx and the BTC client's observeOutTx function are not able to query the transaction by the available hashes found in the tracker HashList. This results in the outbound cctx being repeatedly processed by the observers, unnecessarily "blocking" other pending cctxs from being sent out (due to limiting the amount of sent outbound cctx per the 3-second ticker).

Only after such a "stuck" transaction is manually added to the outbound transaction tracker via the AddTxHashToOutTxTracker function by providing a block inclusion proof, the correct and up-to-date transaction hash is added to the tracker and the observers can finally confirm and conclude the cctx. However, this is a separate process and requires (manual) intervention, which might take some time until it is noticed.

Tools Used

Manual review

Recommended mitigation steps

Consider keeping track of all transaction hashes associated with a pending cctx instead of limiting it to two hashes.

Assessed type

Other

c4-pre-sort commented 11 months ago

DadeKuma marked the issue as primary issue

c4-pre-sort commented 11 months ago

DadeKuma marked the issue as sufficient quality report

c4-sponsor commented 11 months ago

lumtis (sponsor) acknowledged

c4-judge commented 10 months ago

0xean marked the issue as satisfactory