Use a transfer module account as an intermediary address for forwards

[DimitrisJim]

Reference code:

ibc-go/modules/apps/transfer/types/keys.go

Lines 86 to 97 in efcfa5d

	func GetForwardAddress(portID, channelID string) sdk.AccAddress {
	// a slash is used to create domain separation between port and channel identifiers to
	// prevent address collisions between escrow addresses created for different channels
	contents := fmt.Sprintf("%s/%s", portID, channelID)
	// ADR 028 AddressHash construction
	preImage := []byte(forwardAddressVersion)
	preImage = append(preImage, 0)
	preImage = append(preImage, contents...)
	hash := sha256.Sum256(preImage)
	return hash[:20]
	}

Surfaced during internal walkthrough of ics20 v2 forwarding (commit efcfa5d)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[DimitrisJim]

adding needs discussion here, we have an either or we should take care of deciding on (probably requiring some investigation)

[srdtrk]

I recommend we switch to a module account. In SDK v0.50, bank keeper's BurnCoins method only works on module accounts. This is extended to any account in the next version of the SDK.

The simplifications I'm doing in #6558 would benefit from the ability to use BurnCoins directly

[gjermundgaraba]

I also think module account is the right choice here because it will reduce the risk that we get that forwarding address logic wrong. And the SDK has this functionality built in for exactly these kinds of use cases: https://docs.cosmos.network/main/build/modules/bank#module-accounts

[colin-axner]

Yes we should use SDK module accounts (for forwarding escrow) 👍

Context on why transfer doesn't use module accounts already is that transfer was developed before module accounts was a feature. Migrating all escrow addresses is quite sensitive (and not necessarily beneficial given that the current construction is safe). But this historical baggage should not inform our current decisions when possible. We also use module accounts in ics29

[colin-axner]

Did some extra thinking about needing to create isolated buckets based on the hop port/channel, but it doesn't seem to provide any security benefits. Transfer escrow addresses being indexed by destination port/channel is useful since it creates fault tolerance. If connection to chainB goes malicious, chainB cannot steal funds in escrow for chainC

Given that the only funds moving through the forward address go through the normal transfer logic, I don't see how a malicious chain could generate some sequence of steps to steal from funds deposited through the forward address. In addition, funds should never be stored in the forward address after the transfer message completes execution.

When receiving a packet, tokens are either unescrowed or minted to the forwarding address which will then either escrow or burn. Upon refunds, tokens are either unescrowed or minted to the forwarding address, which will then either be escrow or burned when reversing to the original sender.

[crodriguezvega]

Closed by #6688