Skip to content
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 #1036

Closed
wants to merge 6 commits into from

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Dec 22, 2023

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.

We send the packet down the stack first to complete the inbound transfer, this means there's a few ways we could implement this:

Liquid Stake (no forwarding)
    1. Inbound transfer to ORIGINAL     
    2. Liquid stake with ORIGINAL   
    
              OR               
                                                  
   1. Inbound transfer to HASHED
   2. Liquid stake with HASHED
   3. Bank send to ORIGINAL

Liquid Stake (with forwarding)
   1. Inbound transfer to HASHED
   4. Liquid stake with HASHED
   5. Outbound transfer from HASHED

Redeem Stake / Claim
  1. Inbound transfer to ORIGINAL       
  
             OR     

  1. Inbound transfer to HASHED
  2. Bank send to ORIGINAL

Two additional constraints (not that these were impossible, but I think they would have made the code hard to follow)

  • There didn't seem to be a clean way to switch between ORIGINAL/HASHED in the inbound transfer (i.e. we have to pick one of the two before calling down the stack)
  • There didn't seem to be a clean way to use different inbound transfer addresses across LiquidStake and LiquidStake&Forward (i.e. we couldn't easily use ORIGINAL for LS, and HASHED for LS&Forward)

All that said, I opted with the approach of doing the inbound transfer with the hashed address and doing a bank send later if needed. Very open to a better way of doing this though if you can think of one!

High Level Design

  • Added a new data structure (AutopilotTransferMetadata) to replace FungibleTokenPacketData and include the hashed sender
  • This data structure was sent to each action so the relevant addresses could be retrieved as needed

Brief Changelog

  • Added GenerateHashedReceiver to generate the hashed address (body of function taken from PFM)
  • Added AutopilotTransferMetadata which stores the original packet data + the hashed sender
  • Renamed PacketForwardMetadata to AutopilotActionMetadata
  • Passed AutopilotTransferMetadata into each action's function instead of FungibleTokenPacketData
  • Used hashed address for inbound transfer and added a bank send to the original sender afterwards (if needed)
  • 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!)

@sampocs sampocs added the v17 label Dec 22, 2023
@sampocs sampocs marked this pull request as draft December 22, 2023 20:57
@sampocs
Copy link
Collaborator Author

sampocs commented Dec 27, 2023

disregard this PR - it grew a bit to complex. opting for a different approach in #1038

@sampocs sampocs closed this Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant