diff --git a/x/group/CHANGELOG.md b/x/group/CHANGELOG.md index f2098fdc94..c5b00bc67c 100644 --- a/x/group/CHANGELOG.md +++ b/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 diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index ee431fe3e5..798495a3f1 100644 --- a/x/group/keeper/msg_server.go +++ b/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 } diff --git a/x/group/keeper/msg_server_test.go b/x/group/keeper/msg_server_test.go index 3ed95edec2..961908136d 100644 --- a/x/group/keeper/msg_server_test.go +++ b/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() { diff --git a/x/group/simulation/msg_factory.go b/x/group/simulation/msg_factory.go index 30b55fc498..c8b939022f 100644 --- a/x/group/simulation/msg_factory.go +++ b/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" diff --git a/x/group/types.go b/x/group/types.go index d91d4fd897..fca8016a0d 100644 --- a/x/group/types.go +++ b/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 diff --git a/x/group/types_test.go b/x/group/types_test.go index 2b02c71f56..4489bb0bb6 100644 --- a/x/group/types_test.go +++ b/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) {