fix(group): add group members weight checks (backport #13869) (#13880)

* fix(group): add group members weight checks (#13869) (cherry picked from commit 3423442) # Conflicts: # CHANGELOG.md # api/cosmos/tx/v1beta1/service.pulsar.go # types/tx/service.pb.go # x/group/keeper/keeper.go * fix conflicts * updates * updates Co-authored-by: Julien Robert <[email protected]>
cosmos · Nov 16, 2022 · 0b81939 · 0b81939
1 parent 2d515e0
commit 0b81939
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Bug Fixes
 
+* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Group members weight must be positive and a finite number.
 * (x/bank) [#13821](https://github.com/cosmos/cosmos-sdk/pull/13821) Fix bank store migration of coin metadata.
 * (x/group) [#13808](https://github.com/cosmos/cosmos-sdk/pull/13808) Fix propagation of message events to the current context in `EndBlocker`.
 * (x/gov) [#13728](https://github.com/cosmos/cosmos-sdk/pull/13728) Fix propagation of message events to the current context in `EndBlocker`.

diff --git a/x/group/internal/math/dec.go b/x/group/internal/math/dec.go
@@ -1,4 +1,4 @@
-// Package math provides helper functions for doing mathematical calculations and parsing for the ecocredit module.
+// Package math provides helper functions for doing mathematical calculations and parsing for the group module.
 package math
 
 import (
@@ -46,11 +46,22 @@ func (x Dec) IsPositive() bool {
 	return !x.dec.Negative && !x.dec.IsZero()
 }
 
+func (x Dec) IsFinite() bool {
+	return x.dec.Form != apd.Finite
+}
+
+// NewDecFromString returns a new Dec from a string
+// It only support finite numbers, not NaN, +Inf, -Inf
 func NewDecFromString(s string) (Dec, error) {
 	d, _, err := apd.NewFromString(s)
 	if err != nil {
 		return Dec{}, errors.ErrInvalidDecString.Wrap(err.Error())
 	}
+
+	if d.Form != apd.Finite {
+		return Dec{}, errors.ErrInvalidDecString.Wrapf("expected a finite decimal, got %s", s)
+	}
+
 	return Dec{*d}, nil
 }
 

diff --git a/x/group/internal/math/dec_test.go b/x/group/internal/math/dec_test.go
@@ -54,6 +54,14 @@ func TestDec(t *testing.T) {
 	require.NoError(t, err)
 	minusFivePointZero, err := NewDecFromString("-5.0")
 	require.NoError(t, err)
+	_, err = NewDecFromString("inf")
+	require.Error(t, err)
+	_, err = NewDecFromString("Infinite")
+	require.Error(t, err)
+	_, err = NewDecFromString("foo")
+	require.Error(t, err)
+	_, err = NewDecFromString("NaN")
+	require.Error(t, err)
 
 	res, err := two.Add(zero)
 	require.NoError(t, err)

diff --git a/x/group/keeper/keeper.go b/x/group/keeper/keeper.go
@@ -380,6 +380,8 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
 	if err != nil {
 		return nil
 	}
+
+	//nolint:gosec // implicit memory aliasing in for loop
 	for _, proposal := range proposals {
 		policyInfo, err := k.getGroupPolicyInfo(ctx, proposal.GroupPolicyAddress)
 		if err != nil {
@@ -400,8 +402,7 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
 				return err
 			}
 		} else {
-			err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo) //nolint:gosec // implicit memory aliasing in for loop
-			if err != nil {
+			if err := k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo); err != nil {
 				return sdkerrors.Wrap(err, "doTallyAndUpdate")
 			}
 

diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go
@@ -192,6 +192,26 @@ func (s *TestSuite) TestCreateGroup() {
 			},
 			expErr: true,
 		},
+		"invalid member weight - Inf": {
+			req: &group.MsgCreateGroup{
+				Admin: addr1.String(),
+				Members: []group.MemberRequest{{
+					Address: addr3.String(),
+					Weight:  "inf",
+				}},
+			},
+			expErr: true,
+		},
+		"invalid member weight - NaN": {
+			req: &group.MsgCreateGroup{
+				Admin: addr1.String(),
+				Members: []group.MemberRequest{{
+					Address: addr3.String(),
+					Weight:  "NaN",
+				}},
+			},
+			expErr: true,
+		},
 	}
 
 	var seq uint32 = 1

diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go
@@ -687,15 +687,13 @@ func (k Keeper) doTallyAndUpdate(ctx sdk.Context, p *group.Proposal, electorate
 
 	sinceSubmission := ctx.BlockTime().Sub(p.SubmitTime) // duration passed since proposal submission.
 	result, err := policy.Allow(tallyResult, electorate.TotalWeight, sinceSubmission)
-	// If the result was final (i.e. enough votes to pass) or if the voting
-	// period ended, then we consider the proposal as final.
-	isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd)
-
-	switch {
-	case err != nil:
+	if err != nil {
 		return sdkerrors.Wrap(err, "policy allow")
+	}
 
-	case isFinal:
+	// If the result was final (i.e. enough votes to pass) or if the voting
+	// period ended, then we consider the proposal as final.
+	if isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd); isFinal {
 		if err := k.pruneVotes(ctx, p.Id); err != nil {
 			return err
 		}

diff --git a/x/group/module/abci.go b/x/group/module/abci.go
@@ -9,12 +9,8 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) {
 	if err := k.TallyProposalsAtVPEnd(ctx); err != nil {
 		panic(err)
 	}
-	pruneProposals(ctx, k)
-}
 
-func pruneProposals(ctx sdk.Context, k keeper.Keeper) {
-	err := k.PruneProposals(ctx)
-	if err != nil {
+	if err := k.PruneProposals(ctx); err != nil {
 		panic(err)
 	}
 }
diff --git a/x/group/msgs.go b/x/group/msgs.go
@@ -293,10 +293,7 @@ func (m MsgCreateGroupPolicy) GetSignBytes() []byte {
 
 // GetSigners returns the expected signers for a MsgCreateGroupPolicy.
 func (m MsgCreateGroupPolicy) GetSigners() []sdk.AccAddress {
-	admin, err := sdk.AccAddressFromBech32(m.Admin)
-	if err != nil {
-		panic(err)
-	}
+	admin := sdk.MustAccAddressFromBech32(m.Admin)
 	return []sdk.AccAddress{admin}
 }