PFM hook possible fixes when used along with IBC-hook? # #68
Comments
|
PR to address this issue: #71 |
This was referenced Dec 22, 2023
mergify bot
pushed a commit
to Stride-Labs/stride
that referenced
this issue
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
Hello, what is planned fix for https://github.com/SCV-Security/responsible-disclosures/blob/main/SCV%20-%20Responsible%20Disclosure%20-%20PFM%20and%20IBC-hooks.pdf ?
I though of all fixes I can do in contract with all data available, the only fix CW developer can do is signed token inside packet, but without ZK this is dead end for IBCing high gas cost chains with different signature schemes.
Planned fix would help to write code the way anticipating that fix to happen.
Thank you
The text was updated successfully, but these errors were encountered: