Skip to content
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

x/gov/simulation: NewDecodeStore unmarshals wrong proposal due to typo in value source #8570

Closed
4 tasks done
odeke-em opened this issue Feb 12, 2021 · 0 comments · Fixed by #8603
Closed
4 tasks done
Assignees

Comments

@odeke-em
Copy link
Collaborator

Summary of Bug

Noticed while auditing, see this code

var proposalA types.Proposal
err := cdc.UnmarshalBinaryBare(kvA.Value, &proposalA)
if err != nil {
panic(err)
}
var proposalB types.Proposal
err = cdc.UnmarshalBinaryBare(kvA.Value, &proposalB)

notice that both ProposalA and ProposalB unmarshal from kvA.Value?
shouldn’t that use kvB.Value? Looks like a typo from PR #6147 from May 2020.
8B96D98B-AD07-45B2-A985-1210C0BB473F

We shall also need to improve the tests to catch such bugs, given that that code has been in for 9 months without any tests catching it, shows we need to tighten the test.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
odeke-em added a commit that referenced this issue Feb 17, 2021
… tests

The code in NewDecodeStore decoded the wrong proposal due
to a typographical error, but the tests used the exact same
value for the key value pairs hence the typo could never be caught.
I noticed it during an audit of the code, and I've fixed the
tests to pass in varying values for the various key value pairs.

Fixes #8570
odeke-em added a commit that referenced this issue Feb 17, 2021
… tests (#8603)

The code in NewDecodeStore decoded the wrong proposal due
to a typographical error, but the tests used the exact same
value for the key value pairs hence the typo could never be caught.
I noticed it during an audit of the code, and I've fixed the
tests to pass in varying values for the various key value pairs.

Fixes #8570

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants