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

feat(bank): Allow injectable restrictions on bank transfers #14224

Merged
merged 58 commits into from
Aug 18, 2023

Conversation

SpicyLemon
Copy link
Collaborator

@SpicyLemon SpicyLemon commented Dec 8, 2022

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)

…and give it similar stuff. Add unit tests for it.
…the MsgMultiSend can now only contain a single input (#12610 and #12670).
…ing SendCoins to SendCoinsWithoutRestriction and create a new SendCoins that just applies the restriction then calls SendCoinsWithoutRestriction. Make restriction calls from InputOutputCoins too.
…ctions. It can be up to each restriction to decide if it should be bypassed.
…an be updated from inside the keeper without needing a pointer receiver.
…e the keeper AppendSendRestriction and PrependSendRestriction.
@SpicyLemon SpicyLemon changed the title Allow injectable restrictions on bank transfers feat(bank)!: Allow injectable restrictions on bank transfers Dec 8, 2022
@SpicyLemon SpicyLemon marked this pull request as ready for review December 8, 2022 17:30
@SpicyLemon SpicyLemon requested a review from a team as a code owner December 8, 2022 17:30
@iramiller
Copy link
Contributor

iramiller commented Dec 8, 2022

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
Copy link

sonarcloud bot commented Dec 9, 2022

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@hxrts
Copy link
Contributor

hxrts commented Dec 20, 2022

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
Copy link
Collaborator Author

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
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Jul 31, 2023
@github-actions github-actions bot removed the Stale label Aug 1, 2023
Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @SpicyLemon!

@robert-zaremba
Copy link
Collaborator

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

@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 18, 2023
@@ -22,7 +22,11 @@ import (
type SendKeeper interface {
ViewKeeper

InputOutputCoins(ctx context.Context, inputs types.Input, outputs []types.Output) error
AppendSendRestriction(restriction types.SendRestrictionFn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@julienrbrt julienrbrt removed the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 18, 2023
@julienrbrt julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Aug 18, 2023
@julienrbrt julienrbrt mentioned this pull request Aug 18, 2023
15 tasks
@julienrbrt julienrbrt added this pull request to the merge queue Aug 18, 2023
Merged via the queue into main with commit ff2e5a2 Aug 18, 2023
47 of 49 checks passed
@julienrbrt julienrbrt deleted the dwedul/14124-bank-restrictions branch August 18, 2023 22:50
mergify bot pushed a commit that referenced this pull request Aug 18, 2023
(cherry picked from commit ff2e5a2)

# Conflicts:
#	CHANGELOG.md
Comment on lines +239 to +240
AppendSendRestriction(restriction SendRestrictionFn)
PrependSendRestriction(restriction SendRestrictionFn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SendCoins(ctx context.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
AppendSendRestriction(restriction SendRestrictionFn)
PrependSendRestriction(restriction SendRestrictionFn)
ClearSendRestriction()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

GNaD13 commented Dec 26, 2023

Can we include this feature in v0.46?

@tac0turtle
Copy link
Member

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

@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release C:x/bank C:x/gov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to quarantine and sanctioned accounts.