fix: adding check for blocked addresses before escrowing fees by seantking · Pull Request #1890 · cosmos/ibc-go

seantking · 2022-08-04T18:11:57Z

Description

closes: #1889

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

seantking · 2022-08-04T18:17:19Z

testing/simapp/app.go

+		// do not add the following modules to blocked addresses
 		// this is only used for testing
-		if acc == ibcmock.ModuleName {
+		if acc == ibcmock.ModuleName || acc == distrtypes.ModuleName {


This is pretty hacky I just left it in for discussion. I'm guessing we don't want to unblock the distribution module?

We have the following test case. We ensure a non-blocked module account can escrow fees.

When I try to use the mock module account I get an AuthKeeper error saying the account doesn't exist. I'm not sure why exactly. I've left the test case as is using the distribution module account for now.

cc: @colin-axner

I was looking at the test case "refund account is a module account" and trying to use ibcmock.ModuleName instead of disttypes.ModuleName:

"refund account is module account", func() { msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String() expPacketFee := types.NewPacketFee(fee, msg.Signer, nil) expFeesInEscrow = []types.PacketFee{expPacketFee} }, true,

The account does not exist error is being returned from here in escrow.go. And interestingly if we actually use AccountKeeper.GetModuleAccount() we will not encounter that error as it calls directly into GetModuleAccountAndPermissions() which creates and sets the account in x/auth.

This differs from GetModuleAddress which just checks the permAddrs map created on keeper initialisation.

It's definitely pretty ambiguous as to what constitutes a blocked address and what doesn't. Without this logical OR added for distrtypes.ModuleName it is and will be in the blockedAddrs map in x/bank. However, its usage in x/distribution when for example, withdrawing delegation rewards uses SendCoinsFromModuleToAccount which only checks that the recipientAddr is not a blocked address.

When I use the following in the malleate() func, we don't fail on the account not existing but instead fail with an "insufficient funds" error.

moduleAcc := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), ibcmock.ModuleName) msg.Signer = moduleAcc.GetAddress().String() expPacketFee := types.NewPacketFee(fee, msg.Signer, nil) expFeesInEscrow = []types.PacketFee{expPacketFee}

It would be nice to get some more clarity around module accounts in general. We could use the above and also fund the ibcmock.ModuleName module account to get past the error.

If the blocked address logic gets removed from the SDK, what test case would we want to have? I'd say probably still wise to just use the mock module account since it may not make sense long term to use distr (as community pool might be removed from it)

Maybe we can add the mock module to the list of genAccs in NewTestChainWithValSet?

Also I think you can use GetModuleAddress().String()

codecov-commenter · 2022-08-04T18:44:53Z

Codecov Report

Merging #1890 (e3115f5) into main (f891c29) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1890   +/-   ##
=======================================
  Coverage   80.12%   80.13%           
=======================================
  Files         167      167           
  Lines       11750    11770   +20     
=======================================
+ Hits         9415     9432   +17     
- Misses       1921     1923    +2     
- Partials      414      415    +1

Impacted Files	Coverage Δ
modules/apps/29-fee/keeper/msg_server.go	`96.96% <100.00%> (+0.76%)`	⬆️
modules/apps/29-fee/keeper/escrow.go	`93.16% <0.00%> (-1.87%)`	⬇️

seantking · 2022-08-04T19:13:31Z

Codecov Report

Merging #1890 (8525e4e) into main (af4e651) will decrease coverage by 0.12%.
The diff coverage is 68.80%.
@@            Coverage Diff             @@
##             main    #1890      +/-   ##
==========================================
- Coverage   80.04%   79.92%   -0.13%     
==========================================
  Files         166      166              
  Lines       12421    12446      +25     
==========================================
+ Hits         9943     9947       +4     
- Misses       2013     2030      +17     
- Partials      465      469       +4     
Impacted Files Coverage Δ
cmd/build_test_matrix/main.go 70.83% <0.00%> (-3.46%)
...27-interchain-accounts/controller/keeper/keeper.go 94.73% <ø> (ø)
.../apps/27-interchain-accounts/host/keeper/keeper.go 83.33% <ø> (ø)
modules/apps/27-interchain-accounts/module.go 55.55% <0.00%> (-2.14%)
modules/apps/29-fee/keeper/keeper.go 92.48% <ø> (ø)
modules/apps/29-fee/module.go 54.54% <0.00%> (-1.02%)
modules/apps/transfer/keeper/keeper.go 91.17% <ø> (ø)
modules/apps/transfer/keeper/migrations.go 93.10% <ø> (-0.23%)
modules/apps/transfer/keeper/relay.go 88.11% <0.00%> (ø)
modules/apps/transfer/module.go 57.14% <0.00%> (-0.93%)
... and 46 more

Not too sure what's going on with the code cov here. I don't have access to the website either.

crodriguezvega

Looks good to me. Thanks @seantking for fixing the issue so quickly! ⚡

damiannolan · 2022-08-05T09:20:27Z

modules/apps/29-fee/keeper/msg_server.go

+	refundAcc, err := sdk.AccAddressFromBech32(msg.PacketFee.RefundAddress)
+	if err != nil {
+		return nil, err
+	}
+
+	if k.bankKeeper.BlockedAddr(refundAcc) {
+		return nil, sdkerrors.Wrapf(types.ErrBlockedAddress, "%s", refundAcc)
+	}
+
 	if k.IsLocked(ctx) {
 		return nil, types.ErrFeeModuleLocked
 	}


Should we move the check to below k.IsLocked(ctx) for consistency with PayPacketFee rpc?

colin-axner

