Add address length validation to MsgSend and MsgMultiSend

[webmaster128]

Description

Since the protobuf transition, user input for AccAddress is casted instead of decoded from bech32. So a validation step that existed before does not exist anymore. As a consequence, a user can put any binary data into the system which can have unintended effects. From the client's perspective this leads to error messages at some point in signature verification instead of message validation.

I see basically two path to this:

1. assume any user input AccAddress is arbitrary binary data that needs validation
2. ensure AccAddress is not created by casting but only by validated construction, such that any AccAddress is always correct

Given that AccAddress is an alias, 2. would probably not be feasible (correct me if I'm wrong). This PR implements 1. for x/bank messages.

closes: #7015

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• 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
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #7053 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7053   +/-   ##
=======================================
  Coverage   54.68%   54.68%           
=======================================
  Files         540      540           
  Lines       36856    36856           
=======================================
  Hits        20153    20153           
  Misses      15060    15060           
  Partials     1643     1643           

[ethanfrey]

Nice fix and welcome to go 👍

Amazing how many test cases were misusing this as well.

[tac0turtle]

WOW!! Amazing to see happy path testing become stricter. I wonder if this is present in other modules as well??!!?!

Note: In the current usage of pubkey Address() is expecting a pubkey length otherwise it will panic. I have a feeling .Address() is called on pubkeys without checking to see if they are valid which will cause a node panic. I would not be surprised if someone could induce a node panic via a incorrect pubkey size!!!!

[jgimeno]

Good one!!! Thanks

[webmaster128]

you're welcome, Ethan, Marko, Jonathan

I wonder if this is present in other modules as well??!!?!

I would be surprised if not. MsgSend was the first place where the client hit the issue and so I looked into bank. If someone want to check the other modules and to the same thing there, I'd appreciate if the SDK team manages that in a separate ticket.

[anilcse]

🎉

[alexanderbez]

Thanks, @webmaster128. Please update/rebase this PR and it should automatically merge.

[webmaster128]

@alexanderbez rebased but automerge does not work

[alessio]

@webmaster128 it's out of sync with master again. Can you merge master again please?

[webmaster128]

@alessio thank you, I updated it again. But it is already out of sync again. Can you please take care of it one way or another? I enabled Allow edits and access to secrets by maintainers now. Hope that helps.