Merge commit from fork

* Prevent empty groups

* Handle inflight proposals

* Update changelog

* No empty group with simulations

* Remove release version from changelog

x/group/CHANGELOG.md

@@ -25,6 +25,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
 
+### Bug Fixes
+
+* [GHSA-x5vx-95h7-rv4p](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-x5vx-95h7-rv4p) Fix Group module can halt chain when handling a malicious proposal
+
+
 ## [v0.2.0-rc.1](https://github.com/cosmos/cosmos-sdk/releases/tag/x/group/v0.2.0-rc.1) - 2024-12-18
 
 ### Improvements

x/group/keeper/msg_server.go

@@ -201,6 +201,10 @@ func (k Keeper) UpdateGroupMembers(ctx context.Context, msg *group.MsgUpdateGrou
 				return err
 			}
 		}
+		// ensure that group has one or more members
+		if totalWeight.IsZero() {
+			return errorsmod.Wrap(errors.ErrInvalid, "group must not be empty")
+		}
 		// Update group in the groupTable.
 		g.TotalWeight = totalWeight.String()
 		g.Version++
@@ -1132,10 +1136,8 @@ func (k Keeper) validateMembers(members []group.MemberRequest) error {
 		if _, err := math.NewNonNegativeDecFromString(member.Weight); err != nil {
 			return errorsmod.Wrap(err, "weight must be non negative")
 		}
-
 		index[member.Address] = struct{}{}
 	}
-
 	return nil
 }
 

x/group/keeper/msg_server_test.go

@@ -233,14 +233,13 @@ func (s *TestSuite) TestCreateGroup() {
 }
 
 func (s *TestSuite) TestUpdateGroupMembers() {
-	member1 := s.addrsStr[4]
-	member2 := s.addrsStr[5]
-	members := []group.MemberRequest{{
-		Address: member1,
-		Weight:  "1",
-	}}
+	unknownAddr, myAdmin := s.addrsStr[0], s.addrsStr[1]
+	member1, member2, member3 := s.addrsStr[2], s.addrsStr[3], s.addrsStr[4]
+	members := []group.MemberRequest{
+		{Address: member1, Weight: "1"},
+		{Address: member2, Weight: "2"},
+	}
 
-	myAdmin := s.addrsStr[3]
 	groupRes, err := s.groupKeeper.CreateGroup(s.ctx, &group.MsgCreateGroup{
 		Admin:   myAdmin,
 		Members: members,
@@ -260,7 +259,7 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 				GroupId: 0,
 				Admin:   myAdmin,
 				MemberUpdates: []group.MemberRequest{{
-					Address: member2,
+					Address: member3,
 					Weight:  "2",
 				}},
 			},
@@ -293,7 +292,7 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 				Admin:   myAdmin,
 				MemberUpdates: []group.MemberRequest{
 					{
-						Address:  member2,
+						Address:  member3,
 						Weight:   "2",
 						Metadata: strings.Repeat("a", 10240),
 					},
@@ -304,12 +303,61 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 		},
 		"add new member": {
 			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: member2,
-					Weight:  "2",
-				}},
+				GroupId:       groupID,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: member3, Weight: "3"}},
+			},
+			expGroup: &group.GroupInfo{
+				Id:          groupID,
+				Admin:       myAdmin,
+				TotalWeight: "6",
+				Version:     2,
+				CreatedAt:   s.blockTime,
+			},
+			expMembers: []*group.GroupMember{
+				{
+					Member:  &group.Member{Address: member3, Weight: "3", AddedAt: s.sdkCtx.HeaderInfo().Time},
+					GroupId: groupID,
+				},
+				{
+					Member:  &group.Member{Address: member1, Weight: "1", AddedAt: s.blockTime},
+					GroupId: groupID,
+				},
+				{
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+					GroupId: groupID,
+				},
+			},
+		},
+		"update member": {
+			req: &group.MsgUpdateGroupMembers{
+				GroupId:       groupID,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}},
+			},
+			expGroup: &group.GroupInfo{
+				Id:          groupID,
+				Admin:       myAdmin,
+				TotalWeight: "4",
+				Version:     2,
+				CreatedAt:   s.blockTime,
+			},
+			expMembers: []*group.GroupMember{
+				{
+					Member:  &group.Member{Address: member1, Weight: "2", AddedAt: s.blockTime},
+					GroupId: groupID,
+				},
+				{
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+					GroupId: groupID,
+				},
+			},
+		},
+		"update member with same data": {
+			req: &group.MsgUpdateGroupMembers{
+				GroupId:       groupID,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "1"}},
 			},
 			expGroup: &group.GroupInfo{
 				Id:          groupID,
@@ -320,31 +368,46 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 			},
 			expMembers: []*group.GroupMember{
 				{
-					Member: &group.Member{
-						Address: member2,
-						Weight:  "2",
-						AddedAt: s.sdkCtx.HeaderInfo().Time,
-					},
 					GroupId: groupID,
+					Member:  &group.Member{Address: member1, Weight: "1", AddedAt: s.blockTime},
 				},
 				{
-					Member: &group.Member{
-						Address: member1,
-						Weight:  "1",
-						AddedAt: s.blockTime,
-					},
 					GroupId: groupID,
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
 				},
 			},
 		},
-		"update member": {
+		"replace member": {
 			req: &group.MsgUpdateGroupMembers{
 				GroupId: groupID,
 				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: member1,
-					Weight:  "2",
+				MemberUpdates: []group.MemberRequest{
+					{Address: member1, Weight: "0"},
+					{Address: member3, Weight: "1"},
+				},
+			},
+			expGroup: &group.GroupInfo{
+				Id:          groupID,
+				Admin:       myAdmin,
+				TotalWeight: "3",
+				Version:     2,
+				CreatedAt:   s.blockTime,
+			},
+			expMembers: []*group.GroupMember{
+				{
+					Member:  &group.Member{Address: member3, Weight: "1", AddedAt: s.sdkCtx.HeaderInfo().Time},
+					GroupId: groupID,
+				},
+				{
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+					GroupId: groupID,
 				}},
+		},
+		"remove existing member": {
+			req: &group.MsgUpdateGroupMembers{
+				GroupId:       groupID,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "0"}},
 			},
 			expGroup: &group.GroupInfo{
 				Id:          groupID,
@@ -355,99 +418,15 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 			},
 			expMembers: []*group.GroupMember{
 				{
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
 					GroupId: groupID,
-					Member: &group.Member{
-						Address: member1,
-						Weight:  "2",
-						AddedAt: s.blockTime,
-					},
-				},
-			},
-		},
-		"update member with same data": {
-			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: member1,
-					Weight:  "1",
 				}},
-			},
-			expGroup: &group.GroupInfo{
-				Id:          groupID,
-				Admin:       myAdmin,
-				TotalWeight: "1",
-				Version:     2,
-				CreatedAt:   s.blockTime,
-			},
-			expMembers: []*group.GroupMember{
-				{
-					GroupId: groupID,
-					Member: &group.Member{
-						Address: member1,
-						Weight:  "1",
-						AddedAt: s.blockTime,
-					},
-				},
-			},
-		},
-		"replace member": {
-			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{
-					{
-						Address: member1,
-						Weight:  "0",
-					},
-					{
-						Address: member2,
-						Weight:  "1",
-					},
-				},
-			},
-			expGroup: &group.GroupInfo{
-				Id:          groupID,
-				Admin:       myAdmin,
-				TotalWeight: "1",
-				Version:     2,
-				CreatedAt:   s.blockTime,
-			},
-			expMembers: []*group.GroupMember{{
-				GroupId: groupID,
-				Member: &group.Member{
-					Address: member2,
-					Weight:  "1",
-					AddedAt: s.sdkCtx.HeaderInfo().Time,
-				},
-			}},
-		},
-		"remove existing member": {
-			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: member1,
-					Weight:  "0",
-				}},
-			},
-			expGroup: &group.GroupInfo{
-				Id:          groupID,
-				Admin:       myAdmin,
-				TotalWeight: "0",
-				Version:     2,
-				CreatedAt:   s.blockTime,
-			},
-			expMembers: []*group.GroupMember{},
 		},
 		"remove unknown member": {
 			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: s.addrsStr[3],
-					Weight:  "0",
-				}},
+				GroupId:       groupID,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: unknownAddr, Weight: "0"}},
 			},
 			expErr: true,
 			expGroup: &group.GroupInfo{
@@ -457,66 +436,73 @@ func (s *TestSuite) TestUpdateGroupMembers() {
 				Version:     1,
 				CreatedAt:   s.blockTime,
 			},
-			expMembers: []*group.GroupMember{{
-				GroupId: groupID,
-				Member: &group.Member{
-					Address: member1,
-					Weight:  "1",
-				},
-			}},
+			expMembers: []*group.GroupMember{
+				{
+					GroupId: groupID,
+					Member:  &group.Member{Address: member1, Weight: "1"},
+				}, {
+					Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+					GroupId: groupID,
+				}},
 		},
 		"with wrong admin": {
 			req: &group.MsgUpdateGroupMembers{
-				GroupId: groupID,
-				Admin:   s.addrsStr[2],
-				MemberUpdates: []group.MemberRequest{{
-					Address: member1,
-					Weight:  "2",
-				}},
+				GroupId:       groupID,
+				Admin:         unknownAddr,
+				MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}},
 			},
 			expErr:    true,
 			expErrMsg: "not group admin",
 			expGroup: &group.GroupInfo{
 				Id:          groupID,
 				Admin:       myAdmin,
-				TotalWeight: "1",
+				TotalWeight: "3",
 				Version:     1,
 				CreatedAt:   s.blockTime,
 			},
 			expMembers: []*group.GroupMember{{
 				GroupId: groupID,
-				Member: &group.Member{
-					Address: member1,
-					Weight:  "1",
-				},
+				Member:  &group.Member{Address: member1, Weight: "1"},
+			}, {
+				Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+				GroupId: groupID,
 			}},
 		},
 		"with unknown groupID": {
 			req: &group.MsgUpdateGroupMembers{
-				GroupId: 999,
-				Admin:   myAdmin,
-				MemberUpdates: []group.MemberRequest{{
-					Address: member1,
-					Weight:  "2",
-				}},
+				GroupId:       999,
+				Admin:         myAdmin,
+				MemberUpdates: []group.MemberRequest{{Address: member1, Weight: "2"}},
 			},
 			expErr:    true,
 			expErrMsg: "not found",
 			expGroup: &group.GroupInfo{
 				Id:          groupID,
 				Admin:       myAdmin,
-				TotalWeight: "1",
+				TotalWeight: "3",
 				Version:     1,
 				CreatedAt:   s.blockTime,
 			},
 			expMembers: []*group.GroupMember{{
 				GroupId: groupID,
-				Member: &group.Member{
-					Address: member1,
-					Weight:  "1",
-				},
+				Member:  &group.Member{Address: member1, Weight: "1"},
+			}, {
+				Member:  &group.Member{Address: member2, Weight: "2", AddedAt: s.blockTime},
+				GroupId: groupID,
 			}},
 		},
