remove stride address

x/autopilot/keeper/airdrop.go

@@ -20,7 +20,7 @@ import (
 func (k Keeper) TryUpdateAirdropClaim(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
-	data transfertypes.FungibleTokenPacketData,
+	fungibleTokenPacketData transfertypes.FungibleTokenPacketData,
 	packetMetadata types.ClaimPacketMetadata,
 ) error {
 	params := k.GetParams(ctx)
@@ -38,12 +38,20 @@ func (k Keeper) TryUpdateAirdropClaim(
 			"host zone not found for transfer channel %s", packet.GetDestChannel())
 	}
 
-	// grab relevant addresses
-	senderStrideAddress := utils.ConvertAddressToStrideAddress(data.Sender)
-	if senderStrideAddress == "" {
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, fmt.Sprintf("invalid sender address (%s)", data.Sender))
+	// By getting senderAddressConvertedToStrideAddress, we can check if the sender of this packet
+	// has a corresponding address on Stride that owns claim records.
+	// Importantly, this functionality is gated by chains - i.e. packets that come across the canonical Evmos <> Stride
+	// can update claim records associated with _Evmos_ only.
+	// This prevents malicious IBC channels from updating claim records (in essence, Stride trusts that the sender address on
+	// certain IBC channels triggered the transaction that triggered the IBC transfer).
+	// NOTE: An older version of PFM broke this assumption, but this functionality is only turned on for Evmos (has new PFM)
+	// and Injective (has no PFM).
+	senderAddressConvertedToStrideAddress := utils.ConvertAddressToStrideAddress(fungibleTokenPacketData.Sender)
+	if senderAddressConvertedToStrideAddress == "" {
+		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, fmt.Sprintf("invalid sender address (%s)", fungibleTokenPacketData.Sender))
 	}
-	newStrideAddress := packetMetadata.StrideAddress
+	// This is the updated owner of a claim record.
+	targetStrideAddressForClaimRecord := fungibleTokenPacketData.Receiver
 
 	// find the airdrop for this host chain ID
 	airdrop, found := k.claimKeeper.GetAirdropByChainId(ctx, hostZone.ChainId)
@@ -55,8 +63,8 @@ func (k Keeper) TryUpdateAirdropClaim(
 	}
 
 	airdropId := airdrop.AirdropIdentifier
-	k.Logger(ctx).Info(fmt.Sprintf("updating airdrop address %s (orig %s) to %s for airdrop %s",
-		senderStrideAddress, data.Sender, newStrideAddress, airdropId))
+	k.Logger(ctx).Info(fmt.Sprintf("updating airdrop address %s (senderAddressConvertedToStrideAddress %s) to %s for airdrop %s",
+		senderAddressConvertedToStrideAddress, fungibleTokenPacketData.Sender, targetStrideAddressForClaimRecord, airdropId))
 
-	return k.claimKeeper.UpdateAirdropAddress(ctx, senderStrideAddress, newStrideAddress, airdropId)
+	return k.claimKeeper.UpdateAirdropAddress(ctx, senderAddressConvertedToStrideAddress, targetStrideAddressForClaimRecord, airdropId)
 }

x/autopilot/keeper/liquidstake.go

@@ -19,7 +19,7 @@ import (
 func (k Keeper) TryLiquidStaking(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
-	newData transfertypes.FungibleTokenPacketData,
+	fungibleTokenPacketData transfertypes.FungibleTokenPacketData,
 	packetMetadata types.StakeibcPacketMetadata,
 ) error {
 	params := k.GetParams(ctx)
@@ -28,19 +28,19 @@ func (k Keeper) TryLiquidStaking(
 	}
 
 	// In this case, we can't process a liquid staking transaction, because we're dealing with STRD tokens
-	if transfertypes.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), newData.Denom) {
+	if transfertypes.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), fungibleTokenPacketData.Denom) {
 		return errors.New("the native token is not supported for liquid staking")
 	}
 
-	amount, ok := sdk.NewIntFromString(newData.Amount)
+	amount, ok := sdk.NewIntFromString(fungibleTokenPacketData.Amount)
 	if !ok {
 		return errors.New("not a parsable amount field")
 	}
 
-	// Note: newData.denom is base denom e.g. uatom - not ibc/xxx
-	var token = sdk.NewCoin(newData.Denom, amount)
+	// Note: fungibleTokenPacketData.denom is base denom e.g. uatom - not ibc/xxx
+	var token = sdk.NewCoin(fungibleTokenPacketData.Denom, amount)
 
