feat(bank): Allow injectable restrictions on bank transfers

[SpicyLemon]

Description

Closes: #14124

This PR provides a mechanism for injecting functions that can restrict bank transfers.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[iramiller]

Very interesting capability ... these hooks can support everything from the send_disabled filters on tokens to rate limits on sends to/from accounts ... or even totals on token sent within a time period.

It seems important that the send restrictions are implemented at the lowest level of balance update apis/logic within the bank module. There have been several security vulnerabilities reported due to the send_disabled logic being implemented at a higher level in the msg_server and relying on all other integrations with the bank module send keeper (such as IBC, wasmd) to also copy this restriction logic when making use of the lower level send APIs on the bank keeper. Moving to this approach would close a significant security risk avenue.

[sonarcloud]

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[hxrts]

I'm looking into a pretty different use case and found this issue. One question, how are you thinking about restriction of staking reward withdrawals? Because as of now I could delegate withdrawal capability using authz to a secondary account and then access staking rewards and transfer those freely.

Marko was saying there might be some value in generalizing this approach in order to place restrictions on other modules.

[SpicyLemon]

I'm looking into a pretty different use case and found this issue. One question, how are you thinking about restriction of staking reward withdrawals? Because as of now I could delegate withdrawal capability using authz to a secondary account and then access staking rewards and transfer those freely.

Marko was saying there might be some value in generalizing this approach in order to place restrictions on other modules.

These new restrictions are applied at the lowest public level of the keeper functions. i.e. SendCoins and InputOutputCoins. In order for funds to move, one of those two functions must be used. At that level, the addresses involved are the source and destination accounts (regardless of any sort of feegrant or authz grants).

Restrictions in a module are going to be specific to the business of that module; their location and purpose are going to be different. As such, the data needed to apply a restriction is also going to be dependent on the module and what's being restricted. The location and purpose of each restriction is going to be different, and I'm not sure a generalized solution is desirable. Some general guidelines might be helpful, but that's as far as I'd go.

In this PR, I designed the restrictions as something provided to the keeper after the keeper has been created. The bank keeper is needed by almost all other modules, so requiring the a restriction-providing keeper to be defined before it is painful and probably not even possible in most cases. If this were a less-core module, I probably would have opted for providing them as arguments to the keeper constructor. I also designed them to be additive, so that multiple restrictions can be provided from sources that have no knowledge of each other.

I have three on-deck uses of these send restrictions. Two of them are restrictions on the receiver, but also need to know the sender; the third only needs to know the sender. Neither of the first two would try to prevent a send, only divert the send, which is why I made the SendRestrictionFn return an address. The third one would prevent a send altogether, which can be done by returning an error. They could be split up, e.g. into a ReceiveRestrictionFn (for the first two) and SendRestrictionFn for the third. However, I chose to combine them because it's simpler both from the standpoint of a user of the SDK (one trying to provide a restriction) and from inside the x/bank module too. Plus all inputs and outputs needed for the third use-case are included in the first two (it just doesn't care about the receiver).

The bank module already has the ability to inject a restriction on minting. It's use and the way it's provided to the module are different from what I've added though. The WithMintCoinsRestriction function copies the existing keeper struct and returns a new copy of it with the provided restriction; that mint-coins restriction doesn't apply to other instances of the keeper. In contrast, these new send restrictions should be in effect for all uses of SendCoins and InputOutputCoins. I bring this up to point out the different needs and uses; they shouldn't be treated the same way.

All that being said, I'm hoping that the SendRestrictionFn and how they're injected is enough to cover the needs of anyone wanting to restrict the sending of funds. It's generic in that approach, but the solution I have here is custom tailored to the module and action being restricted.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[odeke-em]

LGTM, thank you @SpicyLemon!

[robert-zaremba]

Is there any chance to include this in v0.50?

[testinginprod]

Are send restrictions meant to be updated as the chain is running? Because I think having them initted once reduces API surface.

[SpicyLemon]

They're not meant to be updated as the chain is running. The reasons they're appended/prepended like this are:

1. The bank keeper can be created as one of the first (crucial since several other modules depend on it). It's expected that modules that need to inject send restrictions will often also need other bank keeper functionality.
2. It allows other modules to inject their own restrictions (e.g. in their own keeper constructors) without needing knowledge of any other modules that have injected/will inject restrictions.
3. Modules that need to inject a restriction won't work without that restriction being applied. It should be up to that module to make sure the restriction is injected. It shouldn't be relegated to something a blockchain author needs to know to wire up outside the module. If the restriction can be initialized only once, you'd have to create all the keepers, know which ones have restriction to apply, combine them, and give them and init them.

For example, we (Provenance) have three modules with send restrictions: sanction, quarantine, and marker. None of those modules work correctly without those send restrictions being applied. Because they're injected into the bank module during their own keeper creation, we can be certain that they're being applied.

Also, because they're being injected during their own keeper creation, they can each have unit tests in their own module that make sure they're being applied. If they had to be initialized once all together, those tests would have to live live at an app level.

Basically, this design fixes some pain points I encountered by having multiple modules with gov hooks.

[elias-orijtech]

This is late, and may have been discussed already, but I wonder what the need is for having 2 new methods, compared to just, say, AppendSendRestriction? More importantly, how can a module decide whether its SendRestrictionFn should be prepended vs appended? It seems like a global decision that can't be made locally.

[SpicyLemon]

The ordering of send restrictions might matter. I expect most things will use Append, but there could easily be a situation where a module needs theirs to go before the ones already injected.

I hate when I get into a situation where I just can't do what I know I need to do because the only layer I have access too didn't provide enough control. So, while Append will usually be good enough, Prepend and Clear can easily save the day.

[elias-orijtech]

could easily be a situation where a module needs theirs to go before the ones already injected.

But how does the module know whether it's safe to insert its restriction function before all others? That is, I see a problem where modules can be fighting each other for the correct position of their restrictions.

[SpicyLemon]

The module can't know. The blockchain author can though, and needs to have the tools available to fix ordering problems if the arise.

[elias-orijtech]

When is this needed? If you've added restrictions, do you ever want to remove them again?

[SpicyLemon]

Clear() is needed for situations where a blockchain author needs to change the ordering of restrictions, and can't do so by other means.

[GNaD13]

Can we include this feature in v0.46?

[tac0turtle]

we are unable to backport this, it does have breaking changes