ACK. Nice work. Would prefer to resolve the testing/simapp/app.go discussion before merging, but I'm ok doing it in a followup

crodriguezvega · 2022-08-08T13:18:07Z

modules/apps/29-fee/keeper/msg_server.go

+		return nil, err
+	}
+
+	if k.bankKeeper.BlockedAddr(refundAcc) {


Maybe instead of adding a new error, do something like?

if k.bankKeeper.BlockedAddr(refundAcc) { return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to escrow fees", refundAcc) }

crodriguezvega · 2022-08-09T08:09:35Z

@seantking @colin-axner I am wondering if we should also add a check to ensure that the payee address is not blocked in the rpc handler for the MsgRegisterPayee message. This is executed on the source chain with a source chain address, so we could potentially already make sure that the payee is not blocked and prevent failures on distribution. What do you think?

colin-axner · 2022-08-09T10:01:06Z

I am wondering if we should also add a check to ensure that the payee address is not blocked in the rpc handler for the MsgRegisterPayee message.

Yes definitely, nice catch!

seantking · 2022-08-09T11:23:52Z

@seantking @colin-axner I am wondering if we should also add a check to ensure that the payee address is not blocked in the rpc handler for the MsgRegisterPayee message. This is executed on the source chain with a source chain address, so we could potentially already make sure that the payee is not blocked and prevent failures on distribution. What do you think?

Great idea! Added a check and test for this.

modules/apps/29-fee/keeper/msg_server_test.go

colin-axner

followup ACK

crodriguezvega

🥇

* fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903) # Conflicts: # modules/apps/29-fee/keeper/msg_server_test.go

* fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903)

#1890) (#1950) * fix: adding check for blocked addresses before escrowing fees (#1890) * fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903) # Conflicts: # modules/apps/29-fee/keeper/msg_server_test.go * fix conflicts Co-authored-by: Sean King <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>

#1890) (#1951) * fix: adding check for blocked addresses before escrowing fees (#1890) * fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903) * fix merge error Co-authored-by: Sean King <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>

cosmos#1890) (cosmos#1950) * fix: adding check for blocked addresses before escrowing fees (cosmos#1890) * fix: adding check for blocked addresses before escrowing fees * refactor: move check below isLocked check * refactor: use sdk error instead of fee error * feat: adding check to RegisterPayee * chore: format import * refactor: remove dist module from unblocked addr (cherry picked from commit 7694903) # Conflicts: # modules/apps/29-fee/keeper/msg_server_test.go * fix conflicts Co-authored-by: Sean King <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]>

fix: adding check for blocked addresses before escrowing fees

8525e4e

seantking requested review from AdityaSripal, charleenfei, chatton, colin-axner, crodriguezvega and damiannolan as code owners August 4, 2022 18:11

seantking commented Aug 4, 2022

View reviewed changes

crodriguezvega approved these changes Aug 5, 2022

View reviewed changes

damiannolan reviewed Aug 5, 2022

View reviewed changes

colin-axner approved these changes Aug 8, 2022

View reviewed changes

crodriguezvega reviewed Aug 8, 2022

View reviewed changes

seantking and others added 4 commits August 9, 2022 12:52

Merge branch 'main' into sean/issue#1889-fix-module-acc-refund

42d6666

refactor: move check below isLocked check

808ee69

refactor: use sdk error instead of fee error

337df64

feat: adding check to RegisterPayee

e9248c4

Merge branch 'main' into sean/issue#1889-fix-module-acc-refund

8a6b1f1

colin-axner reviewed Aug 9, 2022

View reviewed changes

modules/apps/29-fee/keeper/msg_server_test.go Show resolved Hide resolved

chore: format import

e3115f5

colin-axner approved these changes Aug 9, 2022

View reviewed changes

seantking and others added 2 commits August 9, 2022 19:49

Merge branch 'main' into sean/issue#1889-fix-module-acc-refund

6309549

refactor: remove dist module from unblocked addr

ffb9e33

crodriguezvega approved these changes Aug 9, 2022

View reviewed changes

crodriguezvega merged commit 7694903 into main Aug 9, 2022

crodriguezvega deleted the sean/issue#1889-fix-module-acc-refund branch August 9, 2022 19:01

crodriguezvega added backport-to-v4.0.x labels Aug 9, 2022

mergify bot mentioned this pull request Aug 9, 2022

fix: adding check for blocked addresses before escrowing fees (backport #1890) #1950

Merged

mergify bot mentioned this pull request Aug 9, 2022

fix: adding check for blocked addresses before escrowing fees (backport #1890) #1951

Merged

chatton mentioned this pull request Aug 24, 2022

Consider using git-cliff to generate CHANGELOG.md #2106

Closed

3 tasks

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

ulbqb mentioned this pull request Jul 31, 2023

Bump up to v4.3.1 Finschia/ibc-go#19

Merged

9 tasks

Conversation

seantking commented Aug 4, 2022

Description

Uh oh!

seantking Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damiannolan Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colin-axner Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

seantking commented Aug 4, 2022

Codecov Report

Uh oh!

crodriguezvega left a comment

Choose a reason for hiding this comment

Uh oh!

damiannolan Aug 5, 2022

Choose a reason for hiding this comment

Uh oh!

colin-axner Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

crodriguezvega Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

seantking Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

crodriguezvega commented Aug 9, 2022

Uh oh!

colin-axner commented Aug 9, 2022

Uh oh!

seantking commented Aug 9, 2022

Uh oh!

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

crodriguezvega left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seantking Aug 4, 2022 •

edited

Loading

damiannolan Aug 5, 2022 •

edited

Loading

codecov-commenter commented Aug 4, 2022 •

edited

Loading