-	prefixedDenom := transfertypes.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) + newData.Denom
+	prefixedDenom := transfertypes.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) + fungibleTokenPacketData.Denom
 	ibcDenom := transfertypes.ParseDenomTrace(prefixedDenom).IBCDenom()
 
 	hostZone, err := k.stakeibcKeeper.GetHostZoneFromHostDenom(ctx, token.Denom)
@@ -52,12 +52,16 @@ func (k Keeper) TryLiquidStaking(
 		return fmt.Errorf("ibc denom %s is not equal to host zone ibc denom %s", ibcDenom, hostZone.IbcDenom)
 	}
 
-	strideAddress, err := sdk.AccAddressFromBech32(packetMetadata.StrideAddress)
+	// fungibleTokenPacketData.Receiver is either the ICS-20 receiver field, or it's parsed from the memo
+	// in the receiver field
+	// Importantly, this is also the address that receives tokens (fungibleTokenPacketData.Amount above)
+	// That means, anyone can send tokens to any address, and cause that address to liquid stake them
+	liquidStakingAddress, err := sdk.AccAddressFromBech32(fungibleTokenPacketData.Receiver)
 	if err != nil {
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid stride_address (%s) in autopilot memo", strideAddress)
+		return errorsmod.Wrapf(sdkerrors.ErrInvalidAddress, "invalid stride_address (%s) in autopilot memo", liquidStakingAddress)
 	}
 
-	return k.RunLiquidStake(ctx, strideAddress, token)
+	return k.RunLiquidStake(ctx, liquidStakingAddress, token)
 }
 
 func (k Keeper) RunLiquidStake(ctx sdk.Context, addr sdk.AccAddress, token sdk.Coin) error {

x/autopilot/module_ibc.go

@@ -125,33 +125,34 @@ func (im IBCModule) OnRecvPacket(
 		packet.Sequence, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel))
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
-	var data transfertypes.FungibleTokenPacketData
-	if err := transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
+	var fungibleTokenPacketData transfertypes.FungibleTokenPacketData
+	if err := transfertypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &fungibleTokenPacketData); err != nil {
 		return channeltypes.NewErrorAcknowledgement(err)
 	}
 
 	// Error any transactions with a Memo or Receiver field are greater than the max characters
-	if len(data.Memo) > MaxMemoCharLength {
-		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(types.ErrInvalidMemoSize, "memo length: %d", len(data.Memo)))
+	if len(fungibleTokenPacketData.Memo) > MaxMemoCharLength {
+		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(types.ErrInvalidMemoSize, "memo length: %d", len(fungibleTokenPacketData.Memo)))
 	}
-	if len(data.Receiver) > MaxMemoCharLength {
-		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(types.ErrInvalidMemoSize, "receiver length: %d", len(data.Receiver)))
+	if len(fungibleTokenPacketData.Receiver) > MaxMemoCharLength {
+		return channeltypes.NewErrorAcknowledgement(errorsmod.Wrapf(types.ErrInvalidMemoSize, "receiver length: %d", len(fungibleTokenPacketData.Receiver)))
 	}
 
 	// ibc-go v5 has a Memo field that can store forwarding info
 	// For older version of ibc-go, the data must be stored in the receiver field
+	// TODO: can we remove this backwards compatibility?
 	var metadata string
