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

Keeper Send Coins does not perform expected validation #414

Closed
iramiller opened this issue Feb 19, 2021 · 1 comment · Fixed by #446
Closed

Keeper Send Coins does not perform expected validation #414

iramiller opened this issue Feb 19, 2021 · 1 comment · Fixed by #446
Assignees
Labels
bug Something isn't working
Milestone

Comments

@iramiller
Copy link
Contributor

Summary

Within the CosmWasm keeper the function that sends coins using the bank module performs some of the validation that should be within the bank keeper but not all. Currently the blockedaddresses are checked but not the send_enabled coins. This allows a smart contract to bypass restrictions that prevent account holders from transfering coins

sdkerr := k.bankKeeper.SendCoins(ctx, caller, contractAddress, coins)

The bank module only performs the send disabled checks on the msg_server send method.
https://github.com/cosmos/cosmos-sdk/blob/73e38e4009a57951b72e2998b4e3d5db2d499d04/x/bank/keeper/msg_server.go#L29

Proposed Fix

The CosmWasm project should either implement all of the checks the bankmodule does on the send method or optionally switch from the direct unprotected keeper call to using the Send method which implements these checks

@ethanfrey
Copy link
Member

Seems reasonable. I would prefer to just using the service module rather than the keeper. ADR 33 is about deprecating direct keeper access anyway.

@ethanfrey ethanfrey added this to the v0.16.0 milestone Feb 19, 2021
@alpe alpe self-assigned this Mar 10, 2021
@alpe alpe added the bug Something isn't working label Mar 10, 2021
@alpe alpe closed this as completed in #446 Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants