-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(x/accounts): check for overflows in multisig weights and votes #20384
Changes from 3 commits
ddf0294
96289d8
00f8d18
f800a92
3350270
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -266,19 +269,32 @@ func (a Account) ExecuteProposal(ctx context.Context, msg *v1.MsgExecuteProposal | |
|
||
switch v1.VoteOption(vote) { | ||
case v1.VoteOption_VOTE_OPTION_YES: | ||
yesVotes += weight | ||
yesVotes, err = safeAdd(yesVotes, weight) | ||
if err != nil { | ||
return true, err | ||
} | ||
case v1.VoteOption_VOTE_OPTION_NO: | ||
noVotes += weight | ||
noVotes, err = safeAdd(noVotes, weight) | ||
if err != nil { | ||
return true, err | ||
} | ||
case v1.VoteOption_VOTE_OPTION_ABSTAIN: | ||
abstainVotes += weight | ||
abstainVotes, err = safeAdd(abstainVotes, weight) | ||
if err != nil { | ||
return true, err | ||
} | ||
} | ||
return false, nil | ||
}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
totalWeight := yesVotes + noVotes + abstainVotes | ||
totalWeight, err := safeAdd(yesVotes, noVotes, abstainVotes) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
var ( | ||
rejectErr error | ||
execErr error | ||
|
@@ -387,3 +403,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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we set it to max instead? that way tallying will see the max number not 0. 0 could signify 0 votes and passing here now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an error is returned we don't really care about the result, as we'll return it without continuing execution There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Does it make sense to check if all weight equate to less than maxuint64 when adding memebers or creating the multisig There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm doing that check here: https://github.com/cosmos/cosmos-sdk/pull/20384/files#diff-fdd6d41c43d4db2fca28c5b410bd647692b4f7fb78997c62abf0835c8052d9c6R94 but now I that I've double-check I'm not doing that on update config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: why can't we simply ensure that total_weight <= math.Uint64Max at member lvl There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @testinginprod can you explain a bit more? You mean that we should only check for overflow when creating/updating members? That sounds good, because then in the tally we'd never go over that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thinking. Checks on init/ update would be enough but tally seems not to be on the criticial path for performance so I would not mind safety over performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, I've removed the check on tally and added the test on update |
||
} | ||
sum += num | ||
} | ||
return sum, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit, i would pefer
sum > math.MaxUint64 - num
for easier readability