fix(sequencers): incorrect sorting mechanism allows manipulation of p…

…roposer selection (#1292)

x/sequencer/keeper/invariants.go

@@ -107,6 +107,13 @@ func SequencerPositiveBalancePostBondReduction(k Keeper) sdk.Invariant {
 		sequencers := k.GetAllSequencers(ctx)
 		for _, seq := range sequencers {
 			effectiveBond := seq.Tokens
+
+			// assert single denom for bond
+			if effectiveBond.Len() != 1 {
+				broken = true
+				msg += "sequencer has multiple denoms " + seq.Address + "\n"
+			}
+
 			if bondReductions := k.GetBondReductionsBySequencer(ctx, seq.Address); len(bondReductions) > 0 {
 				for _, bd := range bondReductions {
 					effectiveBond = effectiveBond.Sub(bd.DecreaseBondAmount)

x/sequencer/keeper/msg_server_create_sequencer.go

@@ -63,28 +63,22 @@ func (k msgServer) CreateSequencer(goCtx context.Context, msg *types.MsgCreateSe
 		}
 	}
 
-	bond := sdk.Coins{}
-	if minBond := k.GetParams(ctx).MinBond; !(minBond.IsNil() || minBond.IsZero()) {
-		if msg.Bond.Denom != minBond.Denom {
-			return nil, errorsmod.Wrapf(
-				types.ErrInvalidCoinDenom, "got %s, expected %s", msg.Bond.Denom, minBond.Denom,
-			)
-		}
-
-		if msg.Bond.Amount.LT(minBond.Amount) {
-			return nil, errorsmod.Wrapf(
-				types.ErrInsufficientBond, "got %s, expected %s", msg.Bond.Amount, minBond,
-			)
-		}
+	// validate bond requirement
+	minBond := k.GetParams(ctx).MinBond
+	if !msg.Bond.IsGTE(minBond) {
+		return nil, errorsmod.Wrapf(
+			types.ErrInsufficientBond, "got %s, expected %s", msg.Bond, minBond,
+		)
+	}
 
-		seqAcc := sdk.MustAccAddressFromBech32(msg.Creator)
-		err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, seqAcc, types.ModuleName, sdk.NewCoins(msg.Bond))
-		if err != nil {
-			return nil, err
-		}
-		bond = sdk.NewCoins(msg.Bond)
+	// send bond to module account
+	seqAcc := sdk.MustAccAddressFromBech32(msg.Creator)
+	err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, seqAcc, types.ModuleName, sdk.NewCoins(msg.Bond))
+	if err != nil {
+		return nil, err
 	}
 
+	bond := sdk.NewCoins(msg.Bond)
 	sequencer := types.Sequencer{
 		Address:      msg.Creator,
 		DymintPubKey: msg.DymintPubKey,

x/sequencer/keeper/msg_server_create_sequencer_test.go

@@ -1,10 +1,12 @@
 package keeper_test
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"time"
 
+	errorsmod "cosmossdk.io/errors"
 	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -25,75 +27,83 @@ const (
 var bond = types.DefaultParams().MinBond
 
 func (suite *SequencerTestSuite) TestMinBond() {
+	panicErr := errors.New("panic")
+
 	testCases := []struct {
 		name          string
 		requiredBond  sdk.Coin
 		bond          sdk.Coin
 		expectedError error
 	}{
-		{
-			name:          "No bond required",
-			requiredBond:  sdk.Coin{},
-			bond:          sdk.NewCoin("adym", sdk.NewInt(10000000)),
-			expectedError: nil,
-		},
 		{
 			name:          "Valid bond",
 			requiredBond:  bond,
 			bond:          bond,
 			expectedError: nil,
 		},
-		{
-			name:          "Bad denom",
-			requiredBond:  bond,
-			bond:          sdk.NewCoin("invalid", sdk.NewInt(100)),
-			expectedError: types.ErrInvalidCoinDenom,
-		},
 		{
 			name:          "Insufficient bond",
 			requiredBond:  bond,
 			bond:          sdk.NewCoin(bond.Denom, bond.Amount.Quo(sdk.NewInt(2))),
 			expectedError: types.ErrInsufficientBond,
 		},
+		{
+			name:          "wrong bond denom",
+			requiredBond:  bond,
+			bond:          sdk.NewCoin("nonbonddenom", bond.Amount),
+			expectedError: panicErr,
+		},
 	}
 
 	for _, tc := range testCases {
-		seqParams := types.DefaultParams()
-		seqParams.MinBond = tc.requiredBond
-		suite.App.SequencerKeeper.SetParams(suite.Ctx, seqParams)
+		suite.Run(tc.name, func() {
+			seqParams := types.DefaultParams()
+			seqParams.MinBond = tc.requiredBond
+			suite.App.SequencerKeeper.SetParams(suite.Ctx, seqParams)
 
-		rollappId, pk := suite.CreateDefaultRollapp()
+			rollappId, pk := suite.CreateDefaultRollapp()
 
-		// fund account
-		addr := sdk.AccAddress(pk.Address())
-		pkAny, err := codectypes.NewAnyWithValue(pk)
-		suite.Require().Nil(err)
-		err = bankutil.FundAccount(suite.App.BankKeeper, suite.Ctx, addr, sdk.NewCoins(tc.bond))
-		suite.Require().Nil(err)
+			// fund account
+			addr := sdk.AccAddress(pk.Address())
+			pkAny, err := codectypes.NewAnyWithValue(pk)
+			suite.Require().Nil(err)
+			err = bankutil.FundAccount(suite.App.BankKeeper, suite.Ctx, addr, sdk.NewCoins(tc.bond))
+			suite.Require().Nil(err)
 
-		sequencerMsg1 := types.MsgCreateSequencer{
-			Creator:      addr.String(),
-			DymintPubKey: pkAny,
-			Bond:         bond,
-			RollappId:    rollappId,
-			Metadata: types.SequencerMetadata{
-				Rpcs: []string{"https://rpc.wpd.evm.rollapp.noisnemyd.xyz:443"},
-			},
-		}
-		_, err = suite.msgServer.CreateSequencer(suite.Ctx, &sequencerMsg1)
-		if tc.expectedError != nil {
-			tc := tc
-			suite.Require().ErrorAs(err, &tc.expectedError, tc.name)
-		} else {
-			suite.Require().NoError(err)
-			sequencer, found := suite.App.SequencerKeeper.GetSequencer(suite.Ctx, addr.String())
-			suite.Require().True(found, tc.name)
-			if tc.requiredBond.IsNil() {
-				suite.Require().True(sequencer.Tokens.IsZero(), tc.name)
+			sequencerMsg1 := types.MsgCreateSequencer{
+				Creator:      addr.String(),
+				DymintPubKey: pkAny,
+				Bond:         tc.bond,
+				RollappId:    rollappId,
+				Metadata: types.SequencerMetadata{
+					Rpcs: []string{"https://rpc.wpd.evm.rollapp.noisnemyd.xyz:443"},
+				},
+			}
+
+			// Use a defer and recover to catch potential panics
+			var createErr error
+			func() {
+				defer func() {
+					if r := recover(); r != nil {
+						createErr = errorsmod.Wrapf(panicErr, "panic: %v", r)
+					}
+				}()
+				_, createErr = suite.msgServer.CreateSequencer(suite.Ctx, &sequencerMsg1)
+			}()
+
+			if tc.expectedError != nil {
+				suite.Require().ErrorAs(createErr, &tc.expectedError, tc.name)
 			} else {
-				suite.Require().Equal(sdk.NewCoins(tc.requiredBond), sequencer.Tokens, tc.name)
+				suite.Require().NoError(createErr)
+				sequencer, found := suite.App.SequencerKeeper.GetSequencer(suite.Ctx, addr.String())
+				suite.Require().True(found, tc.name)
+				if tc.requiredBond.IsNil() {
+					suite.Require().True(sequencer.Tokens.IsZero(), tc.name)
+				} else {
+					suite.Require().Equal(sdk.NewCoins(tc.requiredBond), sequencer.Tokens, tc.name)
+				}
 			}
-		}
+		})
 	}
 }
 

x/sequencer/keeper/msg_server_increase_bond.go

@@ -29,6 +29,11 @@ func (k msgServer) IncreaseBond(goCtx context.Context, msg *types.MsgIncreaseBon
 		return nil, err
 	}
 
+	// validate the addition amt is of same denom of existing bond
+	if found, _ := sequencer.Tokens.Find(msg.AddAmount.Denom); !found {
+		return nil, types.ErrInvalidCoinDenom
+	}
+
 	// update the sequencers bond amount in state
 	sequencer.Tokens = sequencer.Tokens.Add(msg.AddAmount)
 	k.UpdateSequencer(ctx, &sequencer, sequencer.Status)

x/sequencer/keeper/unbond.go

@@ -1,6 +1,7 @@
 package keeper
 
 import (
+	"fmt"
 	"time"
 
 	errorsmod "cosmossdk.io/errors"
@@ -73,6 +74,9 @@ func (k Keeper) reduceSequencerBond(ctx sdk.Context, seq *types.Sequencer, amt s
 	if amt.IsZero() {
 		return nil
 	}
+	if !seq.Tokens.IsAllGTE(amt) {
+		return fmt.Errorf("sequencer does not have enough bond: got %s, reducing by %s", seq.Tokens.String(), amt.String())
+	}
 	if burn {
 		err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, amt)
 		if err != nil {

x/sequencer/types/msg_create_sequencer.go

@@ -100,7 +100,7 @@ func (msg *MsgCreateSequencer) ValidateBasic() error {
 		return errorsmod.Wrap(ErrInvalidMetadata, err.Error())
 	}
 
-	if !msg.Bond.IsValid() {
+	if !msg.Bond.IsValid() || msg.Bond.IsZero() {
 		return errorsmod.Wrapf(ErrInvalidCoins, "invalid bond amount: %s", msg.Bond.String())
 	}
 

x/sequencer/types/params.go

@@ -62,7 +62,7 @@ func validateMinBond(i interface{}) error {
 	}
 
 	if v.IsNil() || v.IsZero() {
-		return nil
+		return fmt.Errorf("min bond must be positive: %s", v)
 	}
 
 	if !v.IsValid() {