Zellic - Audit Reports

Review of changes

In this section, we summarize all the changes that were made to the codebase to remediate our findings from the previous audit.

3.1 --- Any `ZetaSent` events are processed regardless of what contract emits them

This finding was fixed by introducing the ConnectorZEVM contract's address into the SystemContract structure. This address is now used in the EVM hooks to ensure that ZetaSent events are only processed if they are emitted by the ConnectorZEVM contract.

All nonrelevant code has been omitted from the excerpt below.

func (k Keeper) ProcessLogs(ctx sdk.Context, logs []*ethtypes.Log, emittingContract ethcommon.Address, txOrigin string) error {

	system, found := k.fungibleKeeper.GetSystemContract(ctx)
	connectorZEVMAddr := ethcommon.HexToAddress(system.ConnectorZevm) // Zellic: new ConnectorZEVM address

	for _, log := range logs {
		eZeta, err := ParseZetaSentEvent(*log, connectorZEVMAddr)
		if err == nil {
			if err := k.ProcessZetaSentEvent(ctx, eZeta, emittingContract, txOrigin); err != nil {
				return err
			}
		}
	}
	return nil
}

func ParseZetaSentEvent(log ethtypes.Log, connectorZEVM ethcommon.Address) (*connectorzevm.ZetaConnectorZEVMZetaSent, error) {
	// [ ... ]

    // Zellic: new check to ensure events are emitted by this contract
	if event.Raw.Address != connectorZEVM {
		return nil, fmt.Errorf("ParseZetaSentEvent: event address %s does not match connectorZEVM %s", event.Raw.Address.Hex(), connectorZEVM.Hex())
	}
	return event, nil
}

3.2 --- Bonded validators can trigger reverts for successful transactions

This finding was fixed by introducing a new Admin Policy account type. With this fix, although outgoing transactions can still be removed from the tracker, this is only allowed if the message creator is the Admin Policy account. Previously, any bonded validators could perform this action.

func (k msgServer) RemoveFromOutTxTracker(goCtx context.Context, msg *types.MsgRemoveFromOutTxTracker) (*types.MsgRemoveFromOutTxTrackerResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

    // Zellic: new check added to ensure only the Admin Policy account can remove transactions from the out TX tracker
	if msg.Creator != k.zetaObserverKeeper.GetParams(ctx).GetAdminPolicyAccount
	(zetaObserverTypes.Policy_Type_out_tx_tracker) {
		return &types.MsgRemoveFromOutTxTrackerResponse{}, zetaObserverTypes.ErrNotAuthorizedPolicy
	}

	k.RemoveOutTxTracker(ctx, msg.ChainId, msg.Nonce)
	return &types.MsgRemoveFromOutTxTrackerResponse{}, nil
}

3.3 --- Sending ZETA to a Bitcoin network results in BTC being sent instead

This finding was fixed by introducing a new check in the ProcessZetaSentEvent() function that checks to ensure that any external chains ZETA is being sent to has a ZETA-token contract deployed on it. Since the Bitcoin chain does not implement smart contracts, this check will prevent ZETA from being sent to the Bitcoin chain (and any other non--EVM-compatible chains).

func (k Keeper) ProcessZetaSentEvent(ctx sdk.Context, event *connectorzevm.ZetaConnectorZEVMZetaSent, emittingContract ethcommon.Address, txOrigin string) error {
	// [ ... ]

	// Validation if we want to send ZETA to external chain, but there is no ZETA token.
	coreParams, found := k.zetaObserverKeeper.GetCoreParamsByChainID(ctx, receiverChain.ChainId)
	if !found {
		return zetacoretypes.ErrNotFoundCoreParams
	}
	if receiverChain.IsExternalChain() && coreParams.ZetaTokenContractAddress == "" {
		return zetacoretypes.ErrUnableToSendCoinType
	}

    // [ ... ]
}

3.4 --- Race condition in Bitcoin client leads to double spend

This finding was fixed by introducing a new map to the Bitcoin client that now tracks all transaction hashes of successfully broadcasted transactions to the Bitcoin chain. This prevents the same transaction from being broadcasted twice, thus preventing the double spend.

// returns isIncluded, isConfirmed, Error
func (ob *BitcoinChainClient) IsSendOutTxProcessed(sendHash string, nonce int, _ common.CoinType, logger zerolog.Logger) (bool, bool, error) {
	outTxID := ob.GetTxID(uint64(nonce))
	logger.Info().Msgf("IsSendOutTxProcessed %s", outTxID)

	ob.mu.Lock()
	txnHash, broadcasted := ob.broadcastedTx[outTxID]
	res, mined := ob.minedTx[outTxID]
	ob.mu.Unlock()

	if !mined {
		if !broadcasted {
			return false, false, nil
		}
		
        // [ ... ]
	}
	
    // [ ... ]
    return true, true, nil
}

