ICS20 Inconsistencies with spec and improvements to code

[ebuchman]

Surfaced from Informal Systems IBC Audit of cosmos-sdk hash 82f15f306e8a6a2e9ae3e122c348b579c43a3d92 and ics hash eb31a56467a48cd7eb4c2232705ad0fc632f9a19.

This is a working list of inconsistencies between the code and the spec of ICS20 that will continue to be updated. Most of these are issues with the spec, but there's a few places where the code could be improved.

• Spec has a setup function which calls BindPort and ClaimCapability and should run once when module is created. But Code calls these functions during OnChainOpenInit and OnChanOpenTry instead.
• Spec uses undefined newAddress() in onChanOpenInit and onChanOpenTry and stores it in a map under the channel id. Code uses a deterministic function of the portID and channelID, ie. newAddress(portID, channelID). With crossing-hellos we could have a safety problem according to this spec as we can execute onChanOpenInit (creates escrow address E1), then createOutgoingPacket using E1, then onChanOpenTry we creates a new escrow address E2 that replaces E1, so money is gone. It seems that at code level we don't have this problem as implementation does not follow spec, i.e., it generates escrow account based on portId/channelID. But if someone follows the spec, then we have safety problem.
• OnChanCloseInit causes rollback in code (returns an error) but is NoOp in spec
• Naming inconsistency: TransferCoins in the spec
  vs. SendCoins in the code
• FungibleTokenPacketData: amount is uint256 in spec and uint64 in the code.
• module binds to "transfer" port in the code, and to "bank" in the spec
• In the spec onRecvPacket prefix should contain "/" at the end to be aligned with the code.
• createOutgoingPacket spec has unused source argument
• createOutgoingPacket spec is missing sequence number when constructing a Packet and calling sendPacket
• Denom hashing and coin traces aren't mentioned in the spec but play an essential role - should at least link to ADR001
• Code is missing explicit createOutgoingPacket function - either keeper.SendTransfer should be renamed to CreateOutgoingPacket or it should call such a function.
• GetReceiveEnabled check in OnRecvPacket code is not found in spec. Is this really how IBC application functionality should be enabled/disabled by governance? Is there a more generic approach?
• refundPacketToken spec does not return errors, but there are fallible operations in the code (ie. SendCoins and MintCoins). If we cannot 'mint vouchers back to sender' there is a problem. Either the code should clarify/assert that errors are not possible, or errors should be handled somehow more severely.
• English text for Data Structures has the wording "and whether the sending chain is the source of the asset", which is missing both from the pseudo-code and from the implementation.

Additionally, some places where the code could be improved for maintainability:

• In OnAcknowledgementPacket, the default branch of a switch statement is used for success logic. This is bit risky to maintain if additional error codes get added in the future (in the ICS it simply says if (!ack.success)). Rather, all error and success cases for the ack.Response should be handled explicitly and the default should be an error.
• OnRecvPacket converts all errors equally into Acknowledgements. The ICS specifies that errors from Transfer and Mint should be converted to acknowledgements, but keeper.OnRecvPacket can also error in other ways, like on data.ValidateBasic, or if receiving is not enabled, or the receiver address is incorrectly decoded. Probably these last error types should be genuine errors (ie. cause rollbacks) or they should be captured in the spec.

[AdityaSripal]

Spec has a setup function which calls BindPort and ClaimCapability and should run once when module is created. But Code calls these functions during OnChainOpenInit and OnChanOpenTry instead.

This isn't true, BindPort does happen when module is initialized here, this will call BindPort on portKeeper and claim the resultant port capability: https://github.com/cosmos/cosmos-sdk/blob/master/x/ibc/applications/transfer/keeper/genesis.go#L20
The capability that is being claimed in OnChanOpenInit and OnChanOpenTry is the channel capability. This can be made more clear in code and docs.
Though the functionality of setUp is split up in code across app initialization logic and InitChain logic.

OnChanCloseInit causes rollback in code (returns an error) but is NoOp in spec

I believe spec should change right? cc: @cwgoes

module binds to "transfer" port in the code, and to "bank" in the spec

I believe spec should change. No need to add confusion between transfer module and bank module.

[cwgoes]

I believe spec should change right? cc: @cwgoes

Yes, this should change in the spec.

I believe spec should change. No need to add confusion between transfer module and bank module.

Aye, sure.