fix: multi challenger audit (#86)

* update merkle logic to prevent second preimage attack (#83)

* use latest slinky and use clear to delete all validators

* go mod tidy

* fix receiver proto index

* fix/multi-challenger-audit

go.mod

@@ -27,7 +27,7 @@ require (
 	github.com/initia-labs/OPinit/api v0.3.0
 	github.com/pkg/errors v0.9.1
 	github.com/skip-mev/block-sdk/v2 v2.1.1
-	github.com/skip-mev/slinky v0.4.2
+	github.com/skip-mev/slinky v0.4.3
 	github.com/spf13/cobra v1.8.0
 	github.com/stretchr/testify v1.9.0
 	golang.org/x/crypto v0.23.0

go.sum

@@ -1053,8 +1053,8 @@ github.com/skip-mev/block-sdk/v2 v2.1.1 h1:Audcf6Dtev16JRewV8M8GfjEDDsu6ayxlz9GH
 github.com/skip-mev/block-sdk/v2 v2.1.1/go.mod h1:AOjFICNrPpq/Cq1f97CcC7ljWhxCzmfmyz4/Ir8/xFM=
 github.com/skip-mev/chaintestutil v0.0.0-20240116134208-3e49bf514803 h1:VRRVYN3wsOIOqVT3e3nDh3vyUl6RvF9QwdK4BvgPP9c=
 github.com/skip-mev/chaintestutil v0.0.0-20240116134208-3e49bf514803/go.mod h1:LF2koCTmygQnz11yjSfHvNP8axdyZ2lTEw0EwI+dnno=
-github.com/skip-mev/slinky v0.4.2 h1:YOYPR+1HI4u8tCVHjytAPn59zRkZBRbZhNecTsyG/Vk=
-github.com/skip-mev/slinky v0.4.2/go.mod h1:6LkqFirdIh1FfHijGyRf1MeiBgVshDseTH/jHTOVQVs=
+github.com/skip-mev/slinky v0.4.3 h1:4LWqeDa2and84GXG3HgAAmDa145+TG2WQ0uRDAU7AzA=
+github.com/skip-mev/slinky v0.4.3/go.mod h1:6LkqFirdIh1FfHijGyRf1MeiBgVshDseTH/jHTOVQVs=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
 github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/soheilhy/cmux v0.1.4/go.mod h1:IM3LyeVVIOuxMH7sFAkER9+bJ4dT7Ms6E4xg4kGIyLM=

go.work.sum

@@ -977,6 +977,8 @@ github.com/sivchari/containedctx v1.0.3/go.mod h1:c1RDvCbnJLtH4lLcYD/GqwiBSSf4F5
 github.com/sivchari/nosnakecase v1.7.0/go.mod h1:CwDzrzPea40/GB6uynrNLiorAlgFRvRbFSgJx2Gs+QY=
 github.com/sivchari/tenv v1.7.1/go.mod h1:64yStXKSOxDfX47NlhVwND4dHwfZDdbp2Lyl018Icvg=
 github.com/skeema/knownhosts v1.2.1/go.mod h1:xYbVRSPxqBZFrdmDyMmsOs+uX1UZC3nTN3ThzgDxUwo=
+github.com/skip-mev/slinky v0.4.3 h1:4LWqeDa2and84GXG3HgAAmDa145+TG2WQ0uRDAU7AzA=
+github.com/skip-mev/slinky v0.4.3/go.mod h1:6LkqFirdIh1FfHijGyRf1MeiBgVshDseTH/jHTOVQVs=
 github.com/snikch/goodman v0.0.0-20171125024755-10e37e294daa/go.mod h1:oJyF+mSPHbB5mVY2iO9KV3pTt/QbIkGaO8gQ2WrDbP4=
 github.com/sonatard/noctx v0.0.2/go.mod h1:kzFz+CzWSjQ2OzIm46uJZoXuBpa2+0y3T36U18dWqIo=
 github.com/sourcegraph/go-diff v0.7.0/go.mod h1:iBszgVvyxdc8SFZ7gm69go2KDdt3ag071iBaWPF6cjs=

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);
@@ -177,8 +177,8 @@ message MsgFinalizeTokenWithdrawal {
 
   // withdraw tx data
 
-  string sender   = 1 [(gogoproto.moretags) = "yaml:\"sender\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
-  string receiver = 5 [(gogoproto.moretags) = "yaml:\"receiver\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
+  string sender   = 5 [(gogoproto.moretags) = "yaml:\"sender\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
+  string receiver = 1 [(gogoproto.moretags) = "yaml:\"receiver\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
   uint64 sequence = 6 [(gogoproto.moretags) = "yaml:\"sequence\""];
   cosmos.base.v1beta1.Coin amount = 7
       [(gogoproto.moretags) = "yaml:\"amount\"", (gogoproto.nullable) = false, (amino.dont_omitempty) = true];
@@ -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/opchild/keeper/host_validator_store.go

@@ -128,12 +128,7 @@ func (hv HostValidatorStore) GetAllValidators(ctx context.Context) (validators [
 }
 
 func (hv HostValidatorStore) DeleteAllValidators(ctx context.Context) error {
-	return hv.validators.Walk(ctx, nil, func(key []byte, _ stakingtypes.Validator) (stop bool, err error) {
-		if err := hv.validators.Remove(ctx, key); err != nil {
-			return true, err
-		}
-		return false, nil
-	})
+	return hv.validators.Clear(ctx, nil)
 }
 
 func (hv HostValidatorStore) GetLastHeight(ctx context.Context) (int64, error) {

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")
 	}
 
@@ -340,7 +334,9 @@ func (ms MsgServer) FinalizeTokenWithdrawal(ctx context.Context, req *types.MsgF
 			seed = append(seed, types.Splitter)
 			seed = binary.BigEndian.AppendUint64(seed, amount.Uint64())
 
+			// double hash the leaf node
 			withdrawalHash = sha3.Sum256(seed)
+			withdrawalHash = sha3.Sum256(withdrawalHash[:])
 		}
 
 		if ok, err := ms.HasProvenWithdrawal(ctx, bridgeId, withdrawalHash); err != nil {
@@ -436,7 +432,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 +443,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 +486,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