Policy_Type_group1 Can Activate flags.BlockHeaderVerificationFlags

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

ZetaChain implements a critical separation of privileges by using two types of Admin Groups. According to "Important Areas" documentation and the implementation itself, certain actions can only be executed by one group or the other.

• Group1 is assigned for emergency actions that need rapidity over security, and is represented by an address that requires a single signature from its multisig scheme.

• Group2 is assigned for operational actions influencing the logic of the blockchain that need security over rapidity, and is represented by an address that requires several signatures from its multisig scheme.

The issue is that Group1 is allowed to enable/disable the crosschain flag flags.BlockHeaderVerificationFlags. This not only differs from the "Important Areas" documentation, but is also a violation of the designed separation of privileges. More specifically, enabling the flags.BlockHeaderVerificationFlags cannot be justified as an emergency action and activates (instead of deactivating in case of emergency) critical operations in the blockchain. This renders a critical flag for the blockchain operation mistakenly open to only one address in the Group1 multisig.

Proof of Concept

This is the relevant (fixed) part of the "Permissions Table" in the "Important Areas" documentation:

Message	Group	Description	Module
MsgUpdateCrosschainFlags - Disabling outbounds/inbounds	1	Disable inbound txs or outbound txs to be observed on ZetaChain	observer
MsgUpdateCrosschainFlags - Other actions	2	Other actions for crosschain flags are: enabling disabled outbound/inbound, changing parameters for gas price increase mechanism	observer

Admins in the Group1 should only be able to disable outbounds/inbounds for the case of emergencies, while all other actions should be reserved to Group2. But this is not what the code in the UpdateCrosschainFlags message function enforces:

• https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/observer/keeper/msg_server_update_crosschain_flags.go#L12-L58

func (k msgServer) UpdateCrosschainFlags(goCtx context.Context, msg *types.MsgUpdateCrosschainFlags) (*types.MsgUpdateCrosschainFlagsResponse, error) {
	ctx := sdk.UnwrapSDKContext(goCtx)

	requiredGroup := types.Policy_Type_group1
	if msg.IsInboundEnabled || msg.IsOutboundEnabled || msg.GasPriceIncreaseFlags != nil {
		requiredGroup = types.Policy_Type_group2
	}

	// check permission
	if msg.Creator != k.GetParams(ctx).GetAdminPolicyAccount(requiredGroup) {
		return &types.MsgUpdateCrosschainFlagsResponse{}, types.ErrNotAuthorizedPolicy
	}
    // ... SNIP...
	if msg.BlockHeaderVerificationFlags != nil {
		flags.BlockHeaderVerificationFlags = msg.BlockHeaderVerificationFlags
	}

	k.SetCrosschainFlags(ctx, flags)
    // ... SNIP ...

The flags.BlockHeaderVerificationFlags can be switched by both Group1 and Group2.

These flags are used in:

• msg_server_add_block_header.go, to enable or disable the addition of new block headers for each chain for voting;
• verify_proof.go, to enable or disable the proof verification. Effectively, this enables or disables users from submitting transactions proofs for each chain to be added to the trackers via msg_server_add_to_intx_tracker.go and msg_server_add_to_outtx_tracker.go.

A simple unit test can be included in contest repository's file repos/node/x/observer/keeper/msg_server_update_crosschain_flags_test.go to test for the authorization flaw:

func TestMsgServer_UpdateBlockHeaderVerificationFlagsAUDIT(t *testing.T) {
	t.Run("Group1 can update BlockHeaderVerificationFlags", func(t *testing.T) {
		k, ctx := keepertest.ObserverKeeper(t)
		srv := keeper.NewMsgServerImpl(*k)
		admin := sample.AccAddress()

		setAdminCrossChainFlags(ctx, k, admin, types.Policy_Type_group1)

		// Group1 can change flags to `false`
		_, err := srv.UpdateCrosschainFlags(sdk.WrapSDKContext(ctx), &types.MsgUpdateCrosschainFlags{
			Creator: admin,
			BlockHeaderVerificationFlags: &types.BlockHeaderVerificationFlags{
				IsEthTypeChainEnabled: false,
				IsBtcTypeChainEnabled: false,
			},
		})

		require.NoError(t, err)
		flags, found := k.GetCrosschainFlags(ctx)
		require.True(t, found)
		require.False(t, flags.BlockHeaderVerificationFlags.IsEthTypeChainEnabled)
		require.False(t, flags.BlockHeaderVerificationFlags.IsBtcTypeChainEnabled)

		// Group1 can change flags to `true`
		_, err = srv.UpdateCrosschainFlags(sdk.WrapSDKContext(ctx), &types.MsgUpdateCrosschainFlags{
			Creator: admin,
			BlockHeaderVerificationFlags: &types.BlockHeaderVerificationFlags{
				IsEthTypeChainEnabled: true,
				IsBtcTypeChainEnabled: true,
			},
		})

		require.NoError(t, err)
		flags, found = k.GetCrosschainFlags(ctx)
		require.True(t, found)
		require.True(t, flags.BlockHeaderVerificationFlags.IsEthTypeChainEnabled)
		require.True(t, flags.BlockHeaderVerificationFlags.IsBtcTypeChainEnabled)
	})
}

Then run:

$ go test x/observer/keeper/msg_server_update_crosschain_flags_test.go -run TestMsgServer_UpdateBlockHeaderVerificationFlagsAUDIT

Tools Used

Manual: code editor.

Recommended Mitigation Steps

One potential solution that seems to adhere to the intended security model is to emulate what is done to the other critical flags and allow Group1 to only disable the flags.BlockHeaderVerificationFlags, for the cases of emergency situations. Or, if intended, just remove completely the Group1 permission to set flags.BlockHeaderVerificationFlags.

Assessed type

Access Control

[DadeKuma]

QA might be more appropriate

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[0xean]

going to leave open for sponsor comment before downgrading.

[c4-sponsor]

lumtis (sponsor) confirmed

[lumtis]

Confirm validity. Need to be updated.

[0xean]

while this is very well documented issue, I don't see how it is more than QA at this point.

It is essentially functionality not meeting the specification with little impact beyond that as both of these calls are privileged already.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a