AddToInTxTracker doens't allow permissionless tx validation for Bitcoin chain, InTxTracker permissionless tx validation for Bitcoin chain will always fail

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Permissionless tx validation for EVM and Bitcoin chain should be supported based on contest doc→ The system is currently implemented for Ethereum/BSC and Bitcoin. However, currently permissionless tx validation for EVM and Bitcoin chain are only supported in AddToOutTxTracker, but not supported in AddToInTxTracker. Permissionless validation for InTx for bitcoin chain will always fail.

Proof of Concept

In AddToInTxTracker() from msg_server_add_to_intx_tracker.go, chain is incorrectly diverted. The bitcoin chain is missing and will return an error.

//x/crosschain/keeper/msg_server_add_to_intx_tracker.go
func (k msgServer) AddToInTxTracker(goCtx context.Context, msg *types.MsgAddToInTxTracker) (*types.MsgAddToInTxTrackerResponse, error) {
...
		if common.IsEVMChain(msg.ChainId) {
			err = k.VerifyEVMInTxBody(ctx, msg, txBytes)
			if err != nil {
				return nil, types.ErrTxBodyVerificationFail.Wrapf(err.Error())
			}
} else {
               //@audit when ChainId is bitCoin chain, this will return an error
|>			return nil, types.ErrTxBodyVerificationFail.Wrapf(fmt.Sprintf("cannot verify inTx body for chain %d", msg.ChainId))
		}

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/msg_server_add_to_intx_tracker.go#L38-L39)

Notice in AddToOutTxTracker(), bitcoin chain is correctly diverted with else if common.IsBitcoinChain(msg.ChainId)statement.

Tools Used

Manual review

Recommended Mitigation Steps

In AddToInTxTracker(), add IsBitcoinChain(msg.ChainId) to correctly handle when chainId is bitcoin chain.

Assessed type

Error

[DadeKuma]

According to the README:

The system is currently only used to validate txs to be added in the tracker through MsgAddToInTxTracker and MsgAddToOutTxTracker and not to validate txs to be observed as part of the ZetaChain cross-chain tx workflow

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory:
Invalid

[flowercrimson]

Hey @0xean, I don't get why this is invalid.
(1) According to doc, both AddToInTxTracker and AddToOutTxTracker are supported for permissionless validation and both should support bitcoin chain and EVM chain. So IMO, the lookout's quote supports the validity of this report.

An inbound/outbound tx can be sent by anyone with a proof. The proof is checked against the header of the chain to validate it
→ The system is currently only used to validate txs to be added in the tracker through MsgAddToInTxTracker and MsgAddToOutTxTracker and not to validate txs to be observed as part of the ZetaChain cross-chain tx workflow
→ The system is currently implemented for Ethereum/BSC and Bitcoin

Note that this issue concerns only MsgAddToInTxTracker and MsgAddToOutTxTracker, and doens't concern validate txs to be observed as part of the ZetaChain cross-chain tx workflow. The latter is part of keeper -> vote_inbound_tx / vote_outbound_tx flow, which is irrelevant to this discussion.

(2)This report argues that AddToInTxTracker() is flawed because it currently will revert when the chain is Bitcoin. For reference, AddToOutTxTracker() currently correctly supports both Bitcoin chain and EVM chain. This means AddToInTxTracker() needs to be fixed to support both chains.

a) AddToInTxTracker() : This reverts when the chain is Bitcoin

//x/crosschain/keeper/msg_server_add_to_intx_tracker.go
func (k msgServer) AddToInTxTracker(goCtx context.Context, msg *types.MsgAddToInTxTracker) (*types.MsgAddToInTxTrackerResponse, error) {
...
		if common.IsEVMChain(msg.ChainId) {
			err = k.VerifyEVMInTxBody(ctx, msg, txBytes)
...
} else {
               //@audit when ChainId is bitCoin chain, this will return an error
|>			return nil, types.ErrTxBodyVerificationFail.Wrapf(fmt.Sprintf("cannot verify inTx body for chain %d", msg.ChainId))
		}

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/msg_server_add_to_intx_tracker.go#L38-L39)

b) AddToOutTxTracker(): This supports both bitcoin and evm chains

//x/crosschain/keeper/msg_server_add_to_outtx_tracker.go
func (k msgServer) AddToOutTxTracker(goCtx context.Context, msg *types.MsgAddToOutTxTracker) (*types.MsgAddToOutTxTrackerResponse, error) {
...
	chain := k.zetaObserverKeeper.GetParams(ctx).GetChainFromChainID(msg.ChainId)
...
	if msg.Proof != nil { // verify proof when it is provided
...
|>		err = k.VerifyOutTxBody(ctx, msg, txBytes)
		if err != nil {
			return nil, types.ErrTxBodyVerificationFail.Wrapf(err.Error())
		}
		isProven = true
...
}

func (k Keeper) VerifyOutTxBody(ctx sdk.Context, msg *types.MsgAddToOutTxTracker, txBytes []byte) error {
...
	// verify message against transaction body
	if common.IsEVMChain(msg.ChainId) {
		err = VerifyEVMOutTxBody(msg, txBytes, tss.Eth)
        //@audit-info note: this supports both chains
|>	} else if common.IsBitcoinChain(msg.ChainId) {
		err = VerifyBTCOutTxBody(msg, txBytes, tss.Btc)
	} else {
		return fmt.Errorf("cannot verify outTx body for chain %d", msg.ChainId)
	}
...
}

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

[0xean]

@flowercrimson - thanks for raising this (again). I agree its not handled correctly for BTC on the input flow AND it does state that it should in the contest docs. There is of course a work around (using the permissioned flow). That being said since the sponsor call out this new flow as being important, I think it probably warrants M. Lets get @lumtis to provide an opinion here on severity before we finalize it all.

[c4-judge]

0xean marked the issue as satisfactory

[lumtis]

Yes, Bitcoin should be added here.

[c4-sponsor]

lumtis (sponsor) confirmed

[0xean]

thanks all, M it is.

[c4-judge]

0xean marked the issue as selected for report