Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

Fix nonce issue for replay attack #692

Merged
merged 4 commits into from
Jan 9, 2021
Merged

Fix nonce issue for replay attack #692

merged 4 commits into from
Jan 9, 2021

Conversation

araskachoi
Copy link
Contributor

@araskachoi araskachoi commented Jan 8, 2021

Closes: #687 #686

Description

Checks the nonce and requires the nonce to be the same as the account sequence.
before, the sequence needed to be greater than that of the nonce.

this fix will also throw an error when using an invalid nonce.
before, eth_sendTransaction would return a tx receipt even if the tx was rejected on the chain.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Can you add a bug fix changelog entry under the Unreleased section?

Comment on lines +1008 to +1012
if args.Nonce != nil {
if nonce != (uint64)(*args.Nonce) {
return nil, fmt.Errorf(fmt.Sprintf("invalid nonce; got %d, expected %d", (uint64)(*args.Nonce), nonce))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, thanks for adding this client side validation!

@araskachoi
Copy link
Contributor Author

ACK. Can you add a bug fix changelog entry under the Unreleased section?

@fedekunze can you check the changelog once more before merging? Thanks!

@fedekunze fedekunze merged commit d7bdbd7 into development Jan 9, 2021
@fedekunze fedekunze deleted the araska/nonce branch January 9, 2021 01:44
@summerpro
Copy link
Contributor

@araskachoi this pr cannot close #687 , but can close #686

@fedekunze
Copy link
Contributor

@araskachoi this pr cannot close #687 , but can close #686

@summerpro is right

@araskachoi
Copy link
Contributor Author

@araskachoi this pr cannot close #687 , but can close #686

agreed,

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants