SDK should not call address.String()

[colin-axner]

Summary

A bug surfaced on the relayer which is explained here. Essentially, the address.String() function uses the currently set config to add the AccountPrefix. Even if the AccAddress is generated with the correctly set config file, if the AccAddress.String() occurs when the config file has been switched, the resulting AccountPrefix may be incorrect.

Here is a simplified flow of what occurred on the relayer:

addr := chain.GetAddress()
proof := counterparty.QueryProof()
msg := ibc.NewMsg(proof, addr)

This appears correct, but what resulted was an address being constructed using the counterparty config file. GetAddress was constructing an AccAddress and setting the source config file, but counterparty.QueryProof would change the context and set the counterparty config file. Then in NewMsg, addr.String() was being called which was using the last set config file.

The fix:

proof := counterparty.QueryProof()
addr := chain.GetAddress()
msg := ibc.NewMsg(proof, addr)

This is a very subtle bug. The relayer code is far from perfect, but I can easily see implementations running into this very often and it's incredibly hard to detect since everything looks correct. .String() is using external context to produce the stringified value. This is dangerous.

I would recommend considering all uses of addr.String() as dangerous/risky in this SDK codebase. All NewMsg... functions should take in a string instead of an AccAddress unless they explicitly cannot. In this case, the function must clearly document this potential bug. The .String function should document this clearly (if it is not already) and recommend that libraries do not use this function.

Edit: Here is where String relies on the config. It might be true that in block code execution is safe to call .String() but that client side code is not since the config could be swapped (as in the relayer)

A long term refactor to remove this config file dependency would probably be best.

cc: @fedekunze @amaurymartiny @jackzampolin

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[colin-axner]

cc @ethanfrey I'm not sure if you will need to use the SDK config, but this is a good thing to be aware of. It just cost me another couple hours in debugging the golang relayer

[ethanfrey]

Thank you.
I think the big problem is that this whole config was designed for single-chain usage and is correct inside the state machine.
None of this was designed for a multi-chain relayer.

The go relayer definitely needs multi-chain support from the SDK.
For the ts relayer we will not rely on any of this go code, as long as it processes correct messages, that should be fine (or is the issue that it tries to parse the remote address string into a local AccAddress?

[colin-axner]

For the ts relayer we will not rely on any of this go code

It is just an issue with using the go code, so you should be fine then

[aaronc]

Note that we are planning to deal with this holistically in #7448 which will likely require passing an address config into all keepers as well as ValidateBasic and GetSigners... Ugly I know but I'm not sure there's a way around it (unless we can do some code generation magic which I haven't figured out yet).

[tac0turtle]

in the near future we plan on removing the global bech32 which should fix this issue. This has been a pain point for us as well.

closing this as the issue has this linked and an updated issue will be opened once we begin the work.