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

CheckTx doesn't verify message routing #4639

Closed
4 tasks
alexanderbez opened this issue Jun 28, 2019 · 2 comments · Fixed by #4810
Closed
4 tasks

CheckTx doesn't verify message routing #4639

alexanderbez opened this issue Jun 28, 2019 · 2 comments · Fixed by #4810
Assignees
Labels
good first issue T:Bug Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Milestone

Comments

@alexanderbez
Copy link
Contributor

Summary of Bug

// CheckTx implements the ABCI interface. It runs the "basic checks" to see
// whether or not a transaction can possibly be executed, first decoding, then
// the ante handler (which checks signatures/fees/ValidateBasic), then finally
// the route match to see whether a handler exists.
//
// NOTE:CheckTx does not run the actual Msg handler function(s).

Currently CheckTx does all of what is stated in its godoc except for verifying a router exists for each message! This isn't a big deal or exploitable, but should be fixed nonetheless. I believe we can do this by simply remove the following from BaseApp#runTx:

	if mode == runTxModeCheck {
		return result
	}

This is because BaseApp#runMsgs already has this check and doesn't execute the matched message handler during CheckTx.

Caught by @gamarin2

Version

b2f8c58


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez alexanderbez added T:Bug good first issue Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. labels Jun 28, 2019
@rdrahul
Copy link
Contributor

rdrahul commented Jul 20, 2019

@alexanderbez I want to take this up.

@alexanderbez
Copy link
Contributor Author

Yes please @rdrahul, that would be helpful!

@alexanderbez alexanderbez added this to the v0.37.0 milestone Jul 22, 2019
@fedekunze fedekunze self-assigned this Jul 30, 2019
@fedekunze fedekunze mentioned this issue Jul 30, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue T:Bug Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants