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

MsgTransfer is not validated before calling Transfer #1237

Closed
mpoke opened this issue Aug 25, 2023 · 1 comment · Fixed by #1244
Closed

MsgTransfer is not validated before calling Transfer #1237

mpoke opened this issue Aug 25, 2023 · 1 comment · Fixed by #1244
Assignees
Labels
good first issue Good for newcomers type: bug Issues that need priority attention -- something isn't working

Comments

@mpoke
Copy link
Contributor

mpoke commented Aug 25, 2023

Problem

Before calling the Transfer method of ICS-20, the message is not being validated. Normally, Transfer is called as a result of submitting a message MsgTransfer to the chain, which calls ValidateBasic. However, the validation is skipped in case of calling the method directly.

Closing criteria

Validate MsgTransfer before calling Transfer.

@mpoke mpoke added type: bug Issues that need priority attention -- something isn't working good first issue Good for newcomers labels Aug 25, 2023
@mpoke mpoke added this to Cosmos Hub Aug 25, 2023
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Cosmos Hub Aug 25, 2023
@mpoke mpoke moved this from 🩹 Triage to 📥 Todo in Cosmos Hub Aug 25, 2023
@mpoke
Copy link
Contributor Author

mpoke commented Aug 25, 2023

Alternative solution (suggested by @crodriguezvega):

Also, instead of calling directly Transfer I think the recommended approach is to route the message through the service router. For example, this is what we do in ICA host when a message is received. The keeper of the host is created with a MessageRouter (see also wiring in app.go). So maybe that could be a way to do it as well in interchain security, and then maybe you can remove the dependency to the transfer keeper. I believe in that case ValidateBasic will trigger for the message.

@mpoke mpoke moved this from 📥 Todo to 🏗 In progress in Cosmos Hub Aug 25, 2023
@mpoke mpoke self-assigned this Aug 30, 2023
@mpoke mpoke moved this from 🏗 F3: InProgress to 👀 F3: InReview in Cosmos Hub Aug 30, 2023
@github-project-automation github-project-automation bot moved this from 👀 F3: InReview to 👍 F4: Assessment in Cosmos Hub Sep 5, 2023
@mpoke mpoke moved this from 👍 F4: Assessment to ✅ Done in Cosmos Hub Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: bug Issues that need priority attention -- something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

1 participant