-	if data.Memo != "" { // ibc-go v5+
-		metadata = data.Memo
+	if fungibleTokenPacketData.Memo != "" { // ibc-go v5+
+		metadata = fungibleTokenPacketData.Memo
 	} else { // before ibc-go v5
-		metadata = data.Receiver
+		metadata = fungibleTokenPacketData.Receiver
 	}
 
 	// If a valid receiver address has been provided and no memo,
 	// this is clearly just an normal IBC transfer
 	// Pass down the stack immediately instead of parsing
-	_, err := sdk.AccAddressFromBech32(data.Receiver)
-	if err == nil && data.Memo == "" {
+	_, err := sdk.AccAddressFromBech32(fungibleTokenPacketData.Receiver)
+	if err == nil && fungibleTokenPacketData.Memo == "" {
 		return im.app.OnRecvPacket(ctx, packet, relayer)
 	}
 
@@ -167,11 +168,11 @@ func (im IBCModule) OnRecvPacket(
 		return im.app.OnRecvPacket(ctx, packet, relayer)
 	}
 
-	// Modify the packet data by replacing the JSON metadata field with a receiver address
+	// Modify the packet fungibleTokenPacketData by replacing the JSON metadata field with a receiver address
 	// to allow the packet to continue down the stack
-	newData := data
-	newData.Receiver = packetForwardMetadata.Receiver
-	bz, err := transfertypes.ModuleCdc.MarshalJSON(&newData)
+	replacedReceiverFungibleTokenPacketData := fungibleTokenPacketData
+	replacedReceiverFungibleTokenPacketData.Receiver = packetForwardMetadata.Receiver
+	bz, err := transfertypes.ModuleCdc.MarshalJSON(&replacedReceiverFungibleTokenPacketData)
 	if err != nil {
 		return channeltypes.NewErrorAcknowledgement(err)
 	}
@@ -191,14 +192,14 @@ func (im IBCModule) OnRecvPacket(
 	case types.StakeibcPacketMetadata:
 		// If stakeibc routing is inactive (but the packet had routing info in the memo) return an ack error
 		if !autopilotParams.StakeibcActive {
-			im.keeper.Logger(ctx).Error(fmt.Sprintf("Packet from %s had stakeibc routing info but autopilot stakeibc routing is disabled", newData.Sender))
+			im.keeper.Logger(ctx).Error(fmt.Sprintf("Packet from %s had stakeibc routing info but autopilot stakeibc routing is disabled", replacedReceiverFungibleTokenPacketData.Sender))
 			return channeltypes.NewErrorAcknowledgement(types.ErrPacketForwardingInactive)
 		}
-		im.keeper.Logger(ctx).Info(fmt.Sprintf("Forwaring packet from %s to stakeibc", newData.Sender))
+		im.keeper.Logger(ctx).Info(fmt.Sprintf("Forwaring packet from %s to stakeibc", replacedReceiverFungibleTokenPacketData.Sender))
 
 		// Try to liquid stake - return an ack error if it fails, otherwise return the ack generated from the earlier packet propogation
-		if err := im.keeper.TryLiquidStaking(ctx, packet, newData, routingInfo); err != nil {
-			im.keeper.Logger(ctx).Error(fmt.Sprintf("Error liquid staking packet from autopilot for %s: %s", newData.Sender, err.Error()))
+		if err := im.keeper.TryLiquidStaking(ctx, packet, replacedReceiverFungibleTokenPacketData, routingInfo); err != nil {
+			im.keeper.Logger(ctx).Error(fmt.Sprintf("Error liquid staking packet from autopilot for %s: %s", replacedReceiverFungibleTokenPacketData.Sender, err.Error()))
 			return channeltypes.NewErrorAcknowledgement(err)
 		}
 
@@ -207,13 +208,13 @@ func (im IBCModule) OnRecvPacket(
 	case types.ClaimPacketMetadata:
 		// If claim routing is inactive (but the packet had routing info in the memo) return an ack error
 		if !autopilotParams.ClaimActive {
-			im.keeper.Logger(ctx).Error(fmt.Sprintf("Packet from %s had claim routing info but autopilot claim routing is disabled", newData.Sender))
+			im.keeper.Logger(ctx).Error(fmt.Sprintf("Packet from %s had claim routing info but autopilot claim routing is disabled", replacedReceiverFungibleTokenPacketData.Sender))
 			return channeltypes.NewErrorAcknowledgement(types.ErrPacketForwardingInactive)
 		}
-		im.keeper.Logger(ctx).Info(fmt.Sprintf("Forwaring packet from %s to claim", newData.Sender))
+		im.keeper.Logger(ctx).Info(fmt.Sprintf("Forwaring packet from %s to claim", replacedReceiverFungibleTokenPacketData.Sender))
 
-		if err := im.keeper.TryUpdateAirdropClaim(ctx, packet, newData, routingInfo); err != nil {
-			im.keeper.Logger(ctx).Error(fmt.Sprintf("Error updating airdrop claim from autopilot for %s: %s", newData.Sender, err.Error()))
+		if err := im.keeper.TryUpdateAirdropClaim(ctx, packet, replacedReceiverFungibleTokenPacketData, routingInfo); err != nil {
+			im.keeper.Logger(ctx).Error(fmt.Sprintf("Error updating airdrop claim from autopilot for %s: %s", replacedReceiverFungibleTokenPacketData.Sender, err.Error()))
 			return channeltypes.NewErrorAcknowledgement(err)
 		}
 

x/autopilot/types/parser.go

@@ -25,38 +25,58 @@ type ModuleRoutingInfo interface {
 	Validate() error
 }
 
+// NOTE: I removed StakeibcPacketMetadata from StakeibcPacketMetadata/ClaimPacketMetadata because it was
+// just being set to the receiver.
+// I'm not sure this was necessary for airdrop linking, but it was necessary for liquid stakes, because the
+// address that liquid stakes _must_ be the address that received tokens.
+// A cleaner design here would be to do something like PFM, which is
+// liquid_staking_address = hash(channel, sender)
+// This removes the need for a receiving address.
+// Tokens can be liquid staked via this protocol-owned address, which more clearly separates
+// concerns (random addresses can't receive tokens, then be forced to liquid stake, which is the case today).
+// The above isn't a big deal at face value - if you want to send someone an LST you can also do it using the bank module.
+// For liquid stake, this is straightforward - we can just use a bank send after the tokens have been liquid staked.
+
+// For liquid stake and forward, it is slightly more challenging, because if the IBC transfer fails, a fallback address on Stride
+// is required (and this might not happen immediately - it could happen in a timeout). Unfortunately, liquid stake and forward
+// re-introduces the PFM bug, because senders can no longer be trusted. Consider the following case:
+// - A on Evmos sends 10 EVMOS to C on Stride, to be forwarded to B on Osmosis
+// - 10 EVMOS -> 10 stEVMOS via B on Stride, IBC transferred to C on Osmosis with sender B
+// - solution: enforce B is the same address as A, so A = B = C and the sender is trusted
+// - drawback: doesn't work for chains with a different address derivition - the fallback address B
+// 				might not be accessible (low confidence)
+
+// Possible solutions
+// (1) Just make the sender hash(channel, sender) so it's unusable on the destination chain
+// 		pros: PFM-like solution, proven in prod
+// 		cons: more complex since we need a fallback if the IBC transfer fails
+// (2) Constrain the forwarding logic
+// 		(1) tokens can only be sent to/from Evmos on the canonical channel
+// 		(2) the receiver on Stride is overridden and set to the mechanical Stride address
+// 			- what if the IBC times out and the user needs to manually claim their tokens?
+// 			- we could design a retry mechanism (tx on Stride that anyone can call)
+
 // Packet metadata info specific to Stakeibc (e.g. 1-click liquid staking)
 type StakeibcPacketMetadata struct {
-	Action        string `json:"action"`
-	StrideAddress string
+	Action string `json:"action"`
 }
 
 // Packet metadata info specific to Claim (e.g. airdrops for non-118 coins)
 type ClaimPacketMetadata struct {
-	StrideAddress string
 }
 
 // Validate stakeibc packet metadata fields
 // including the stride address and action type
 func (m StakeibcPacketMetadata) Validate() error {
-	_, err := sdk.AccAddressFromBech32(m.StrideAddress)
-	if err != nil {
-		return err
-	}
 	if m.Action != "LiquidStake" {
 		return errorsmod.Wrapf(ErrUnsupportedStakeibcAction, "action %s is not supported", m.Action)
 	}
 
 	return nil
 }
 
-// Validate claim packet metadata includes the stride address
+// Should we remove this struct?
 func (m ClaimPacketMetadata) Validate() error {
-	_, err := sdk.AccAddressFromBech32(m.StrideAddress)
-	if err != nil {
-		return err
-	}
-
 	return nil
 }
 
@@ -89,13 +109,11 @@ func ParsePacketMetadata(metadata string) (*PacketForwardMetadata, error) {
 	var routingInfo ModuleRoutingInfo
 	if raw.Autopilot.Stakeibc != nil {
 		// override the stride address with the receiver address
-		raw.Autopilot.Stakeibc.StrideAddress = raw.Autopilot.Receiver
 		moduleCount++
 		routingInfo = *raw.Autopilot.Stakeibc
 	}
 	if raw.Autopilot.Claim != nil {
 		// override the stride address with the receiver address
-		raw.Autopilot.Claim.StrideAddress = raw.Autopilot.Receiver
 		moduleCount++
 		routingInfo = *raw.Autopilot.Claim
 	}