[pfm][fix] patch arbitrary account passthrough #71

agouin · 2023-07-25T21:29:25Z

https://twitter.com/SCVSecurity/status/1682329758020022272

To ensure that users cannot pass funds through arbitrary accounts on intermediate chains, use hash of pfm+sender+channel.

e2e test: strangelove-ventures/interchaintest#678

…+sender+channel

javiersuweijie · 2023-07-26T09:01:35Z

Hey @agouin, a PR has been made that performed the same fix: #72. Would greatly appreciate to use that instead since the tests have been fixed.

The PR also handles some edge cases when dealing with fee processing during retries.

agouin · 2023-07-26T16:14:21Z

Hey @javiersuweijie thanks for your PR. I did not see the prior #69 that used the module account before starting this, my apologies. I put this up as a draft so that I could test it in the interchaintest packet forward test . I left some comments on your PR, but I think the code changes should look more like what's here in this PR.

nicolaslara · 2023-07-27T15:52:24Z

middleware/packet-forward-middleware/router/ibc_middleware.go

+// the receiver address is deterministic and can be used to identify the sender on the
+// initial chain.
+func getReceiver(channel string, originalSender string) (string, error) {
+	senderStr := fmt.Sprintf("%s/%s", channel, originalSender)


Should we also use the port here? (in case this is used to wrap a port other than transfer in the future)

^ this makes sense to me

This is not necessary, since PFM is ICS-20 middleware. Also, channels are on top of a port in IBC, so in the case this was modified in the future to support non-transfer packets, the channel ID would still give us enough to cover the channel+port. https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel

jtieri

I agree with @nicolaslara that adding the port to the hash for the overridden receiver could future proof this.

Otherwise this lgtm. The only other change I would suggest is making note of the new behavior somewhere in the readme since this is somewhat of a substantial change to how the PFM works.

jtieri · 2023-07-27T16:27:38Z

middleware/packet-forward-middleware/router/ibc_middleware.go

+// the receiver address is deterministic and can be used to identify the sender on the
+// initial chain.
+func getReceiver(channel string, originalSender string) (string, error) {
+	senderStr := fmt.Sprintf("%s/%s", channel, originalSender)


^ this makes sense to me

override packet fix incorrect bech32 update error desc refactor receiveFunds

agouin · 2023-07-29T05:10:45Z

README has been updated to explain the intermediate receiver behavior and recommended usage of the packet receiver field, since we can use this fix as a feature to detect PFM on intermediate chains.

* don't allow funds to pass through arbitrary accounts, use hash of pfm+sender+channel * lint * use types.ModuleName * update tests * better errors override packet fix incorrect bech32 update error desc refactor receiveFunds * update README * tidy errors * lint * chore: remove go.work * chore: lint --------- Co-authored-by: jtieri <[email protected]> (cherry picked from commit d72e83c) # Conflicts: # .gitignore # middleware/packet-forward-middleware/router/module_test.go # modules/ibc-hooks/client/cli/query.go # modules/ibc-hooks/hooks.go # modules/ibc-hooks/ibc_module.go # modules/ibc-hooks/ics4_middleware.go # modules/ibc-hooks/keeper/keeper.go # modules/ibc-hooks/sdkmodule.go # modules/ibc-hooks/simapp/ante.go # modules/ibc-hooks/simapp/app.go # modules/ibc-hooks/simapp/export.go # modules/ibc-hooks/simapp/simulation_test.go # modules/ibc-hooks/simapp/test_helpers.go # modules/ibc-hooks/tests/unit/mocks/isc4_middleware_mock.go # modules/ibc-hooks/tests/unit/module_test.go # modules/ibc-hooks/wasm_hook.go

…ugh (#78) * [pfm][fix] patch arbitrary account passthrough (#71) * don't allow funds to pass through arbitrary accounts, use hash of pfm+sender+channel * lint * use types.ModuleName * update tests * better errors override packet fix incorrect bech32 update error desc refactor receiveFunds * update README * tidy errors * lint * chore: remove go.work * chore: lint --------- Co-authored-by: jtieri <[email protected]> (cherry picked from commit d72e83c) # Conflicts: # .gitignore # middleware/packet-forward-middleware/router/module_test.go # modules/ibc-hooks/client/cli/query.go # modules/ibc-hooks/hooks.go # modules/ibc-hooks/ibc_module.go # modules/ibc-hooks/ics4_middleware.go # modules/ibc-hooks/keeper/keeper.go # modules/ibc-hooks/sdkmodule.go # modules/ibc-hooks/simapp/ante.go # modules/ibc-hooks/simapp/app.go # modules/ibc-hooks/simapp/export.go # modules/ibc-hooks/simapp/simulation_test.go # modules/ibc-hooks/simapp/test_helpers.go # modules/ibc-hooks/tests/unit/mocks/isc4_middleware_mock.go # modules/ibc-hooks/tests/unit/module_test.go # modules/ibc-hooks/wasm_hook.go * fix conflict * fix merge --------- Co-authored-by: Andrew Gouin <[email protected]> Co-authored-by: Nicolas Lara <[email protected]>

…ugh (#79) * [pfm][fix] patch arbitrary account passthrough (#71) * don't allow funds to pass through arbitrary accounts, use hash of pfm+sender+channel * lint * use types.ModuleName * update tests * better errors override packet fix incorrect bech32 update error desc refactor receiveFunds * update README * tidy errors * lint * chore: remove go.work * chore: lint --------- Co-authored-by: jtieri <[email protected]> (cherry picked from commit d72e83c) # Conflicts: # .gitignore # middleware/packet-forward-middleware/router/module_test.go # modules/ibc-hooks/client/cli/query.go # modules/ibc-hooks/hooks.go # modules/ibc-hooks/ibc_module.go # modules/ibc-hooks/ics4_middleware.go # modules/ibc-hooks/keeper/keeper.go # modules/ibc-hooks/sdkmodule.go # modules/ibc-hooks/simapp/ante.go # modules/ibc-hooks/simapp/app.go # modules/ibc-hooks/simapp/export.go # modules/ibc-hooks/simapp/simulation_test.go # modules/ibc-hooks/simapp/test_helpers.go # modules/ibc-hooks/tests/unit/mocks/isc4_middleware_mock.go # modules/ibc-hooks/tests/unit/module_test.go # modules/ibc-hooks/wasm_hook.go * fix merge * chore: resolve conflicts --------- Co-authored-by: Andrew Gouin <[email protected]> Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: jtieri <[email protected]>

…ugh (#80) * [pfm][fix] patch arbitrary account passthrough (#71) * don't allow funds to pass through arbitrary accounts, use hash of pfm+sender+channel * lint * use types.ModuleName * update tests * better errors override packet fix incorrect bech32 update error desc refactor receiveFunds * update README * tidy errors * lint * chore: remove go.work * chore: lint --------- Co-authored-by: jtieri <[email protected]> (cherry picked from commit d72e83c) # Conflicts: # .gitignore # middleware/packet-forward-middleware/router/module_test.go # modules/ibc-hooks/client/cli/query.go # modules/ibc-hooks/hooks.go # modules/ibc-hooks/ibc_module.go # modules/ibc-hooks/ics4_middleware.go # modules/ibc-hooks/keeper/keeper.go # modules/ibc-hooks/sdkmodule.go # modules/ibc-hooks/simapp/ante.go # modules/ibc-hooks/simapp/app.go # modules/ibc-hooks/simapp/export.go # modules/ibc-hooks/simapp/simulation_test.go # modules/ibc-hooks/simapp/test_helpers.go # modules/ibc-hooks/tests/unit/mocks/isc4_middleware_mock.go # modules/ibc-hooks/tests/unit/module_test.go # modules/ibc-hooks/wasm_hook.go * fix merge * chore: resolve conflicts --------- Co-authored-by: Andrew Gouin <[email protected]> Co-authored-by: Nicolas Lara <[email protected]> Co-authored-by: jtieri <[email protected]>

## Context During liquid stake and forward, the autopilot "receiver" of the inbound transfer becomes the "sender" of the transfer back to the host. However, downstream applications shouldn't trust this new "sender" so we need to use a generated address instead. To be clear, using the original sender would not introduce an attack vector on Stride. However, it could introduce an attack vector on a different zone, _if_ they were to trust the sender. The hashed sender is used to make the assumption more explicit that new zones should not trust the address. This bug appeared in PFM (if more context is needed): * [Issue](cosmos/ibc-apps#68) * [PR fix](cosmos/ibc-apps#71) ## Design Considerations There wasn't an immediately obvious way to implement this. The complexity arises in that the address used for the inbound transfer doesn't always line up with the address used in the autopilot action. Additionally, if a bank send is required after the transfer (in the event that we transferred to an intermediate recipient), some scenarios would require us to build the IBC denom hash ourselves (not impossible, but adds a lot of unnecessary code). There didn't seem to be a clean way to refactor the callback such that we use a different receiver based on the action type (i.e. use "hashed" for LS&Forward, but "original" for Claim). As such, I think the only two options were as follows: 1. _Send inbound transfer to original receiver_ * ✅ **Claim**: No changes * ✅ **Liquid Stake**: No changes * ❌ **Liquid Stake and Forward**: Requires us to bank send to hashed receiver (but this is straightforward since the token is not an IBC denom) 2. _Send inbound transfer to hashed receiver_ * ❌❌ **Claim**: Would require a bank send to the original receiver (we'd need to copy over all the IBC hashing code from ibc-go in order to handle both native and non-native denoms) * ❌ **Liquid Stake**: Would require a bank send to the original receiver (but this is straightforward since the token is not an IBC denom) * ✅ **Liquid Stake and Forward**: No changes 3. _Decide recipient based on action_ * ✅ **Claim**: No changes * ✅ **Liquid Stake**: No changes * ✅ **Claim:** No changes * ❌ Requires some somewhat sloppy refactoring since the change must be upstream of the transfer ~I do think using the hashed receiver address as the inbound recipient and actor for each of the autopilot actions, does feel slightly more correct. However, I don't think it's a significant enough argument to justify the additional changes that would be required. Additionally, going with option 1 demands no changes to the existing autopilot functions that have been live on mainnet.~ After discussing offline, we decided to do option 3 and to simplify the autopilot schema so the refactor isn't as messy. This gives us the benefit of not having to worry about doing an extra bank send, but also not duplicating code in multiple places (see [here](#1038 (comment))). Option 3 is tracked in #1046 ## Brief Changelog * Added `GenerateHashedAddress` to generate the hashed address from the channel Id a previous sender (body of function taken from PFM) * Renamed `PacketForwardMetadata` to `AutopilotMetadata` * Added a bank send to the hashed address in LS and forward * Set the hashed address as the outbound sender during LS and forward * Restricted packets to being either autopilot or PFM (but not both) * Cleaned up the variable names to avoid confusion with all the data structures (open to better names though!) * Moved structs out of parser.go

agouin added 3 commits July 25, 2023 15:28

don't allow funds to pass through arbitrary accounts, use hash of pfm…

49099d0

…+sender+channel

lint

ea08528

use types.ModuleName

3ab53e8

update tests

ba57ba3

agouin marked this pull request as ready for review July 26, 2023 18:12

agouin mentioned this pull request Jul 26, 2023

e2e arbitrary passthrough fix strangelove-ventures/interchaintest#678

Closed

Merge branch 'main' into andrew/pfm_fix_arbitrary_passthrough

11e7bbb

agouin requested a review from jtieri July 26, 2023 20:25

javiersuweijie mentioned this pull request Jul 27, 2023

feat: use a generated intermediate address as the forwarder #72

Closed

nicolaslara reviewed Jul 27, 2023

View reviewed changes

jtieri reviewed Jul 27, 2023

View reviewed changes

agouin added 2 commits July 28, 2023 22:51

better errors

dd28df5

override packet fix incorrect bech32 update error desc refactor receiveFunds

update README

269f15c

agouin added 2 commits July 28, 2023 23:17

tidy errors

555a7fa

lint

7f9f94b

akc2267 mentioned this pull request Jul 31, 2023

PFM hook possible fixes when used along with IBC-hook? # #68

Closed

This was referenced Aug 1, 2023

apply fix to pfm when ready ComposableFi/picasso#213

Closed

handle memo according security fix of go middleware ComposableFi/composable-ibc#383

Closed

jtieri approved these changes Aug 2, 2023

View reviewed changes

jtieri added BACKPORT Backport into maintained branches packet-forward-middleware Label for items related to the packet forward middleware labels Aug 2, 2023

jtieri added 2 commits August 2, 2023 15:35

chore: remove go.work

c462083

chore: lint

bfddd58

jtieri merged commit d72e83c into main Aug 2, 2023
5 checks passed

jtieri deleted the andrew/pfm_fix_arbitrary_passthrough branch August 2, 2023 21:10

jtieri restored the andrew/pfm_fix_arbitrary_passthrough branch August 2, 2023 21:11

This was referenced Aug 2, 2023

[BP: release/v4 <- #71] [pfm][fix] patch arbitrary account passthrough #78

Merged

[BP: release/v5 <- #71] [pfm][fix] patch arbitrary account passthrough #79

Merged

[BP: release/v6 <- #71] [pfm][fix] patch arbitrary account passthrough #80

Merged

agouin deleted the andrew/pfm_fix_arbitrary_passthrough branch August 4, 2023 00:29

This was referenced Dec 22, 2023

autopilot hashed sender Stride-Labs/stride#1036

Closed

autopilot hashed sender Stride-Labs/stride#1038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pfm][fix] patch arbitrary account passthrough #71

[pfm][fix] patch arbitrary account passthrough #71

agouin commented Jul 25, 2023 •

edited

Loading

javiersuweijie commented Jul 26, 2023

agouin commented Jul 26, 2023

nicolaslara Jul 27, 2023

jtieri Jul 27, 2023

agouin Jul 27, 2023

jtieri left a comment

jtieri Jul 27, 2023

agouin commented Jul 29, 2023

[pfm][fix] patch arbitrary account passthrough #71

[pfm][fix] patch arbitrary account passthrough #71

Conversation

agouin commented Jul 25, 2023 • edited Loading

javiersuweijie commented Jul 26, 2023

agouin commented Jul 26, 2023

nicolaslara Jul 27, 2023

Choose a reason for hiding this comment

jtieri Jul 27, 2023

Choose a reason for hiding this comment

agouin Jul 27, 2023

Choose a reason for hiding this comment

jtieri left a comment

Choose a reason for hiding this comment

jtieri Jul 27, 2023

Choose a reason for hiding this comment

agouin commented Jul 29, 2023

agouin commented Jul 25, 2023 •

edited

Loading