autopilot hashed sender option (3) #1046
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
riley-stride
reviewed
Jan 8, 2024
asalzmann
reviewed
Jan 9, 2024
asalzmann
approved these changes
Jan 9, 2024
mergify bot
pushed a commit
that referenced
this pull request
Jan 10, 2024
## 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implemented option 3 from the discussion in #1038 where, for LS and forwards, we update the receiver before passing it down the stack