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

types: Coin.Validate should also check that .Amount is not nil before trying to deference it; not only check that .Denom is valid #15690

Closed
odeke-em opened this issue Apr 4, 2023 · 0 comments · Fixed by #15691
Assignees
Labels

Comments

@odeke-em
Copy link
Collaborator

odeke-em commented Apr 4, 2023

Summary of Bug

Reported offline by @zanicar in a scenario in which they sent a JSON payload with an amount that can't JSON unmarshal
{"Denom": "stake", "Amount": 10000}, that value when JSON unmarshalled silently throws Amount away but saves .Denom and the code in .Validate() solely checks that .Denom is valid per

cosmos-sdk/types/coin.go

Lines 42 to 52 in 445dc8a

func (coin Coin) Validate() error {
if err := ValidateDenom(coin.Denom); err != nil {
return err
}
if coin.Amount.IsNegative() {
return fmt.Errorf("negative coin amount: %v", coin.Amount)
}
return nil
}

Version

445dc8a

Remedy

We should also check that .Amount is not nil before trying to dereference it

@odeke-em odeke-em added the T:Bug label Apr 4, 2023
@odeke-em odeke-em self-assigned this Apr 4, 2023
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 4, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 5, 2023
This change fixes a scenario in which Coin.Validate() would
panic when given a nil Amount. While here, added a fuzz test
along with unit/regression tests.
Found and reported by the Quicksilver team.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 5, 2023
This change fixes a scenario in which Coin.Validate() would panic when given a nil Amount.
While here, added a fuzz test along with unit/regression tests.

Fixes #15690
odeke-em added a commit that referenced this issue Apr 8, 2023
Adds a fuzz test for QueryBalance in which we shall
feed it inputs and watch for any panics that'll then
get reported and could potentially take down nodes.

This test independently had found the bug reported in #15690
odeke-em added a commit that referenced this issue Apr 10, 2023
Adds a fuzz test for QueryBalance in which we shall
feed it inputs and watch for any panics that'll then
get reported and could potentially take down nodes.

This test independently had found the bug reported in #15690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant