fix(group): add group members weight checks

CHANGELOG.md

@@ -164,7 +164,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/gov) [#13160](https://github.com/cosmos/cosmos-sdk/pull/13160) Remove custom marshaling of proposl and voteoption.
 * (types) [#13430](https://github.com/cosmos/cosmos-sdk/pull/13430) Remove unused code `ResponseCheckTx` and `ResponseDeliverTx`
 * (store) [#13529](https://github.com/cosmos/cosmos-sdk/pull/13529) Add method `LatestVersion` to `MultiStore` interface, add method `SetQueryMultiStore` to baesapp to support alternative `MultiStore` implementation for query service.
-* (pruning) [#13609]](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning package to be under store package
+* (pruning) [#13609](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning package to be under store package
 * [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) Most methods on `types/module.AppModule` have been moved to 
 extension interfaces. `module.Manager.Modules` is now of type `map[string]interface{}` to support in parallel the new 
 `cosmossdk.io/core/appmodule.AppModule` API.
@@ -176,9 +176,11 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf
 
 ### Bug Fixes
 
+* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Fix issue when tallying group proposal votes.
+* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Group members weight must be positive and a finite number.
 * (x/auth) [#13838](https://github.com/cosmos/cosmos-sdk/pull/13838) Fix calling `String()` and `MarshalYAML` panics when pubkey is set on a `BaseAccount`. 
 * (rosetta) [#13583](https://github.com/cosmos/cosmos-sdk/pull/13583) Misc fixes for cosmos-rosetta.
-* (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740)  Fix evidence query API to decode the hash properly.
+* (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740) Fix evidence query API to decode the hash properly.
 * (bank) [#13691](https://github.com/cosmos/cosmos-sdk/issues/13691) Fix unhandled error for vesting account transfers, when total vesting amount exceeds total balance.
 * [#13553](https://github.com/cosmos/cosmos-sdk/pull/13553) Ensure all parameter validation for decimal types handles nil decimal values.
 * [#13145](https://github.com/cosmos/cosmos-sdk/pull/13145) Fix panic when calling `String()` to a Record struct type.

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
 }
 

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)

x/group/keeper/keeper.go

@@ -382,7 +382,6 @@ func (k Keeper) PruneProposals(ctx sdk.Context) error {
 // TallyProposalsAtVPEnd iterates over all proposals whose voting period
 // has ended, tallies their votes, prunes them, and updates the proposal's
 // `FinalTallyResult` field.
-
 func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
 	proposals, err := k.proposalsByVPEnd(ctx, ctx.BlockTime())
 	if err != nil {
@@ -400,6 +399,16 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
 			return sdkerrors.Wrap(err, "group")
 		}
 
+		// skip the invalid decision policies that could have been created before
+		// https://github.com/cosmos/cosmos-sdk/pull/13869
+		decisionPolicy, err := policyInfo.GetDecisionPolicy()
+		if err != nil {
+			return sdkerrors.Wrap(err, "decision policy")
+		}
+		if err := decisionPolicy.Validate(electorate, k.config); err != nil {
+			continue
+		}
+
 		proposalID := proposal.Id
 		if proposal.Status == group.PROPOSAL_STATUS_ABORTED || proposal.Status == group.PROPOSAL_STATUS_WITHDRAWN {
 			if err := k.pruneProposal(ctx, proposalID); err != nil {
@@ -409,8 +418,7 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
 				return err
 			}
 		} else {
-			err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo)
-			if err != nil {
+			if err := k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo); err != nil {
 				return sdkerrors.Wrap(err, "doTallyAndUpdate")
 			}
 

x/group/keeper/keeper_test.go

@@ -253,6 +253,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
@@ -899,6 +919,19 @@ func (s *TestSuite) TestCreateGroupWithPolicy() {
 			),
 			expErr: false,
 		},
+		"minExecutionPeriod = votingPeriod + MaxExecutionPeriod": {
+			req: &group.MsgCreateGroupWithPolicy{
+				Admin:              addr1.String(),
+				Members:            members,
+				GroupPolicyAsAdmin: false,
+			},
+			policy: group.NewThresholdDecisionPolicy(
+				"1",
+				time.Second,
+				time.Second+group.DefaultConfig().MaxExecutionPeriod,
+			),
+			expErr: true,
+		},
 	}
 
 	for msg, spec := range specs {
@@ -1862,6 +1895,67 @@ func (s *TestSuite) TestWithdrawProposal() {
 	}
 }
 
+func (s *TestSuite) TestTallyProposalsAtVPEnd() {
+	// panics before https://github.com/cosmos/cosmos-sdk/pull/13869 fixes
+	// we need to skip the test because extra validation was added making the invalid threshold policy
+	// impossible to create. However, we still want to make sure that the panic will not be triggered for the already existing invalid policies.
+	s.T().Skip()
+
+	addrs := s.addrs
+	addr1 := addrs[0]
+	addr2 := addrs[1]
+	votingPeriod := time.Duration(4 * time.Minute)
+	minExecutionPeriod := votingPeriod + group.DefaultConfig().MaxExecutionPeriod
+
+	groupMsg := &group.MsgCreateGroupWithPolicy{
+		Admin: addr1.String(),
+		Members: []group.MemberRequest{
+			{Address: addr1.String(), Weight: "1"},
+			{Address: addr2.String(), Weight: "1"},
+		},
+	}
+	policy := group.NewThresholdDecisionPolicy(
+		"1",
+		votingPeriod,
+		minExecutionPeriod,
+	)
+	s.Require().NoError(groupMsg.SetDecisionPolicy(policy))
+
+	s.setNextAccount()
+	groupRes, err := s.groupKeeper.CreateGroupWithPolicy(s.ctx, groupMsg)
+	s.Require().NoError(err)
+	accountAddr := groupRes.GetGroupPolicyAddress()
+	groupPolicy, err := sdk.AccAddressFromBech32(accountAddr)
+	s.Require().NoError(err)
+	s.Require().NotNil(groupPolicy)
+
+	proposalRes, err := s.groupKeeper.SubmitProposal(s.ctx, &group.MsgSubmitProposal{
+		GroupPolicyAddress: accountAddr,
+		Proposers:          []string{addr1.String()},
+		Messages:           nil,
+	})
+	s.Require().NoError(err)
+
+	_, err = s.groupKeeper.Vote(s.ctx, &group.MsgVote{
+		ProposalId: proposalRes.ProposalId,
+		Voter:      addr1.String(),
+		Option:     group.VOTE_OPTION_YES,
+	})
+	s.Require().NoError(err)
+
+	// move forward in time
+	ctx := s.sdkCtx.WithBlockTime(s.sdkCtx.BlockTime().Add(votingPeriod + 1))
+
+	result, err := s.groupKeeper.TallyResult(ctx, &group.QueryTallyResultRequest{
+		ProposalId: proposalRes.ProposalId,
+	})
+	s.Require().Equal("1", result.Tally.YesCount)
+	s.Require().NoError(err)
+
+	s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctx))
+	s.NotPanics(func() { module.EndBlocker(ctx, s.groupKeeper) })
+}
+
 func (s *TestSuite) TestVote() {
 	addrs := s.addrs
 	addr1 := addrs[0]

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
 		}

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)
 	}
 }

x/group/msgs.go

@@ -294,10 +294,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}
 }
 

x/group/types.go

@@ -136,8 +136,8 @@ func (p *ThresholdDecisionPolicy) Validate(g GroupInfo, config Config) error {
 		return sdkerrors.Wrap(err, "group total weight")
 	}
 
-	if p.Windows.MinExecutionPeriod > p.Windows.VotingPeriod+config.MaxExecutionPeriod {
-		return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be smaller than voting_period + max_execution_period")
+	if p.Windows.MinExecutionPeriod >= p.Windows.VotingPeriod+config.MaxExecutionPeriod {
+		return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be strictly smaller than voting_period + max_execution_period")
 	}
 	return nil
 }
@@ -171,8 +171,8 @@ func (p PercentageDecisionPolicy) ValidateBasic() error {
 }
 
 func (p *PercentageDecisionPolicy) Validate(g GroupInfo, config Config) error {
-	if p.Windows.MinExecutionPeriod > p.Windows.VotingPeriod+config.MaxExecutionPeriod {
-		return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be smaller than voting_period + max_execution_period")
+	if p.Windows.MinExecutionPeriod >= p.Windows.VotingPeriod+config.MaxExecutionPeriod {
+		return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be strictly smaller than voting_period + max_execution_period")
 	}
 	return nil
 }