-
Notifications
You must be signed in to change notification settings - Fork 208
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
autopilot hashed sender #1038
autopilot hashed sender #1038
Conversation
0dd0b5c
to
1214072
Compare
1214072
to
2c22d17
Compare
Just talked this one through with Sam and think I agree that (1) is a good approach. My original intuition would have been to opt for approach (3): on the inbound IBC transfer, use a different receiver based on the action type (i.e. use "hashed" for LS&Forward, but "original" for LS and for Claim). Was thinking that at the stage of the IBC middleware stack where the transfer occurs, we could check the memo and decide whether to transfer to "hashed" or "original" at that stage. But it sounds like that might require some refactoring. And (1) is easy to think about because it puts all the LS&Forward logic in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you so much for doing this! Very meticulous, and a good solution.
I left a couple of small clarifying questions, but should hopefully be quick! All signed off otherwise
This would have been my intuition as well - what refactoring would it require? (reviewing the current approach independent of this) |
I now remember why I thought option (3) would be messy. it's because we call the same TryLiquidStake function regardless of whether we're doing just LS, or LS&Forward. Which means we need a clunky switch statement to check whether we want to use the hashed receiver or original receiver. I implemented option (3) [here] to make things more concrete. (Funny enough option (1) and (3) are the exact same number of LOC 😆 ). I personally think option (1) is a bit cleaner than (3) because we only have action-specific stuff in one place instead of two. And more importantly, we don't duplicate logic between the two [ex: here and here]. That said, I'm still open to option (3) if that's your preference! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeout looks good to me.
should we add our rationale for it into the comments? so that if we ever want to change it in the future that context isn't lost.
Great idea! Updated here |
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):
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:
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).
Option 3 is tracked in #1046
Brief Changelog
GenerateHashedAddress
to generate the hashed address from the channel Id a previous sender (body of function taken from PFM)PacketForwardMetadata
toAutopilotMetadata