feat: enforce intermediate forwarder

[javiersuweijie]

This PR aims to introduce a potential workaround for the issue disclosed by SCV here

The main issue is that the packet forwarding middleware allows a user to use any address as the forwarder. This allows malicious users to specify an address that they do not control as an intermediate hop which could impact downstream modules that rely on the IBC packet sender for authentication. (e.g. IBC-hooks)

This change forces the intermediate forwarder to be the IBC hooks module account so that there is no way to spoof any addresses. Semantically, this also makes sense since we are requesting the PFM to forward for us and the receiver should see that the packet came from the module.

cc: @jackzampolin

[javiersuweijie]

cc: @jtieri too

[nicolaslara]

Thanks for providing this!

I think the idea of enforcing a forwarder address is good, but I don't think it should be the module address. When we set it to the module address, all forwarded tokens will be received from the same sender, which removes the information about the original sender. Instead I think we should do something like:

func DeriveIntermediateAddress(port, channel, originalSender string) (string, error) {
       senderStr := fmt.Sprintf("%s/%s/%s", port, channel, originalSender)
       senderHash32 := address.Hash(types.IntermediateAddrPrefix, []byte(senderStr))
       addr := sdk.AccAddress(senderHash32[:])
       bech32Prefix := sdk.GetConfig().GetBech32AccountAddrPrefix()
       return sdk.Bech32ifyAddressBytes(bech32Prefix, addr)
}

This gives us a bech32 addr that is unique to the path taken and allows the receiver to verify its validity if provided in the message (i.e.: if the message includes the original sender and expected path, the receiver can hash it and verify that it is, indeed, the path the packet took from the original sender).

[javiersuweijie]

@nicolaslara Agree with your take that we lose all information of the initial sender and that's undesirable. The only way to get the actual sender is to perform a look-up using the IBC packet sequence which is clunky.

Was toying with your idea as well but the main issue that I was struggling with is how the initial IBC transfer message should look like:

transfertypes.FungibleTokenPacketData{
		Denom:    "uosmo",
		Amount:   "1000000",
		Receiver:  "<any random address?>",
	}

If we generate an intermediate address, then we are ignoring the receiver in the initial fungible token packet by replacing it with a generated address. It might also be slightly more complex to move the token ownership from the Receiver to the new generated address so that the transfer module knows where to take the tokens from when forwarding the packet.

Furthermore, with this approach, without knowing the sender beforehand, we still need to do a look-up with the IBC packet sequence to find the original sender.

[nicolaslara]

Yeah, I think we should just ignore (or better yet, get rid of) the receiver in the intermediate steps. On the sender, the receiver can be derived without needing to query any new information (just hash the string and bech32 encode it). We have a utility function for that on the osmosisd cli.

I agree it's odd that you either have to do that (if we enforce it should match) or have any random address that gets discarded, but that's what is happening on this implementation as well, right? the receiver is replaced by the module addr.

Furthermore, with this approach, without knowing the sender beforehand, we still need to do a look-up with the IBC packet sequence to find the original sender.

I don't think so, the sender is included in the ics20 packet

[javiersuweijie]

that's what is happening on this implementation as well, right? the receiver is replaced by the module addr.

This implementation enforces that the receiver must be the module account else it fails the transfer.
It will look like Sender -> Module -> Receiver

I don't think so, the sender is included in the ics20 packet

What I meant was the sender would be the result of bech32(fmt.Sprintf("%s/%s/%s", port, channel, originalSender)) and without knowing the originalSender on the destination chain, the recipient will still need to check on the intermediate chain to verify the sender.

Actually, both solutions have some drawbacks. Should PFM be implemented as a separate message altogether?

types.ForwardFungibleTokenPacketData{
		Denom:    "uosmo",
		Amount:   "1000000",
		Next: {...}
	}

Then under the hood, it will use the transfer module to handle the logic and generate the intermediate address using the sender address like you mentioned. Feel like this is the cleanest approach.

[nicolaslara]

Discussed this with Jack at nebular, and realized the issue with getting rid of the intermediate receiver field is that if the intermediate chain doesn't have PFM, then there is no fallback address for users to recover the funds. I think the right solution here is to keep the receiver field but use it as a fallback address. If we fail to forward, then the funds are left on the receiver addr. What do you think about that @javiersuweijie ?

[javiersuweijie]

Closing this in favour of: #72