Skip to content

Commit

Permalink
fix(x/accounts): check for overflows in multisig weights and votes (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
facundomedica authored May 21, 2024
1 parent a2aaed6 commit a2dd2a0
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 2 deletions.
17 changes: 16 additions & 1 deletion x/accounts/defaults/multisig/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ func (a *Account) Init(ctx context.Context, msg *v1.MsgInit) (*v1.MsgInitRespons
return nil, err
}

totalWeight += msg.Members[i].Weight
totalWeight, err = safeAdd(totalWeight, msg.Members[i].Weight)
if err != nil {
return nil, err
}
}

if err := validateConfig(*msg.Config, totalWeight); err != nil {
Expand Down Expand Up @@ -279,6 +282,7 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal
}

totalWeight := yesVotes + noVotes + abstainVotes

var (
rejectErr error
execErr error
Expand Down Expand Up @@ -387,3 +391,14 @@ func (a *Account) RegisterQueryHandlers(builder *accountstd.QueryBuilder) {
accountstd.RegisterQueryHandler(builder, a.QueryProposal)
accountstd.RegisterQueryHandler(builder, a.QueryConfig)
}

func safeAdd(nums ...uint64) (uint64, error) {
var sum uint64
for _, num := range nums {
if sum+num < sum {
return 0, errors.New("overflow")
}
sum += num
}
return sum, nil
}
54 changes: 54 additions & 0 deletions x/accounts/defaults/multisig/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package multisig

import (
"context"
"math"
"testing"
"time"

Expand Down Expand Up @@ -312,6 +313,29 @@ func TestUpdateConfig(t *testing.T) {
},
},
},
{
"change members, invalid weights",
&v1.MsgUpdateConfig{
UpdateMembers: []*v1.Member{
{
Address: "addr1",
Weight: math.MaxUint64,
},
{
Address: "addr2",
Weight: 1,
},
},
Config: &v1.Config{
Threshold: 666,
Quorum: 400,
VotingPeriod: 60,
},
},
"overflow",
nil,
nil,
},
}

for _, tc := range testcases {
Expand Down Expand Up @@ -658,3 +682,33 @@ func TestProposalPassing(t *testing.T) {

require.Equal(t, expectedMembers, cfg.Members)
}

func TestWeightOverflow(t *testing.T) {
ctx, ss := newMockContext(t)
acc := setup(t, ctx, ss, nil)

startAcc := &v1.MsgInit{
Config: &v1.Config{
Threshold: 2640,
Quorum: 2000,
VotingPeriod: 60,
},
Members: []*v1.Member{
{
Address: "addr1",
Weight: math.MaxUint64,
},
},
}

_, err := acc.Init(ctx, startAcc)
require.NoError(t, err)

// add another member with weight 1 to trigger an overflow
startAcc.Members = append(startAcc.Members, &v1.Member{
Address: "addr2",
Weight: 1,
})
_, err = acc.Init(ctx, startAcc)
require.ErrorContains(t, err, "overflow")
}
6 changes: 5 additions & 1 deletion x/accounts/defaults/multisig/update_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ func (a Account) UpdateConfig(ctx context.Context, msg *v1.MsgUpdateConfig) (*v1
// get the weight from the stored members
totalWeight := uint64(0)
err := a.Members.Walk(ctx, nil, func(_ []byte, value uint64) (stop bool, err error) {
totalWeight += value
var adderr error
totalWeight, adderr = safeAdd(totalWeight, value)
if adderr != nil {
return true, adderr
}
return false, nil
})
if err != nil {
Expand Down

0 comments on commit a2dd2a0

Please sign in to comment.