fix/multi-challenger-audit

proto/opinit/ophost/v1/tx.proto

@@ -54,8 +54,8 @@ service Msg {
   // UpdateProposer defines a rpc handler method for MsgUpdateProposer.
   rpc UpdateProposer(MsgUpdateProposer) returns (MsgUpdateProposerResponse);
 
-  // UpdateChallenger defines a rpc handler method for MsgUpdateChallenger.
-  rpc UpdateChallenger(MsgUpdateChallenger) returns (MsgUpdateChallengerResponse);
+  // UpdateChallengers defines a rpc handler method for MsgUpdateChallengers.
+  rpc UpdateChallengers(MsgUpdateChallengers) returns (MsgUpdateChallengersResponse);
 
   // UpdateBatchInfo defines a rpc handler method for MsgUpdateBatchInfo.
   rpc UpdateBatchInfo(MsgUpdateBatchInfo) returns (MsgUpdateBatchInfoResponse);
@@ -220,21 +220,22 @@ message MsgUpdateProposerResponse {
 }
 
 // MsgUpdateChallenger is a message to change a challenger
-message MsgUpdateChallenger {
+message MsgUpdateChallengers {
   option (cosmos.msg.v1.signer) = "authority";
-  option (amino.name)           = "ophost/MsgUpdateChallenger";
+  option (amino.name)           = "ophost/MsgUpdateChallengers";
 
   // authority is the address that controls the module (defaults to x/gov unless overwritten)
-  // or the current challenger address.(supports a single challenger that can replace the existing challenger,
-  // excluding their own address)
+  // or the current challenger address.
+  //
+  // If the given authority is a challenger address, it has the ability to replace itself with another address.
   string authority = 1 [(gogoproto.moretags) = "yaml:\"authority\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
   uint64 bridge_id = 2 [(gogoproto.moretags) = "yaml:\"bridge_id\""];
   repeated string new_challengers = 3
       [(gogoproto.moretags) = "yaml:\"new_challengers\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
 }
 
-// MsgUpdateChallengerResponse returns a message handle result.
-message MsgUpdateChallengerResponse {
+// MsgUpdateChallengersResponse returns a message handle result.
+message MsgUpdateChallengersResponse {
   // last finalized output index
   uint64 output_index = 1;
   // last finalized l2 block number
@@ -268,8 +269,10 @@ message MsgUpdateMetadata {
   option (amino.name)           = "ophost/MsgUpdateMetadata";
 
   // authority is the address that controls the module (defaults to x/gov unless overwritten)
-  // or the current challenger address.(supports a single challenger that can replace the existing challenger,
-  // excluding their own address))
+  // or the current challenger address.
+  //
+  // If the given authority is a challenger address, it has the ability to replace oneself to another address or remove
+  // oneself.
   string authority = 1 [(gogoproto.moretags) = "yaml:\"authority\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
   uint64 bridge_id = 2 [(gogoproto.moretags) = "yaml:\"bridge_id\""];
   bytes  metadata  = 3 [(gogoproto.moretags) = "yaml:\"metadata\""];

proto/opinit/ophost/v1/types.proto

@@ -84,6 +84,6 @@ message Output {
 
 // BatchInfoWithOutput defines the batch information with output.
 message BatchInfoWithOutput {
-  BatchInfo batchInfo = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
-  Output    output    = 2 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
+  BatchInfo batch_info = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
+  Output    output     = 2 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true];
 }

x/opchild/client/cli/tx.go

@@ -261,7 +261,7 @@ func NewSetBridgeInfoCmd(ac address.Codec) *cobra.Command {
 			fmt.Sprintf(
 				`send a tx to set a bridge info with a config file as a json.
 				Example:
-				$ %s tx ophost set-bridge-info 1 init10d07y265gmmuvt4z0w9aw880jnsr700j55nka3 mahalo-2 07-tendermint-0 path/to/bridge-config.json
+				$ %s tx opchild set-bridge-info 1 init10d07y265gmmuvt4z0w9aw880jnsr700j55nka3 mahalo-2 07-tendermint-0 path/to/bridge-config.json
 				
 				Where bridge-config.json contains:
 				{
@@ -318,7 +318,7 @@ func NewSetBridgeInfoCmd(ac address.Codec) *cobra.Command {
 			}
 
 			bridgeConfig := ophosttypes.BridgeConfig{
-				Challengers:         []string{origConfig.Challenger},
+				Challengers:         origConfig.Challengers,
 				Proposer:            origConfig.Proposer,
 				SubmissionInterval:  submissionInterval,
 				FinalizationPeriod:  finalizationPeriod,

x/opchild/client/cli/tx_test.go

@@ -421,7 +421,7 @@ func (s *CLITestSuite) TestNewSetBridgeInfo() {
 
 	invalidConfig.WriteString(`{}`)
 	validConfig.WriteString(`{
-        "challenger": "init1q6jhwnarkw2j5qqgx3qlu20k8nrdglft5ksr0g",
+        "challengers": ["init1q6jhwnarkw2j5qqgx3qlu20k8nrdglft5ksr0g"],
         "proposer": "init1k2svyvm60r8rhnzr9vemk5f6fksvm6tyeujp3c",
         "submission_interval": "100s",
         "finalization_period": "1000s",

x/ophost/client/cli/tx.go

@@ -97,7 +97,7 @@ func NewCreateBridge(ac address.Codec) *cobra.Command {
 				
 				Where bridge-config.json contains:
 				{
-					"challenger": "bech32-address",
+					"challengers": ["bech32-address"],
 					"proposer": "bech32-addresss",
 					"submission_interval": "duration",
 					"finalization_period": "duration",
@@ -142,7 +142,7 @@ func NewCreateBridge(ac address.Codec) *cobra.Command {
 			}
 
 			config := types.BridgeConfig{
-				Challengers:         []string{origConfig.Challenger}, // Ensure Challenger is properly assigned
+				Challengers:         origConfig.Challengers, // Ensure Challenger is properly assigned
 				Proposer:            origConfig.Proposer,
 				SubmissionInterval:  submissionInterval,
 				FinalizationPeriod:  finalizationPeriod,

x/ophost/client/cli/tx_test.go

@@ -164,7 +164,7 @@ func (s *CLITestSuite) TestNewCreateBridge() {
 
 	invalidConfig.WriteString(`{}`)
 	validConfig.WriteString(`{
-        "challenger": "init1q6jhwnarkw2j5qqgx3qlu20k8nrdglft5ksr0g",
+        "challengers": ["init1q6jhwnarkw2j5qqgx3qlu20k8nrdglft5ksr0g"],
         "proposer": "init1k2svyvm60r8rhnzr9vemk5f6fksvm6tyeujp3c",
         "submission_interval": "100s",
         "finalization_period": "1000s",

x/ophost/client/cli/types.go

@@ -3,7 +3,7 @@ package cli
 // BridgeConfig defines the set of bridge config.
 // NOTE: it is a modified BridgeConfig from x/ophost/types/types.go to make unmarshal easier
 type BridgeCliConfig struct {
-	Challenger          string       `json:"challenger"`
+	Challengers         []string     `json:"challengers"`
 	Proposer            string       `json:"proposer"`
 	SubmissionInterval  string       `json:"submission_interval"`
 	FinalizationPeriod  string       `json:"finalization_period"`

x/ophost/keeper/msg_server.go

@@ -184,15 +184,9 @@ func (ms MsgServer) DeleteOutput(ctx context.Context, req *types.MsgDeleteOutput
 	if err != nil {
 		return nil, err
 	}
-	permission := false
-	for _, c := range bridgeConfig.Challengers {
-		if c == challenger {
-			permission = true
-			break
-		}
-	}
+
 	// permission check
-	if !permission {
+	if !slices.Contains(bridgeConfig.Challengers, challenger) {
 		return nil, errors.ErrUnauthorized.Wrap("invalid challenger")
 	}
 
@@ -436,7 +430,7 @@ func (ms MsgServer) UpdateProposer(ctx context.Context, req *types.MsgUpdateProp
 	}, nil
 }
 
-func (ms MsgServer) UpdateChallenger(ctx context.Context, req *types.MsgUpdateChallenger) (*types.MsgUpdateChallengerResponse, error) {
+func (ms MsgServer) UpdateChallengers(ctx context.Context, req *types.MsgUpdateChallengers) (*types.MsgUpdateChallengersResponse, error) {
 	if err := req.Validate(ms.authKeeper.AddressCodec()); err != nil {
 		return nil, err
 	}
@@ -447,23 +441,27 @@ func (ms MsgServer) UpdateChallenger(ctx context.Context, req *types.MsgUpdateCh
 		return nil, err
 	}
 
-	if len(req.NewChallengers) == 0 {
-		return nil, errors.ErrUnauthorized.Wrap("invalid new challengers, at least one new challenger is required")
-	}
+	// permission check
 	if req.Authority == ms.authority {
-		// gov can update multiple challengers
 		config.Challengers = req.NewChallengers
-
 	} else if idx := slices.Index(config.Challengers, req.Authority); idx >= 0 {
+		removedChallengers := []string{}
+		for _, challenger := range config.Challengers {
+			if found := slices.Contains(req.NewChallengers, challenger); !found {
+				removedChallengers = append(removedChallengers, challenger)
+			}
+		}
 
-		// replace byself
-		if len(req.NewChallengers) != 1 {
-			return nil, errors.ErrUnauthorized.Wrap("invalid new challengers, only one new challenger is allowed to replace the old one")
+		originChallengersLen := len(config.Challengers)
+		newChallengersLen := len(req.NewChallengers)
+		removedChallengersLen := len(removedChallengers)
+		if removedChallengersLen != 1 || removedChallengers[0] != req.Authority || originChallengersLen-newChallengersLen < 0 {
+			return nil, types.ErrInvalidChallengerUpdate.Wrap("a challenger can replace only oneself or remove oneself")
 		}
-		config.Challengers[idx] = req.NewChallengers[0]
 
+		config.Challengers = req.NewChallengers
 	} else {
-		return nil, govtypes.ErrInvalidSigner.Wrapf("invalid authority; expected %s or in %s, got %s", ms.authority, config.Challengers, req.Authority)
+		return nil, govtypes.ErrInvalidSigner.Wrapf("invalid authority; expected %s or one in [%s], but got %s", ms.authority, strings.Join(config.Challengers, ","), req.Authority)
 	}
 
 	if err := ms.Keeper.bridgeHook.BridgeChallengersUpdated(ctx, bridgeId, config); err != nil {
@@ -486,7 +484,7 @@ func (ms MsgServer) UpdateChallenger(ctx context.Context, req *types.MsgUpdateCh
 		sdk.NewAttribute(types.AttributeKeyFinalizedL2BlockNumber, strconv.FormatUint(finalizedOutput.L2BlockNumber, 10)),
 	))
 
-	return &types.MsgUpdateChallengerResponse{
+	return &types.MsgUpdateChallengersResponse{
 		OutputIndex:   finalizedOutputIndex,
 		L2BlockNumber: finalizedOutput.L2BlockNumber,
 	}, nil

x/ophost/keeper/msg_server_test.go

@@ -2,7 +2,6 @@ package keeper_test
 
 import (
 	"encoding/hex"
-	"slices"
 	"testing"
 	"time"
 
@@ -283,13 +282,13 @@ func Test_UpdateProposal(t *testing.T) {
 	require.Error(t, err)
 }
 
-func Test_UpdateChallenger(t *testing.T) {
+func Test_UpdateChallengers(t *testing.T) {
 	ctx, input := createDefaultTestInput(t)
 	ms := keeper.NewMsgServerImpl(input.OPHostKeeper)
 
 	config := types.BridgeConfig{
 		Proposer:            addrsStr[0],
-		Challengers:         []string{addrsStr[1]},
+		Challengers:         []string{addrsStr[1], addrsStr[2], addrsStr[3]},
 		SubmissionInterval:  time.Second * 10,
 		FinalizationPeriod:  time.Second * 60,
 		SubmissionStartTime: time.Now().UTC(),
@@ -303,37 +302,69 @@ func Test_UpdateChallenger(t *testing.T) {
 	// gov signer
 	govAddr, err := input.AccountKeeper.AddressCodec().BytesToString(authtypes.NewModuleAddress("gov"))
 	require.NoError(t, err)
-	msg := types.NewMsgUpdateChallenger(govAddr, 1, []string{addrsStr[2], addrsStr[3]})
-	_, err = ms.UpdateChallenger(ctx, msg)
+	msg := types.NewMsgUpdateChallengers(govAddr, 1, []string{addrsStr[2], addrsStr[3]})
+	_, err = ms.UpdateChallengers(ctx, msg)
 	require.NoError(t, err)
 	_config, err := ms.GetBridgeConfig(ctx, 1)
 	require.NoError(t, err)
-	require.True(t, slices.Contains(_config.Challengers, addrsStr[2]))
-	require.True(t, slices.Contains(input.BridgeHook.challengers, addrsStr[2]))
+	require.Equal(t, []string{addrsStr[2], addrsStr[3]}, _config.Challengers)
+	require.Equal(t, input.BridgeHook.challengers, _config.Challengers)
+
 	// current challenger
-	msg = types.NewMsgUpdateChallenger(addrsStr[2], 1, []string{addrsStr[4]})
-	_, err = ms.UpdateChallenger(ctx, msg)
+
+	// case 1. replace oneself
+	msg = types.NewMsgUpdateChallengers(addrsStr[2], 1, []string{addrsStr[3], addrsStr[4]})
+	_, err = ms.UpdateChallengers(ctx, msg)
 	require.NoError(t, err)
 	_config, err = ms.GetBridgeConfig(ctx, 1)
 	require.NoError(t, err)
-	require.True(t, slices.Contains(_config.Challengers, addrsStr[4]))
-	require.True(t, slices.Contains(input.BridgeHook.challengers, addrsStr[4]))
+	require.Equal(t, []string{addrsStr[3], addrsStr[4]}, _config.Challengers)
+	require.Equal(t, input.BridgeHook.challengers, _config.Challengers)
+
+	// case 2. try to remove other challenger
+	msg = types.NewMsgUpdateChallengers(addrsStr[4], 1, []string{addrsStr[4]})
+	_, err = ms.UpdateChallengers(ctx, msg)
+	require.Error(t, err)
+
+	// case 2. try to replace other challenger
+	msg = types.NewMsgUpdateChallengers(addrsStr[4], 1, []string{addrsStr[2], addrsStr[4]})
+	_, err = ms.UpdateChallengers(ctx, msg)
+	require.Error(t, err)
+
+	// case 3. remove oneself
+	msg = types.NewMsgUpdateChallengers(addrsStr[3], 1, []string{addrsStr[4]})
+	_, err = ms.UpdateChallengers(ctx, msg)
+	require.NoError(t, err)
+	_config, err = ms.GetBridgeConfig(ctx, 1)
+	require.NoError(t, err)
+	require.Equal(t, []string{addrsStr[4]}, _config.Challengers)
+	require.Equal(t, input.BridgeHook.challengers, _config.Challengers)
+
+	// case 4. try to add more challenger
+	msg = types.NewMsgUpdateChallengers(addrsStr[4], 1, []string{addrsStr[3], addrsStr[4]})
+	_, err = ms.UpdateChallengers(ctx, msg)
+	require.Error(t, err)
+
+	// case 5. try to add more challenger with replace
+	msg = types.NewMsgUpdateChallengers(addrsStr[4], 1, []string{addrsStr[2], addrsStr[3]})
+	_, err = ms.UpdateChallengers(ctx, msg)
+	require.Error(t, err)
 
 	// invalid signer
 	invalidAddr, err := input.AccountKeeper.AddressCodec().BytesToString(authtypes.NewModuleAddress(types.ModuleName))
 	require.NoError(t, err)
-	msg = types.NewMsgUpdateChallenger(invalidAddr, 1, []string{addrsStr[1]})
+	msg = types.NewMsgUpdateChallengers(invalidAddr, 1, []string{addrsStr[1]})
 	require.NoError(t, err)
 
-	_, err = ms.UpdateChallenger(
+	_, err = ms.UpdateChallengers(
 		ctx,
 		msg,
 	)
 	require.Error(t, err)
 
 	// invalid case
-	msg = types.NewMsgUpdateChallenger(govAddr, 1, []string{})
-	_, err = ms.UpdateChallenger(ctx, msg)
+	msg = types.NewMsgUpdateChallengers(govAddr, 1, []string{})
+	_, err = ms.UpdateChallengers(ctx, msg)
 	require.Error(t, err)
 }
 

x/ophost/types/bridge_config.go

@@ -10,11 +10,17 @@ import (
 )
 
 func (config BridgeConfig) Validate(ac address.Codec) error {
-	var err error
+	challengerDupMap := make(map[string]bool, len(config.Challengers))
 	for _, challenger := range config.Challengers {
-		if _, err = ac.StringToBytes(challenger); err != nil {
+		if _, err := ac.StringToBytes(challenger); err != nil {
 			return err
 		}
+
+		if _, ok := challengerDupMap[challenger]; ok {
+			return errors.Wrapf(sdkerrors.ErrInvalidRequest, "challengers must be unique")
+		}
+
+		challengerDupMap[challenger] = true
 	}
 
 	if _, err := ac.StringToBytes(config.Proposer); err != nil {

x/ophost/types/codec.go

@@ -18,7 +18,7 @@ func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
 	legacy.RegisterAminoMsg(cdc, &MsgInitiateTokenDeposit{}, "ophost/MsgInitiateTokenDeposit")
 	legacy.RegisterAminoMsg(cdc, &MsgFinalizeTokenWithdrawal{}, "ophost/MsgFinalizeTokenWithdrawal")
 	legacy.RegisterAminoMsg(cdc, &MsgUpdateProposer{}, "ophost/MsgUpdateProposer")
-	legacy.RegisterAminoMsg(cdc, &MsgUpdateChallenger{}, "ophost/MsgUpdateChallenger")
+	legacy.RegisterAminoMsg(cdc, &MsgUpdateChallengers{}, "ophost/MsgUpdateChallengers")
 	legacy.RegisterAminoMsg(cdc, &MsgUpdateBatchInfo{}, "ophost/MsgUpdateBatchInfo")
 	legacy.RegisterAminoMsg(cdc, &MsgUpdateParams{}, "ophost/MsgUpdateParams")
 
@@ -36,7 +36,7 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
 		&MsgInitiateTokenDeposit{},
 		&MsgFinalizeTokenWithdrawal{},
 		&MsgUpdateProposer{},
-		&MsgUpdateChallenger{},
+		&MsgUpdateChallengers{},
 		&MsgUpdateBatchInfo{},
 		&MsgUpdateParams{},
 	)

x/ophost/types/error.go

@@ -19,4 +19,5 @@ var (
 	ErrEmptyBatchInfo             = errorsmod.Register(ModuleName, 12, "empty batch info")
 	ErrInvalidBridgeMetadata      = errorsmod.Register(ModuleName, 13, "invalid bridge metadata")
 	ErrInvalidBatchInfo           = errorsmod.Register(ModuleName, 14, "invalid batch info")
+	ErrInvalidChallengerUpdate    = errorsmod.Register(ModuleName, 15, "invalid challenger update")
 )

x/ophost/types/genesis.go

@@ -72,6 +72,7 @@ func ValidateGenesis(data *GenesisState, ac address.Codec) error {
 		if len(bridge.BatchInfos) == 0 {
 			return ErrEmptyBatchInfo
 		}
+
 		// last batchinfo should be same with bridgeconfig batchinfo
 		if bridge.BatchInfos[len(bridge.BatchInfos)-1].BatchInfo != bridge.BridgeConfig.BatchInfo ||
 			!bridge.BatchInfos[0].Output.IsEmpty() {