3.5 --- Not waiting for minimum number of block confirmations results in double spend

This finding was fixed by waiting for a configured amount of blocks before considering a block confirmed. In the testing environment, the Bitcoin client now waits for at least two blocks before considering a block as confirmed. On average, each block takes 10 minutes to be mined, so this would be 20 minutes for a block to be considered confirmed.

func GetCoreParams() CoreParamsList {
	params := CoreParamsList{
		CoreParams: []*CoreParams{
			// [ ... ]
			{
				ChainId:                     common.BtcRegtestChain().ChainId,
				ConfirmationCount:           2,
				// [ ... ]
			}},
	}
    
	// [ ... ]
}

func (ob *BitcoinChainClient) observeInTx() error {
	// [ ... ]

	// "confirmed" current block number
	confirmedBlockNum := cnt - int64(ob.GetChainConfig().CoreParams.ConfCount)
	if confirmedBlockNum < 0 || confirmedBlockNum > math2.MaxInt64 {
		return fmt.Errorf("skipping observer , confirmedBlockNum is negative or too large ")
	}

	// query incoming gas asset
	if confirmedBlockNum > lastBN {
		// [ ... ]
	}
    // [ ... ]
}

3.6 --- Multiple events in the same transaction causes loss of funds and chain halting

This finding was fixed by introducing an index to the emitted ZetaSent event. This is then used within the x/crosschain module to distinguish between multiple ZetaSent events within the same transaction. The ZetaChain team decided to add this index onto the gasLimit for each cross-chain transaction, which effectively turns the gasLimit into a unique identifier for each emitted event. Their reasoning behind this approach (as opposed to just adding the index to the MsgVoteOnObservedInboundTx message) was to not unnecessarily add another parameter to the MsgVoteOnObservedInboundTx message just for the purpose of computing a unique hash digest.

func (k Keeper) ProcessZetaSentEvent(ctx sdk.Context, event *connectorzevm.ZetaConnectorZEVMZetaSent, emittingContract ethcommon.Address, txOrigin string) error {
	// [ ... ]

	// Bump gasLimit by event index (which is very unlikely to be larger than 1000) to always have different ZetaSent events msgs.
	msg := zetacoretypes.NewMsgSendVoter("", emittingContract.Hex(), senderChain.ChainId, txOrigin, toAddr, receiverChain.ChainId, amount, "", event.Raw.TxHash.String(), event.Raw.BlockNumber, 90000+uint64(event.Raw.Index), common.CoinType_Zeta, "")

	// [ ... ]
}

3.7 --- Missing authentication when adding node keys

This finding was fixed by removing the public SetNodeKeys message handler completely. This prevents anyone from supplying a public key to be used for TSS signing.

3.8 --- Missing `nil` check when parsing client event

This finding was not fixed. See finding of this report for more details.

3.9 --- Case-sensitive address check allows for double signing

This finding was fixed by completely removing the IsDuplicateSigner() function that was responsible for the case-sensitive address checking.

3.10 --- No panic handler in Zetaclient may halt cross-chain communication

This finding was fixed by adding panic handlers to the Bitcoin client's fetchUTXOs() and TryProcessOutTx() functions.

func (ob *BitcoinChainClient) fetchUTXOS() error {
	defer func() {
		if err := recover(); err != nil {
			ob.logger.WatchUTXOS.Error().Msgf("BTC fetchUTXOS: caught panic error: %v", err)
		}
	}()

    // [ ... ]
}

func (signer *BTCSigner) TryProcessOutTx(send *types.CrossChainTx, outTxMan *OutTxProcessorManager, outTxID string, chainclient ChainClient, zetaBridge *ZetaCoreBridge, height uint64) {
	defer func() {
		if err := recover(); err != nil {
			signer.logger.Error().Msgf("BTC TryProcessOutTx: %s, caught panic error: %v", send.Index, err)
		}
	}()

    // [ ... ]
}

3.11 --- Ethermint Ante handler bypass

This finding was partially fixed, but another bypass was found due to the inclusion of the x/group module. See finding of this report for more details.

3.12 --- Unbonded validators prevent the TSS vote from passing

This finding was fixed by refactoring the CreateTSSVoter() function. It now only allows authorized node accounts (which are created in genesis) to vote on creating a new TSS key.

3.13 --- Code maturity

This finding was acknowledged and partially fixed by removing the commented out code. The remaining FIXMEs and TODOs will continue to be worked on to improve the maturity of the code.