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...

• [x] included the correct type prefix in the PR title
• [x] added ! to the type prefix if API or client breaking change
• [x] targeted the correct branch (see PR Targeting)
• [x] provided a link to the relevant issue or specification
• [ ] followed the guidelines for building modules
• [x] included the necessary unit and integration tests
• [x] added a changelog entry to CHANGELOG.md
• [x] included comments for documenting Go code
• [x] updated the relevant documentation or specification
• [x] 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.

[sonarqubecloud[bot]]

[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.

[alexanderbez]

Great summary @SpicyLemon -- I recall there being some discussion during the last SDK community call with @JimLarson and some use-cases Agoric had and we discussed if the work done here can be done in such a way that is general to support the cases you've laid as well as the one's @JimLarson pointed out.

I guess this is a question more for @JimLarson -- are the proposed API changes here sufficient for your needs as well?

[SpicyLemon]

I've brought this up-to-date and resolved any conflicts.

Are there any more thoughts on this?

Are there further changes needed?

[JimLarson]

Sorry for the late reply - I was out for the holidays, then came down with a cold upon my return.

This is a very nice mechanism and I think the changes contemplated in #14271 would play nicely as yet another restriction - as long as the vesting-based locking is refactored into such a restriction. This would disentangle the check-and-action of subUnlockedCoins() into a restriction and an action subCoins() - for nice symmetry with addCoins(). The multi-send operation would need a little refactoring and look more like a simple iteration of single-recipient transfers, which is probably a good change.

[JimLarson]

On the other hand, changing multi-send into iterated single-sends would mean different events were generated, which might break other things. Maybe event generation could be deferred and aggregated in a reliable way. Or do a send restriction check for the sender for the total amount with the recipient address unspecified? There's probably some way to handle this with the current efficiency and event behavior.

[JimLarson]

Looking at the code more closely, it looks like the best approach would be to lift the event code out of subUnlockedCoins() and addCoins() into their callers - which also do event generation, so this would consolidate the event code. And even better than symmetric addCoins() and subCoins(), we could have only a single xferCoins() which does both, making the conservation of coins even more clear.

[SpicyLemon]

On the other hand, changing multi-send into iterated single-sends would mean different events were generated, which might break other things. Maybe event generation could be deferred and aggregated in a reliable way. Or do a send restriction check for the sender for the total amount with the recipient address unspecified? There's probably some way to handle this with the current efficiency and event behavior.

Honestly. After #12601, MultiSend/InputOutputCoins isn't very useful. I'd be in favor of removing InputOutputCoins altogether and either removing MultiSend too or changing it to a loop of SendCoins.

This is a very nice mechanism and I think the changes contemplated in https://github.com/cosmos/cosmos-sdk/discussions/14271 would play nicely as yet another restriction - as long as the vesting-based locking is refactored into such a restriction. This would disentangle the check-and-action of subUnlockedCoins() into a restriction and an action subCoins() - for nice symmetry with addCoins().

The biggest hurdle there is how to handle functions like SpendableCoins and LockedCoins. A send restriction doesn't alter those functions, but in some cases (e.g. vesting), there needs to be some symmetry.

[alexanderbez]

Tagging @fdymylja for visibility here since he was the vesting abstraction completely isolated from the BaseAccount type.

[robert-zaremba]

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.

I think this is related to a bigger problem in Cosmos SDK. The module exposes lot of API, and many other modules can import any part of the API, making it tight coupled and bad. IMHO, exposing keeper is very bad paradigm (and all keeper interfaces between the modules). Instead we should have subinterfaces of the msg_server interface.

The ADR-33 is supposed to change that, but it's still long to go.

[kocubinski]

Instead we should have subinterfaces of the msg_server interface.

@robert-zaremba I want to understand what you mean by this, could you elaborate a bit?

[robert-zaremba]

Instead we should have subinterfaces of the msg_server interface.

@robert-zaremba I want to understand what you mean by this, could you elaborate a bit?

@kocubinski in general, it's good to separate API into module private and module public. It helps to encapsulate things. IMO, msg server an querier should be the main exposed API, and keeper should stay private to the module (if possible).

So, other modules will depend on the subset of msg server or querier API, not a keeper.

[SpicyLemon]

Instead we should have subinterfaces of the msg_server interface.

@robert-zaremba I want to understand what you mean by this, could you elaborate a bit?

@kocubinski in general, it's good to separate API into module private and module public. It helps to encapsulate things. IMO, msg server an querier should be the main exposed API, and keeper should stay private to the module (if possible).

So, other modules will depend on the subset of msg server or querier API, not a keeper.

I disagree with this.

The msg_server and grpc_query endpoints are exposed at the blockchain user level. This is the public-facing API that is available on the blockchain. But some modules provide functionality that doesn't make sense to fully expose in the public API. Maybe it's a function that when used, requires other followups. Maybe it's a function simply not designed for a public API. Maybe it's just a confusing function or a function easily confused with another endpoint. There's all sorts of reasons to have one API for blockchain users and another for blockchain developers.

By having an separate developer-focused API (i.e. the keepers), you can expose more granular functionality to developers of the chain or other modules. Additionally, there's less overhead when calling the keeper functions than a msg_server or grpc_query endpoint, which affects gas costs.

Plus, exposing a msg_server or grpc_query endpoint requires significantly more work than exposing a keeper function. I feel like the SDK already keeps too many things private. That will only get worse if there's extra work required to expose such functionality. But first, someone has to identify that a certain functionality might possibly be useful to others without exactly knowing how or why.

Furthermore, packing and unpacking the context and messages for use with a msg_server or grpc_query endpoint means they are almost always more difficult to use than a keeper function; a line calling a keeper function almost always reads better than one calling one of those endpoints. Encoding and decoding bech32 strings isn't free. At the very least, it's expensive enough that the bank's Send endpoint attempts to do that conversion as rarely as possible.

---

For example, consider the x/authz module.

Because the GetAuthorization, SaveGrant, and DeleteGrant functions are exposed in the keeper, it allowed us (at Provenance) to utilize x/authz authorizations when processing msg_server endpoints without requiring the use of the Exec endpoint. Specifically, in our x/metadata module, we often need to have multiple signers. The Exec endpoint can only handle a single signer though, so that can't be used in most x/metadata cases. However, we were able to update our module to do it's own checking for authorizations when signatures are missing. Without having those functions available, the x/authz module would be pretty much useless for us.

The Grants grpc_query endpoint is similar to the GetAuthorization keeper function. But the former has significantly more processing to do. It also makes it harder to distinguish between an error that was encountered and an entry not being found; an err == nil check is easier, more accurate, and less bug-prone than processing the contents of an error.

The Grant and Revoke msg_server endpoints could be used instead of SaveGrant, and DeleteGrant, but again, they require extra processing and do validation that isn't needed in these cases.

Exposing the streamlined GetAuthorization, SaveGrant, and DeleteGrant keeper functions as grpc_query or msg_server endpoints a) requires significantly more work than just creating a public keeper function, and b) clutters the blockchain's authz API.

When someone wants to create an authz grant, it's easier for them to know what to use if there's only the one Grant endpoint; they don't have to waste energy figuring out if they should use Grant or SaveGrant. So, it's better to not have both exposed in the same API. By exposing SaveGrant to at the developer level (e.g. as a keeper function), though, it allows use of the authz module by blockchain engineers for uses not previously thought of when the x/authz module was designed.

---

Another example is the bank module's SendCoins function. There are tons of reasons why another module might need to move funds, and that process needs to be as streamlined as possible. Using the Send endpoint requires significantly more processing than SendCoins.

Also, the Send endpoint checks the SendEnabled flags and blockedAddrs, but SendCoins doesn't. If only the Send endpoint were available, you would have had to recognize that there are some times when those checks should be skipped, and design the endpoint to allow bypassing that check. But being able to bypass that check is not something that should be available from the outside. E.g. a MsgSend should not be allowed to bypass the check, but another module moving funds should be allowed to skip that check when they need to.

For example our (Provenance's) x/marker module sets SendEnabled to false for some denoms so that you can't use a MsgSend to transfer certain coins. The module has it's own MsgTransfer though, that does the needed authorization checking, and moves the coins. There is no way we could have done that without the separation of Send and SendCoins.

---

All that being said, though, not having the SendEnabled checking inside SendCoins has caused multiple bugs that allowed the transfer of SendEnabled == false denoms when it shouldn't have been allowed. That checking can easily be redone as a bypassable SendRestrictionFn though. That way, the only time that check is bypassed is when explicitly coded told to do so; we don't have to rely on engineers remembering to make that needed check before using SendCoins.

var _ SendRestrictionFn = BaseSendKeeper{}.SendEnabledRestriction

var bypassSendEnabledCheckKey = "bypass-send-enabled-check"

// WithSendEnabledBypass returns a new context that will cause the send-enabled restriction to be skipped.
func WithSendEnabledBypass(ctx sdk.Context) sdk.Context {
	return ctx.WithValue(bypassSendEnabledCheckKey, true)
}

// HasSendEnabledBypass checks the context to see if the send-enabled restriction should be skipped.
func HasSendEnabledBypass(ctx sdk.Context) bool {
	hasBypass := ctx.Value(bypassSendEnabledCheckKey)
	if hasBypass == nil {
		return false
	}
	bypass, success := hasBypass.(bool)
	return success && bypass
}

// SendEnabledRestriction is IsSendEnabledCoins rebranded as a SendRestrictionFn.
// This restriction can be bypassed by using WithSendEnabledBypass on the provided context.
func (k BaseSendKeeper) SendEnabledRestriction(ctx sdk.Context, _, toAddr sdk.AccAddress, amt sdk.Coins) (newToAddr sdk.AccAddress, err error) {
	if HasSendEnabledBypass(ctx) {
		return toAddr, nil
	}
	return toAddr, k.IsSendEnabledCoins(ctx, amt...)
}

The blockedAddrs check can be similarly done as a restriction with a bypass like this.

Then you'd have a single place for other modules to use to transfer funds that runs all needed checks unless the module specifically says to bypass them. That makes it significantly harder for new bugs to crop up where the SendEnabled check isn't being performed, but should be. It'd be done as part of SendCoins unless explicitly told otherwise.

[SpicyLemon]

There are many reason why a send might need to be blocked. It's unreasonable to place the burden of making all those checks on the caller of SendCoins. For starters, the x/staking module shouldn't need to know about my x/marker module. But when x/staking is transferring coins, I still need certain checks related to x/marker to be performed. I don't have control of the x/staking module, so I can't add that check in there. It also shouldn't be my burden to identify all uses of SendCoins so I can make sure that check is always being done.

The SendCoins function is the focal point of a crucial action: transferring funds. When I've got a restriction that needs to be applied to any and all fund movements, that's the best place to have it done. By allowing those restrictions to be injected into the bank module, I don't have to worry about needing to inject them into all other modules that might call SendCoins. I don't have to rely on everything else applying my restrictions (that they probably don't know about) before calling SendCoins.

[robert-zaremba]

Well, i think that only the proto interface should be exposed by default. This is the module public interface, all rest is the implementation dealtail. Especially the store access.

[iramiller]

I suppose in summary… it’s the Cosmos SDK not the Cosmos Blockchain. It is important that developers building with this can control enough of the implementation that they can build what is needed while exposing a separate layer to the blockchain users.

I think the separation here between the two different audiences is often lost in these discussions unfortunately.

[robert-zaremba]

unpacking the context and messages for use with a msg_server or grpc_query

This is going to change.

Encoding and decoding bech32 strings isn't free.

here as well. There is a task to use use a wrapper type I think.

Without having those functions available, the x/authz module would be pretty much useless for us.

Maybe that's a good feedback for the x/authz service API?

In general, I see your point @SpicyLemon . However we should still be wise what we expose and how. Maybe there should be additional layer between msg server and storage? In the past I was championing an idea to separate keeper from the data access.

[github-actions[bot]]

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.

[SpicyLemon]

Is there anything still holding this back?

I feel like the auth overhaul being discussed won't remove the need for this functionality. And this functionality is very much needed.

[SpicyLemon]

how does this compare to before and after hooks that other teams have adopted? it seems like they could function in the same way?

Key differences:

1. A SendRestrictionFn allows alteration of the destination address.
2. It's easier for a module to add its own SendRestrictionFn. The SendRestrictionFns are added after the bank keeper is created and without requiring knowledge of all previously added SendRestrictionFns. Since many modules/keepers depend on the bank keeper, it's usually one of the first created. Any module/keeper that needs to define a SendRestrictionFn can take in the bank keeper and add the SendRestrictionFn on its own, and it's automatically composed with any existing restrictions.
3. SendRestrictionFns apply to InputOutputCoins too.

Also, I still don't see the need for a separate after hook. State changes during a Send are small enough that if resulting balances are needed (by the hook), it's just an extra safeSub line.

[tac0turtle]

echoing what julien said, need some docs updated and also it seems this doesnt solve the issue you opened. I dont see a way to restrict then later accept the transfer, is this on purpose?

the unfortunate part of this feature is i think we will also need to add before and after send hooks. Unfortunately this wasnt coordinated and this is why this pr has been sitting for a while. Im still hesitant to accept this pr becuase if we keep adding features without thinking it through on all the use cases then bank becomes an abomination of features even more than today. Secondly, we will refactor bank in the near future to reduce the scope of what it does and i cant promise this feature will land there as we dont have a design in mind right now.

[SpicyLemon]

lgtm! Is there a place to describe this feature in the bank docs?

I added some documentation to the spec. I'm not sure if it's in the right spot or overly detailed though. https://github.com/cosmos/cosmos-sdk/blob/fdd3354ccc8d9e13fbd5c90208aa3c4e356b8aed/x/bank/README.md?plain=1#L263-L341

I also changed where the send restriction is applied during SendCoins. It's now done after subUnlockedCoins but before addCoins. I did this for two reasons:

1. The subUnlockedCoins call is the most probable thing to have an error. That was one of the points made when discussing this single SendRestrictionFn vs before/after send hooks.
2. Consistency with InputOutputCoins. For the most part, there's no way for a SendRestrictionFn to know if it's being run during SendCoins or InputOutputCoins. But if it needs to do things based on balances, it'll need to know what has and hasn't been done yet. During InputOutputCoins, the restriction is called for each output. That happens after the subUnlockedCoins. In order to be consistent, I needed to either change SendCoins as I did or else move the subUnlockedCoins to the end of InputOutputCoins. But since subUnlockedCoins is the primary failure point, it makes sense to keep it first.

I thought about refactoring the SendEnabled check as a send restriction, but that would have been a tricky breaking change for anyone using SendCoins or InputOutputCoins, so I held off. It'd be better as it's own PR anyway.

I dont see a way to restrict then later accept the transfer, is this on purpose?

While implementing some send restrictions of our own, I realized that I only ever wanted to bypass a single specific restriction, but I still needed all other restrictions to be applied. By putting a bypass check in each restriction, I could skip what I need to and not care about whatever other restictions might be there. The code calling either SendCoins or InputOutputCoins indicates which restriction(s) should be skipped by using context values, and each restriction checks the context for its own bypass before doing anything.

If the only way to bypass a restriction is to bypass them all and manually apply the others, the code making that call would need to have knowledge of all other restrictions that might be present on the chain, which isn't really possible from inside a module. In most (if not all) cases, the need is to bypass certain specific restrictions, but not all of them. It's like how, technically, any time someone uses SendCoins, they should also check SendEnabled first (which isn't obvious), but even worse now since there's other checks that are difficult (or impossible) for a module to know about.

---

For example, our (Provenance's) x/marker module has an endpoint that facilitates transfer of certain funds. It requires more info than is in a MsgSend, though (i.e. an admin/signer). During such a transfer, we want to bypass only the send restriction that the x/marker module adds, any other restrictions MUST still be applied. So, in that transfer endpoint, we add a bypass flag to the context that's provided to SendCoins, a flag that only that one restriction uses.

Context helpers (Note that this example still uses the sdk.Context, but the bank spec docs in this PR have them using a context.Context): https://github.com/provenance-io/provenance/blob/v1.16.0/x/marker/types/send_restrictions.go

package types

import sdk "github.com/cosmos/cosmos-sdk/types"

var bypassKey = "bypass-marker-restriction"

// WithBypass returns a new context that will cause the marker bank send restriction to be skipped.
func WithBypass(ctx sdk.Context) sdk.Context {
	return ctx.WithValue(bypassKey, true)
}

// WithoutBypass returns a new context that will cause the marker bank send restriction to not be skipped.
func WithoutBypass(ctx sdk.Context) sdk.Context {
	return ctx.WithValue(bypassKey, false)
}

// HasBypass checks the context to see if the marker bank send restriction should be skipped.
func HasBypass(ctx sdk.Context) bool {
	bypassValue := ctx.Value(bypassKey)
	if bypassValue == nil {
		return false
	}
	bypass, isBool := bypassValue.(bool)
	return isBool && bypass
}

The SendRestrictionFn: https://github.com/provenance-io/provenance/blob/v1.16.0/x/marker/keeper/send_restrictions.go#L18-L32

func (k Keeper) SendRestrictionFn(ctx sdk.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.AccAddress, error) {
	// In some cases, it might not be possible to add a bypass to the context.
	// If it's from either the Marker or IBC Transfer module accounts, assume proper validation has been done elsewhere.
	if types.HasBypass(ctx) || fromAddr.Equals(k.markerModuleAddr) || fromAddr.Equals(k.ibcTransferModuleAddr) {
		return toAddr, nil
	}

	for _, coin := range amt {
		if err := k.validateSendDenom(ctx, fromAddr, toAddr, coin.Denom); err != nil {
			return nil, err
		}
	}

	return toAddr, nil
}

The call to `SendCoins: https://github.com/provenance-io/provenance/blob/main/x/marker/keeper/marker.go#L649

err = k.bankKeeper.SendCoins(types.WithBypass(ctx), from, to, sdk.NewCoins(amount))

The marker module doesn't have to know about the existence any other checks in order for them to still be applied.

---

In the end, there's a set of restrictions that I always want applied when any funds are being moved. Sometimes, though, a module is facilitating the transfer, and there's one or two specific restrictions that should be bypassed, but all others still need to be applied, and the facilitating module shouldn't have to know what all those other restrictions are. It only needs to know about which ones to bypass, which, in most cases, will be the one added by the module itself.

Here are some needs that this PR fills:

1. As a module author, I need a way to have a check and some processing done any time funds are being transferred so that I can restrict any/all transfers based on my own logic.
2. As the quarantine module author, I need a way to alter the destination address of a transfer (and record some data) so that I properly facilitate quarantine for an account (i.e. take note of the funds in my module's state and have them transferred to an intermediary account instead).
3. As the author of a module that sometimes needs to move funds, I sometimes want to bypass specific restrictions, but not all restrictions, so that a) my module can move the funds as needed, b) other restrictions (that my module doesn't know/care about) are still applied as expected by the authors of those modules and blockchains.
4. As a blockchain author, I don't want to have to know about all the send restrictions that my chain's modules might have so that it's less likely that I accidentally miss one that's required for a module to properly function. In other words, I want each module to tell the bank module about its restrictions without requiring extra code from me.
5. As a blockchain author, during app creation, I want to initialize the bank module early on, and have restrictions added later (as other modules are initialized) because it's easier to just know that the bank module should be initialized as one of the first, allowing other modules to inform it than it is to initialize the other modules first and then later inform them of the bank module. In most cases, a module with a send restriction will require other bank module functionality too.
6. As a blockchain author, if there's a bug involving the order that the send restriction functions are being applied, I want to be able to redefine them using my own ordering. This would require knowledge of all my chain's restrictions (at least to a certain point in code), but it still needs to be an option.

[github-actions[bot]]

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.

[robert-zaremba]

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

[GNaD13]

Can we include this feature in v0.46?

[tac0turtle]

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