Broken NonceVoter Allows Observer to Halt the Chain

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

As it is, NonceVoter does not count votes or perform any similar operations. A single observer can use it to change chain nonces. Although there is a comment pointing this function is deprecated, it critically influences the system with its broken implementation. Arbitrarily changing nonces for the connected chains can be used to completely break the functioning of the chain and cause loss of funds.

Proof of Concept

This function is restricted to validator observers (k.zetaObserverKeeper.IsAuthorized(ctx, msg.Creator, chain)):

• https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/keeper_chain_nonces.go#L106C1-L115C3

func (k msgServer) NonceVoter(goCtx context.Context, msg *types.MsgNonceVoter) (*types.MsgNonceVoterResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)
	chain := k.zetaObserverKeeper.GetParams(ctx).GetChainFromChainID(msg.ChainId)
	if chain == nil {
		return nil, observertypes.ErrSupportedChains
	}

	if ok := k.zetaObserverKeeper.IsAuthorized(ctx, msg.Creator, chain); !ok {
		return nil, observertypes.ErrNotAuthorizedPolicy
	}
	// ... SNIP ...

After this check, the entire voting logic is essentially ignored, with a relevant part only drafted and commented, and the chain nonce is simply set:

• https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/keeper_chain_nonces.go#L145C1-L146C44

	k.SetChainNonces(ctx, chainNonce)
	return &types.MsgNonceVoterResponse{}, nil

This can be easily tested inside one of the hosts in the smoke-tests:

# cd into repos/node
$ make start-smoketest
$ docker exec -it zetacore0 bash
# Inside zetacore0
# The operator key may be acquired with the command `zetacored keys list`
$ zetacored tx crosschain nonce-voter 18444 1337 --from ${operator_key} --fees "300azeta"
# Confirm the prompt with "y"
$ zetacored q crosschain list-chain-nonces
# OUTPUT:
ChainNonces:
- chain_id: "18444"
  creator: ""
  finalizedHeight: "57"
  index: btc_regtest
  nonce: "1337" # <--- Chain nonce
  signers:
  - zeta1t4n5yj0u3wjkjwum5exhgh5834gnylgs3apmw5
- chain_id: "1337"
  creator: ""
  finalizedHeight: "57"
  index: goerli_localnet
  nonce: "0"
  signers: []
- chain_id: "101"
  creator: ""
  finalizedHeight: "57"
  index: zeta_mainnet
  nonce: "0"
  signers: []
pagination:
  next_key: null
  total: "0"

After the above commands, you can also use the command zetacored q crosschain list-chain-nonces inside zetacored1 to check the 1337 nonce has been set.

p.NonceHigh != int64(nonce.Nonce)

One simple way to halt critical operations of the chain and cause funds to be stuck is to make the function UpdateNonce in cctx_utils.go fail. Luckily, there is a check that causes this function to fail and return an error, and all we need to do is to change the chain nonce:

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

	// #nosec G701 always in range
	if p.NonceHigh != int64(nonce.Nonce) {
		return cosmoserrors.Wrap(types.ErrNonceMismatch, fmt.Sprintf("chain_id %d, high nonce %d, current nonce %d", receiveChainID, p.NonceHigh, nonce.Nonce))
	}

All a faulty or compromised observer needs to do is to use NonceVoter to desyncrhonise the nonces for each chain, and this error will be consistently returned by UpdateNonce for all chains.

These are critical functions that depend on UpdateNonce:

• MigrateTSSFundsForChain
• ProcessCCTX
• VoteOnObservedInboundTx
• VoteOnObservedOutboundTx
  *WhitelistERC20

Essentially, transactions will be halted and the funds stuck.

Tools Used

Manual: code editor, docker.

Recommended Mitigation Steps

Implement the correct voting logic or remove this message server's function from the code. In the codebase, it seems there is already another design choice for setting chain nonces that does not depend on this function, so the best mitigation is probably to remove this function altogether.

Assessed type

Access Control

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #216

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-judge]

0xean marked the issue as selected for report

[c4-judge]

0xean marked the issue as satisfactory

[0xean]

sponsor already reviewed, see #216

[c4-sponsor]

lumtis (sponsor) confirmed