diff --git a/app/ibc-hooks/README.md b/app/ibc-hooks/README.md index 7984ee5..aab69ff 100644 --- a/app/ibc-hooks/README.md +++ b/app/ibc-hooks/README.md @@ -29,26 +29,21 @@ type HookData struct { } type MsgExecuteContract struct { - // Sender is the actor that committed the message in the sender chain - Sender string // Contract is the address of the smart contract Contract string // Msg json encoded message to be passed to the contract Msg RawContractMessage - // Funds coins that are transferred to the contract on execution - Funds sdk.Coins } ``` So we detail where we want to get each of these fields from: * Sender: We cannot trust the sender of an IBC packet, the counterparty chain has full ability to lie about it. -We cannot risk this sender being confused for a particular user or module address on Osmosis. +We cannot risk this sender being confused for a particular user or module address on the chain. So we replace the sender with an account to represent the sender prefixed by the channel and a wasm module prefix. This is done by setting the sender to `Bech32(Hash("ibc-wasm-hook-intermediary" || channelID || sender))`, where the channelId is the channel id on the local chain. * Contract: This field should be directly obtained from the ICS-20 packet metadata * Msg: This field should be directly obtained from the ICS-20 packet metadata. -* Funds: This field is set to the amount of funds being sent over in the ICS 20 packet. One detail is that the denom in the packet is the counterparty chains representation of the denom, so we have to translate it to Osmosis' representation. > **_WARNING:_** Due to a [bug](https://twitter.com/SCVSecurity/status/1682329758020022272) in the packet forward middleware, we cannot trust the sender from chains that use PFM. Until that is fixed, we recommend chains to not trust the sender on contracts executed via IBC hooks. @@ -56,14 +51,10 @@ So our constructed cosmwasm message that we execute will look like: ```go msg := MsgExecuteContract{ - // Sender is the that actor that signed the messages - Sender: "init1-hash-of-channel-and-sender", // Contract is the address of the smart contract Contract: packet.data.memo["wasm"]["contract"], // Msg json encoded message to be passed to the contract Msg: packet.data.memo["wasm"]["msg"], - // Funds coins that are transferred to the contract on execution - Funds: sdk.NewCoin{Denom: ibc.ConvertSenderDenomToLocalDenom(packet.data.Denom), Amount: packet.data.Amount} } ``` @@ -82,13 +73,12 @@ ICS20 is JSON native, so we use JSON for the memo format. "receiver": "contract addr or blank", "memo": { "wasm": { - "contract": "init1contractAddr", - "msg": { - "raw_message_fields": "raw_message_data", - }, - "funds": [ - {"denom": "ibc/denom", "amount": "100"} - ] + "message": { + "contract": "init1contractAddr", + "msg": { + "raw_message_fields": "raw_message_data", + } + } } } } @@ -100,7 +90,7 @@ An ICS20 packet is formatted correctly for wasmhooks iff the following all hold: * `memo` is not blank * `memo` is valid JSON * `memo` has at least one key, with value `"wasm"` -* `memo["wasm"]["message"]` has exactly two entries, `"contract"`, `"msg"` and `"fund"` +* `memo["wasm"]["message"]` has exactly two entries, `"contract"` and `"msg"` * `memo["wasm"]["message"]["msg"]` is a valid JSON object * `receiver == "" || receiver == memo["wasm"]["contract"]` diff --git a/app/ibc-hooks/receive.go b/app/ibc-hooks/receive.go index 0a5df5b..13fecb1 100644 --- a/app/ibc-hooks/receive.go +++ b/app/ibc-hooks/receive.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" + "cosmossdk.io/math" sdk "github.com/cosmos/cosmos-sdk/types" transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" @@ -44,7 +45,7 @@ func (h WasmHooks) onRecvIcs20Packet( } // Calculate the receiver / contract caller based on the packet's channel and sender - intermediateSender := deriveIntermediateSender(packet.GetDestChannel(), data.GetSender()) + intermediateSender := DeriveIntermediateSender(packet.GetDestChannel(), data.GetSender()) // The funds sent on this packet need to be transferred to the intermediary account for the sender. // For this, we override the ICS20 packet's Receiver (essentially hijacking the funds to this new address) @@ -64,7 +65,15 @@ func (h WasmHooks) onRecvIcs20Packet( return ack } + // Extract the denom and amount from the packet data + denom := MustExtractDenomFromPacketOnRecv(packet) + amount, ok := math.NewIntFromString(data.GetAmount()) + if !ok { + return newEmitErrorAcknowledgement(fmt.Errorf("invalid amount: %s", data.GetAmount())) + } + msg.Sender = intermediateSender + msg.Funds = sdk.NewCoins(sdk.NewCoin(denom, amount)) _, err = h.execMsg(ctx, msg) if err != nil { return newEmitErrorAcknowledgement(err) @@ -100,7 +109,7 @@ func (h WasmHooks) onRecvIcs721Packet( } // Calculate the receiver / contract caller based on the packet's channel and sender - intermediateSender := deriveIntermediateSender(packet.GetDestChannel(), data.GetSender()) + intermediateSender := DeriveIntermediateSender(packet.GetDestChannel(), data.GetSender()) // The funds sent on this packet need to be transferred to the intermediary account for the sender. // For this, we override the ICS721 packet's Receiver (essentially hijacking the funds to this new address) @@ -121,6 +130,7 @@ func (h WasmHooks) onRecvIcs721Packet( } msg.Sender = intermediateSender + msg.Funds = sdk.NewCoins() _, err = h.execMsg(ctx, msg) if err != nil { return newEmitErrorAcknowledgement(err) diff --git a/app/ibc-hooks/receive_test.go b/app/ibc-hooks/receive_test.go index e3a9f4a..d65108f 100644 --- a/app/ibc-hooks/receive_test.go +++ b/app/ibc-hooks/receive_test.go @@ -6,12 +6,15 @@ import ( "os" "testing" + "cosmossdk.io/math" "github.com/stretchr/testify/require" sdk "github.com/cosmos/cosmos-sdk/types" transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + nfttransfertypes "github.com/initia-labs/initia/x/ibc/nft-transfer/types" + ibchooks "github.com/initia-labs/miniwasm/app/ibc-hooks" wasmkeeper "github.com/CosmWasm/wasmd/x/wasm/keeper" wasmtypes "github.com/CosmWasm/wasmd/x/wasm/types" @@ -83,9 +86,21 @@ func Test_onReceivePacket_memo(t *testing.T) { dataBz, err := json.Marshal(&data) require.NoError(t, err) + // funds foo coins to the intermediate sender + intermediateSender, err := sdk.AccAddressFromBech32(ibchooks.DeriveIntermediateSender("channel-0", data.GetSender())) + require.NoError(t, err) + denom := ibchooks.MustExtractDenomFromPacketOnRecv(channeltypes.Packet{ + Data: dataBz, + DestinationPort: "wasm", + DestinationChannel: "channel-0", + }) + input.Faucet.Fund(ctx, intermediateSender, sdk.NewCoin(denom, math.NewInt(10000))) + // failed to due to acl ack := input.IBCHooksMiddleware.OnRecvPacket(ctx, channeltypes.Packet{ - Data: dataBz, + Data: dataBz, + DestinationPort: "wasm", + DestinationChannel: "channel-0", }, addr) require.False(t, ack.Success()) @@ -96,8 +111,11 @@ func Test_onReceivePacket_memo(t *testing.T) { // success ack = input.IBCHooksMiddleware.OnRecvPacket(ctx, channeltypes.Packet{ - Data: dataBz, + Data: dataBz, + DestinationPort: "wasm", + DestinationChannel: "channel-0", }, addr) + fmt.Println(string(ack.Acknowledgement())) require.True(t, ack.Success()) // check the contract state diff --git a/app/ibc-hooks/util.go b/app/ibc-hooks/util.go index f29d4c8..a3c68c1 100644 --- a/app/ibc-hooks/util.go +++ b/app/ibc-hooks/util.go @@ -12,6 +12,7 @@ import ( transfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" nfttransfertypes "github.com/initia-labs/initia/x/ibc/nft-transfer/types" @@ -20,9 +21,9 @@ import ( const senderPrefix = "ibc-wasm-hook-intermediary" -// deriveIntermediateSender compute intermediate sender address +// DeriveIntermediateSender compute intermediate sender address // Bech32(Hash(Hash("ibc-hook-intermediary") + channelID/sender)) -func deriveIntermediateSender(channel, originalSender string) string { +func DeriveIntermediateSender(channel, originalSender string) string { senderStr := fmt.Sprintf("%s/%s", channel, originalSender) senderAddr := sdk.AccAddress(address.Hash(senderPrefix, []byte(senderStr))) return senderAddr.String() @@ -134,3 +135,35 @@ func isAckError(appCodec codec.Codec, acknowledgement []byte) bool { return false } + +// MustExtractDenomFromPacketOnRecv takes a packet with a valid ICS20 token data in the Data field and returns the +// denom as represented in the local chain. +// If the data cannot be unmarshalled this function will panic +func MustExtractDenomFromPacketOnRecv(packet ibcexported.PacketI) string { + var data transfertypes.FungibleTokenPacketData + if err := json.Unmarshal(packet.GetData(), &data); err != nil { + panic("unable to unmarshal ICS20 packet data") + } + + var denom string + if transfertypes.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), data.Denom) { + // remove prefix added by sender chain + voucherPrefix := transfertypes.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) + + unprefixedDenom := data.Denom[len(voucherPrefix):] + + // coin denomination used in sending from the escrow address + denom = unprefixedDenom + + // The denomination used to send the coins is either the native denom or the hash of the path + // if the denomination is not native. + denomTrace := transfertypes.ParseDenomTrace(unprefixedDenom) + if denomTrace.Path != "" { + denom = denomTrace.IBCDenom() + } + } else { + prefixedDenom := transfertypes.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) + data.Denom + denom = transfertypes.ParseDenomTrace(prefixedDenom).IBCDenom() + } + return denom +}