+		"remove all members": {
+			req: &group.MsgUpdateGroupMembers{
+				GroupId: groupID,
+				Admin:   myAdmin,
+				MemberUpdates: []group.MemberRequest{
+					{Address: member1, Weight: "0"},
+					{Address: member2, Weight: "0"},
+				},
+			},
+			expErr:    true,
+			expErrMsg: "group must not be empty",
+		},
 	}
 	for msg, spec := range specs {
 		s.Run(msg, func() {

x/group/simulation/msg_factory.go

@@ -329,7 +329,7 @@ func MsgUpdateGroupMembersFactory(k keeper.Keeper, s *SharedState) simsx.SimMsgF
 		}
 		oldMemberAddrs := simsx.Collect(res.Members, func(a *group.GroupMember) string { return a.Member.Address })
 		members := genGroupMembersX(testData, reporter, simsx.ExcludeAddresses(oldMemberAddrs...))
-		if len(res.Members) != 0 {
+		if len(res.Members) != 1 {
 			// set existing random group member weight to zero to remove from the group
 			obsoleteMember := simsx.OneOf(testData.Rand(), res.Members)
 			obsoleteMember.Member.Weight = "0"

x/group/types.go

@@ -211,7 +211,9 @@ func (p PercentageDecisionPolicy) Allow(tally TallyResult, totalPower string) (D
 	if err != nil {
 		return DecisionPolicyResult{}, errorsmod.Wrap(err, "total power")
 	}
-
+	if totalPowerDec.IsZero() {
+		return DecisionPolicyResult{Allow: false, Final: true}, nil
+	}
 	yesPercentage, err := yesCount.Quo(totalPowerDec)
 	if err != nil {
 		return DecisionPolicyResult{}, err

x/group/types_test.go

@@ -239,6 +239,28 @@ func TestPercentageDecisionPolicyAllow(t *testing.T) {
 			},
 			false,
 		},
+		{
+			"empty total power",
+			&group.PercentageDecisionPolicy{
+				Percentage: "0.5",
+				Windows: &group.DecisionPolicyWindows{
+					VotingPeriod: time.Second * 100,
+				},
+			},
+			&group.TallyResult{
+				YesCount:        "1",
+				NoCount:         "0",
+				AbstainCount:    "0",
+				NoWithVetoCount: "0",
+			},
+			"0",
+			time.Second * 50,
+			group.DecisionPolicyResult{
+				Allow: false,
+				Final: true,
+			},
+			false,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {