`AddBlockHeader` Cannot Cope with Reorgs

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/observer/keeper/msg_server_add_block_header.go#L14

Vulnerability details

Impact

AddBlockHeader handles adding a block header to the node's store, through majority voting of observers. Reorgs happen, at different rates and depths for different chains and network conditions, but they do happen. And not only defensive measures must be taken to deal with them, but also remediation measures in case they occur more aggressively than expected. The problem is that a critical function to remove or reorganise the blocks is somehow missing from the external interface.

This submission is not about the estimation of financial loss or impact of reorgs, but the clear technical fact that blocks cannot be removed out of the store or somehow reorganised in case of emergency.

Proof of Concept

The AddBlockHeader function is only called by observers after a certain number of confirmations is reached in the clients; 2 for Bitcoin, 14 for Ethereum, and 14 for BSC. As I intend to submit this topic in a Low/QA report, in this submission I will ignore that I find those confirmation numbers dangerously low for a software like ZetaChain.

Though, even if we consider that chances of reorgs past those confirmation numbers are low, they occur. And when they do occur beyond the expected safety parameters, and depending on the reorg depth, ZetaChain cannot possibly avoid the resultant financial damage. But there must be a plan in place and critical functions available to deal with the necessary actions to guarantee operational continuity. And none could be found. There is only a keeper function RemoveBlockHeader, but, oddly, this is not used anywhere in the codebase.

Currently, ZetaChain cannot operate normally if there is need to remove or reorganise some block(s) in the store. For example, suppose there is a reorg beyond the clients' (very minimal) safety parameters and the block headers have already been added. Financial damage may be inevitable, but observers should be able to vote on a solution to the conundrum so ZetaChain can continue to extend its consensus about the connected chains. What happens if observers try to use AddBlockHeader to vote in a new version of the chain, by attempting to add a block header for the height that has just been added? The function will return an error:

• https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/observer/keeper/msg_server_add_block_header.go#L57C1-L59C4

      if msg.Height != bhs.LatestHeight+1 {
          return nil, cosmoserrors.Wrap(types.ErrNoParentHash, fmt.Sprintf("invalid block height: wanted %d, got %d", bhs.LatestHeight+1, msg.Height))
      }

While most checks will pass, the check that the block header being added is of height equal to latest height plus one will not, and the block will not be added. There are a couple of hacky ways to force the block to be added with wrong parameters. But that would not be a seriously designed remediation measure. Fundamentally, operations regarding the affected chain will be halted.

Tools Used

Manual: code editor.

Recommended Mitigation Steps

There must be a function, be it through observers voting or admin group action, to account for the possible reorganisation of the chain.

The RemoveBlockHeader function may be used:

• https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/observer/keeper/block_header.go#L36C1-L40C2

  // RemoveBlockHeader removes a block header from the store
  func (k Keeper) RemoveBlockHeader(ctx sdk.Context, hash []byte) {
  store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefix(types.BlockHeaderKey))
  store.Delete(hash)
  }

Though, there must be a logic somewhere, maybe in the RemoveBlockHeader message server's function, to also take care of the BlockHeaderState when a block is removed from the store.

Alternatively, altere the AddBlockHeader to accept voting for an earlier height, so the block headers can be reorganised to mirror the chain reorg.

Assessed type

Other

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

Can use SetBlockHeader to achieve the same goal

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid

[ciphermarco]

Hi, @0xean. Could you, please, have a second look at this?

The point of my issue is that there's nothing implemented for re-organising blocks in case of need. Neither of the keeper functions RemoveBlockHeader and SetBlockHeader are exposed by the message server to be used by the consensus operations to swap blocks in the storage in case of reorgs.

This is also not a theoretical issue about the impact of very specific or deep reorgs types. Small reorgs are inevitable and a normal occurrence in this case, and the core of the issue is that there is no implemented way for peers to cope with that and reach consensus for a new tip block. More specifically, as presented above, AddBlockHeader only allows new blocks to be one above the latest block's height (msg.Height != bhs.LatestHeight+1), completely blocking any attempt to reach consensus for a new tip.

Thank you for reviewing this.

[0xean]

Gonna also ask for sponsor comment on this prior to final judging. @lumtis

[brewmaster012]

thanks for the report this is a valid issue however the severity/impact is low as we do not currently rely on block headers and there is no path of exploit.

[0xean]

@ciphermarco - want to make one last comment re: your thoughts on impact before this gets downgraded to QA?

[ciphermarco]

@0xean - thank you for the opportunity.

we do not currently rely on block headers

Without trying to infer intentions from the code itself, the Important Areas documentation for this audit contest reads:

A new architecture has been developed in order to validate txs from external chains permissionlessly instead of requiring observers to vote on each of them.

• Observer votes on block headers of the external chains instead of the txs. The block headers are stored on-chain on ZetaChain
• 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

I think the documentation disagrees with this statment and that's all I can fairly work with for the audit contest. Of course, unless I misunderstood the documentation.

there is no path of exploit

There are " hypothetical attack paths with stated assumptions, but external requirements.", but I don't show any in my report, so I will not argue for them now. I was focusing on the more easily provable type of impact in this issue, as highlighted below:

2 — Med: ~Assets not at direct risk, but~ the function of the protocol or its availability could be impacted, ~or leak value with a hypothetical attack path with stated assumptions, but external requirements.~

I separated the concepts with the comma and didn't think impacting the function of the protocol or its availability needed to show an attack path besides proving a real risk. Given that reorgs are a normal occurrence, the protocol cannot respond to network realities with this limitation, making the risk real and inevitable.

But I agree there's no attack path presented in my report and, if that makes it QA, the rule is the rule and thank you for the fair judging.

Quick edit: I was studying the issues selected for report and I see some of the M's don't have a "path of exploit" or "attack path" shown. Is that really a requirement? Thank you and no more edits or comments :+1: .

[0xean]

Going to go with M on this one.

It does show that the new architecture is flawed when dealing with larger re-orgs, and I also do think that it would break the ability for the trust-less proofs to be verified against the header.

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report