Determine whether this is actually clearer / more ergonomic than our current logic.

This will likely require historical info module to be moved into IBC

Summary

confusion has continued to arise around the notion of misbehaviour (formerly referred to as evidence) for IBC light clients. Some of this discussion is well documented below. We should translate these notions, decisions and understanding into an ADR

ref: cosmos/cosmos-sdk#7145 (comment)

For Admin Use

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

Unnecessary to have frozen sequence since we don't allow anything to verify before frozen sequence anyway.

• Comment about the optimistic handshake (why the connection may not be open in ChanOpenInit)
• Add comment about conditional sequence sets in ConnOpenTry
• Clarify it is not necessary to call ChanOpenConfirm if both chains called ChanOpenAck. Similarly, not necessary to call ChanCloseConfirm if both chains called ChanCloseInit

Summary

Add a spec directory for each IBC client implementation.

• solo machine
• tendermint
• localhost

Problem Definition

Clients can be very complex in verification logic and our IBC implementation can very in small details from the ICS spec while still being valid. We should be clearly documenting in words the behaviour of each client.

Proposal

Add a spec directory for each client (tendermint, localhost, solo machine). This can be done now or as we move clients in a light-clients directory.

Some specific things to be mentioned:

Tendermint (TODO: add from previous prs - validator set hash change, removal of signed header)

Localhost

• comment
• comment

For Admin Use

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

Each IBC tendermint client must be kept up to date by submitting tendermint headers to the chain. These headers then get stored as ConsensusStates. As IBC clients are long-lived, they will store many consensus states. Each consensus state stores the validator set (which may contain ~100 validators), thus they take up a nontrivial amount of space.

However, the only consensus states that need to be kept by the IBC client are the ones that are within the current unbonding period. Every other consensus state that has been stored since client state initialization can be pruned.

Since well-connected chains like hubs maybe connected to many clients, each of which may be submitting many headers, I think it is worth pruning the consensus states so that the space required by IBC module does not explode with time.

Not sure if this is absolutely necessary for IBC 1.0, but definitely seems like a good feature to have, and relatively simple to implement.

Open to other proposals but a simple fix is to prune all expired consensus states upon receiving a new one. This would just require that clients also have a field that includes the earliest consensus height that is still in state EarliestHeight uint64.

The UpdateClient function would then, upon receiving a valid update, check the earliest height to see if it is expired given unbonding period. If it is, then delete it from store and keep checking and deleting the next earliest heights until we find one that isn't expired. Reset EarliestHeight in client and return.

This should keep the space taken by each client bounded by the average number of blocks produced within an unbonding period.

cc: @cwgoes @fedekunze @colin-axner

As title.

Summary

gaiad q ibc client header  --chain-id slippy-0 --node http://159.65.118.186:26657 
Error: RPC error -32603 - Internal error: page should be within [1, 1] range, given 0

For Admin Use

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

Summary of Bug

The 07 client uses the Tendermint Go ValidatorSet defined here. This comes with a bunch of data about proposer selection, including a proposerPriority int64 for each validator and aProposer Validator field. There's also a totalVotingPower. None of these fields are checked or necessary for current IBC, and so they can be set arbitrarily in UpdateClient datagrams. Technically the light client does have enough info to verify them if it wants to run the proposer selection algo itself, but there doesn't seem to be a good use for that yet (until/unless we want to punish proposers for bad blocks over ibc, or other potential changes). I think having unverified fields is generally bad since its a recipe for misuse, so might be worth removing this at some point.

This points to a larger issue in divergence between spec and code due to the use of existing implementation types, eg. tendermint protos. This particular issue could be solved upstream in tendermint by breaking up the ValidatorSet type, or in the SDK by using its own ValidatorSet type that just has the list of pubkeys and voting powers. More generally it seems there may be a strong argument for defining canonical IBC proto files in the spec repo.

For Admin Use

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

Summary

We need to ensure that crossing hello's can succeed. Test for connection and channel handshake should be added to verify that:
ChainA : OpenInit
ChainB: OpenInit
ChainA: OpenTry
ChainB: OpenTry
ChainA: OpenAck
ChainB: OpenAck

can result in an OPEN connection/channel ends on each chain

For Admin Use

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

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.

Summary

Add documentation in the tendermint proto file for IBC explaining which parameters are chain level and which are client level

For Admin Use

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

Write a complete ADR that documents the changes from cosmos/cosmos-sdk#7280. This format will be required by all chains implementing IBC.

@AdityaSripal @colin-axner @cwgoes

For Admin Use

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

Surfaced from Informal Systems IBC Audit

Summary of Bug

The path space laid out in the ICS24 spec doesn't fully match the ics24 code. It also might be helpful if each path in the spec had a name (which could correspond to function names in the code), and if the code was a bit more clearly organized for direct comparison with the spec.

Presumably things to fix in the spec:

• clientType from spec is missing in code (it's part of ClientState, so doesn't have its own path in store)
• commitments/ports/{identifier}/channels/{identifier}/packets/{sequence} in the spec is commitments/ports/{identifier}/channels/{identifier}/sequences/{sequence} in the code (and same for the receipts/ and acks/ paths.
• ChannelCapabilityPath in the code is missing from the spec

There's also some redundancy in the code with prefix functions, like PacketAcknowledgementPath could use PacketAcknowledgementPrefixPath and FullConsensusStatePath could use ConsensusStatePath.

For Admin Use

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

Summary

see comment.

This would ideally happen in all of the ibc module as well as the ibc-transfer module. Documentation would need to be added to the relayer guide to give example interaction with consuming the events

For Admin Use

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

• Clarify comment about preventing transfers in transfer.proto (no cross-chain per-denom enable/disable)
• Use SHA256 instead of imported tmhash
• Add comment explaining why base denom is not validated (coming from another chain)
• Rename validatePrefixedDenom
• Replace "ibc" with constant
• Remove hardcoded numbers (e.g. [4:] should be length of a constant) in DenomTraceFromHash and elsewhere
• Add comment why the acknowledgement is 1
• Abstract out denomination calculation functions
• Add comment about zero-coins
• Move events around (after state changes, not in-between)
• Comment to clarify security model around denomination space namespacing in RecvPacket
• Improve naming of trace functions generally
• Note that ordered channels are considered dangerous

As title, since we're doing non-trivial input validation checks here.

• Check redundancy in ValidateBasic of pubkey
• Consider whether we want to require a non-empty-string diversifier
• Better abstract solo machine client verification functions, they repeat a lot of code

Summary

Add a comment in connection handshake on how versions are constructed to align more with the ICS spec

For Admin Use

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

Summary

• Add comment about optional chain ID format and default version number in chainID basic validation
• Regex for upgrade path validation
• Try to deduplicate code in all the VerifyXYZ functions
• Check FrozenHeight LTE instead of !GT
• Move basic clientState checks to top of VerifyXYZ functions (less intensive checks first)
• Ditto for basic consensusState checks
• Remove redundant check in upgradeClient (upgrade path empty string check and maybe more)
• Remove redundant checks in tendermint update/misbehaviour functions in comparison to 02-client checks and msg.validate basic

For Admin Use

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

Summary

The IBC cli is not being used or tested. It should be refactored to support more dynamic usage.

Problem Definition

While working through 03-connection I noticed the cmd for the OpenTry handshake passed in proofInit to the proofConsensus field. Any attempt at using this command successfully should fail showing that no one has attempted to use this command (which is essential to doing anything IBC).

The iqlusioninc relayer, one of the more widely used relayers, does not rely on these commands and instead has its own custom cli commands. There should be at least a few usable commands it could import instead of implementing all of its own. This should be true for any relayer.

Proposal

Refactor and test the IBC cli/rest commands with usability in mind. Currently proofs are passed in directly or by json file. There should be an option to pass in a connection endpoint to the counterparty chain to query and parse the proofs directly. Perhaps there should also be commands to query and store proofs (if there isn't already)

This should probably be punted until after the gRPC refactor across the sdk codebase.

A concrete list of things that need to be done to close this issue:

manually test the following cmds:

• create client
• update client
• client misbehaviour
• upgrade client
• transfer tx cmd
• transfer query demon trace
• transfer query denom traces
• query client state
• query client states
• query consensus state
• query consensus states
• query header
• query node consensus state
• query connection
• query connections
• query client connections
• query channel
• query channels
• query connection channels
• query channel client state
• query packet commitment
• query packet commitments
• query unreceived packets
• query unrelayed acks
• query next sequence recv

cmds to add and test:

• relay packet
• relay acknowledgement

Also consider applying the original proposal of this issue, allowing an endpoint to be passed/queried for counterparty proofs. Maybe this can be opened in a separate issue and implemented if users request the same feature

Instead, panic in the app.

SDK should provide efficient iteration of consensus states for a particular client in reverse chronological order.

To accomplish this, the consenus state height must be stored as part of the key in such a way that the reverse lexicographic order is the same as the (reverse) chronological order. Then we could simply use the SDK store's ReverseIterator implementation.

Summary

Summarizing here discussion with @milosevic and Ilina about this and also @ebuchman on cosmos/cosmos-sdk#7870:

A connection or channel will never reach Open state if there is a mismatch in the identifiers. It is very simple to write a relayer that watches the OpenInit events from A and immediately send a MsgOpenInit to B with mismatched connection ids, blocking forever the handshake on the connection/ channel.

It may be that the “correct” relayer that would guarantee that the handshake finishes would have to do something like:

• always send MsgOpenInit without specifying the counterparty connection id (this is allowed now in order to support flexible id selection)
• specify the counterpary connection id in OpenTry (i.e. tie the two connection ends with a message that carries proofs)

So maybe one solution is to not allow the MsgOpenInit with counterparty connection_id specified.

This should be looked at more closely and determine the best solution.

For Admin Use

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

Also, take the trace as user input and calculate the hash instead of looking up (so we shouldn't need to look it up from metadata).

Summary

The IBC testing package could use some improvements which include:

• pass in port as an argument so the caller can use their module (this allows for their capability to be binded correctly)
• channel id needs an update naming scheme so creation of multiple clients don't collide
• pass in versions as arguments (preferably as an optional arg) for channels
• add support for recv and ackowledgement message passing which will call port callbacks
• cut down redundancy of querying consensus state proofs into one helper func
• add randomness in test chain/client/connection/channel naming so different chains don't have identical ID names
• batch update client and handshake messages into single transaction
• add support for parsing events to relay messages
• add relaying packets helper func
• query for ack in RelayPacket helper func
• support dynamic validator set to test tendermint updating behavior with hashed validator sets
• add a mocked IBC app module for core ibc handler testing so it isn't reliant on changes to ibc-transfer
• rename the invalidID variable to IDDoesNotExist, as a I believe this is the usage for it
• batch all looped msgs being sent into a single transaction
• add a third argument to setup to indicate channel type
• remove CreateTestHeader from 07-tendermint/types by migrating tendermint types to use ibctesting package (partially done)
• add a README.md to describe how to use it for application modules
• consider refactoring next block/commit uses
• make some optimizations to cut down on test times

These improvements mostly center on allowing module outside of ics20 to be used, which is an important improvement.

For Admin Use

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

• Remove antehandler comments on packet functions since everything happens in handler now.
• Clarify documentation around ordered/unordered differences in receipt handling (note: relayer may be wrong regarding this)
• Add comment on the timeout height check in TimeoutPacket, it is very confusing

• Change CapabilityOwners.Set to document better why we need this index (keep list sorted)
• Add comment for DefaultIndex (cannot start at zero) in genesis.go Godocs
• Add comment on capMap describing what it is for
• Don't use capabilities with empty names, or check if name is empty in AuthenticateCapability
• Add check for nil capability + empty name to ClaimCapability
• Better document invariants around GetModuleOwner
• Add a note to ExportGenesis on why we can't use the starting index
• Rename SetIndex so it's clearer that it can only be called on genesis initialisation

Would be useful to simulate, if we can cover the state space well:

• Client creation, client updates
• Handshakes for connection & channels (opening & closing)
• Packets, acks, timeouts

Summary

The call to addConnectionToClient should be called before SetConnection in OpenInit. The state change won't be committed if the error is returned, but it is easier to read/verify when setting the connection state occurs last.

For Admin Use

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

Implement ADR 025 - IBC passive Channels

For Admin Use

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

Summary

When creating connections and channels we do not store them under client prefixed stores. They are stored under the IBC prefix and then either channelEnds or connections. When verifying connection and channel states for light clients we pass the client prefixed store, so the client can access the consensus states if necessary. The localhost was using this client prefixed store to verify connections/channels. Unfortunately, connections and channels are not stored there, so this check would always fail.

For Admin Use

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

Summary

In connection handshakes we can check that the counterparty client we are verifying isn't expired. Not necessary, but would be safe to do

For Admin Use

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

• Pass channel objects around from msg to keeper methods directly rather than deconstructing into individual fields and reconstructing
• Better error messages indicating which module failed for OnChanOpenInit etc.
• Avoid calling GetChannel() twice in ConnOpenTry

Summary

Can the keeper of IBC transfer be changed to an interface?

Problem Definition

I am currently working on a project to automatically mint some vouchers to the user when the user transfers tokens. I think there is a need to ensure the transactional of these operations, so the mint needs to be completed at the same time as the transfer takes place.

This is easy to achieve in the keeper of the bank module, because the keeper of the bank is an interface.

https://github.com/cosmos/cosmos-sdk/blob/dd9ef120f3415c4d84d3ce34267453ecb94b797f/x/bank/keeper/keeper.go#L22

I just need to refer to the keeper of the bank module and wrap the SendCoins(...) method, and then replace the keeper of the bank module:

https://github.com/wangfeiping/flares/blob/1ed7d65a6f5cd8566267adc038c8fe47c42f32d5/app/app.go#L272

https://github.com/wangfeiping/flares/blob/1ed7d65a6f5cd8566267adc038c8fe47c42f32d5/x/bank/keeper/keeper_wrapper.go#L15

https://github.com/wangfeiping/flares/blob/1ed7d65a6f5cd8566267adc038c8fe47c42f32d5/x/bank/keeper/keeper_wrapper.go#L41

But IBC transfer's keeper is not an interface, so it can't be implemented that way.

https://github.com/cosmos/cosmos-sdk/blob/dd9ef120f3415c4d84d3ce34267453ecb94b797f/x/ibc/applications/transfer/keeper/keeper.go#L36

If the keeper of IBC transfer is also defined by interface, will it be more in line with OCP (Open Close Principles ) and more conducive to future maintenance and expansion?

Proposal

Please change the keeper of IBC transfer to the interface.

For Admin Use

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

Surfaced from Informal Systems IBC Audit

Summary of Bug

ICS26 is effectively implemented in the x/ibc/core/keeper/. This could be clarified.

Code vs Spec:

• Spec always calls module handler ("callback") before calling IBC handler. Code alternates
  • eg. ChanOpenInit/Try call the IBC handler first, module handler (callback) second, while OpenAck/Confirm do the reverse
  • eg. handlePacketRecv and handlePacketAck spec calls app handler first, code calls IBC handler first
  • does it matter what order these are called in ? Should we be consistent?
• handlePacketRecv in spec doesnt write the ack; in code it does

Few notes around return values and events. Could potentially be cleaned up clarified a bit:

• All returned results are empty - any data is sent through events.
• Inconsistency in where events are emitted. For client and connection handlers, events are emitted in the ICS26 code. For channels, events are emitted in ICS04 code.
• ICS04 handshake handlers return sdk.Result containing events, but they're ignored
• App module packet callbacks return sdk.Result containing events, but they're ignored (onReceive/Timeout/AcknowledgePacket)

Other code notes:

• packet-based telemetry code in RecvPacket/Timeout/TimeoutOnClose can be deduplicated

For Admin Use

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

Summary

We should add more in code documentation, with a link to a nice diagram displaying the possible handshake steps. The documentation in code should explain the aim of the handshake step. What we want to verify and configurations are selected in this step.

We should also add a comment in connectiontypes.Counterparty.ValidateBasic() explaining why the counterparty connection id may be blank and that it is verified later in OpenTry and OpenAck

Document VerifyClientState to remove confusion over targetClient vs ClientState. Be more consistent about choice of variable names here as well

For Admin Use

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

Surfaced from Informal Systems IBC Audit of cosmos-sdk hash 82f15f3.

Summary of Bug

This is an instance of a more general class of issue with the SDK's object capability system, which also seems to be mentioned in cosmos/cosmos-sdk#7093 in the context of the Bank Keeper. Here we surfaced a similar issue in the IBC Port and Channel Keepers.

In an ocap system, each ocap has associated with it a set of actions that can be performed, ie messages that can be sent or methods that can be called. In the SDK, actions are defined through Keepers. Since all methods on a Keeper are public, any module with access to a keeper can call all methods exposed by that keeper. In each module, we define a types/expected_keepers.go which define the keepers this module uses (besides its own) as interfaces with a limited set of methods. While useful for testing and local representation of the actions this module needs to execute, this does not meaningfully restrict the ability of a malicious module to add other methods to its expected keepers interfaces, and thus access other methods exposed by those keepers beyond those which are intended!

Of course this depends on malicious modules including code that escapes audit and review, which is maybe not an overwhelming concern right now, but the SDK has at least intended to mitigate such issues following principles of least privilege via the ocap model. The problem is that keepers are actually granted to modules without minimizing privilege at all.

For instance, modules which receive the PortKeeper need only the PortKeeper.BindPort method:

// PortKeeper defines the expected IBC port keeper
type PortKeeper interface {
	BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability
}

But a full 05-port/keeper/Keeper is actually passed in, and the full keeper has a LookupModuleByPort method which returns the capability associated with a port. If this were exposed to IBC application modules, they'd all be able to claim capabilities for any other modules port, which would defeat the purpose of the port system.

Thus a malicious IBC application module could add the LookupModuleByPort method to its expected PortKeeper interface, and then open channels on some other module's port. For instance the following diff to the transfer module should do it:

-// BindPort defines a wrapper function for the ort Keeper's function in
+// BindPort defines a wrapper function for the port Keeper's function in
 // order to expose it to module's InitGenesis function
 func (k Keeper) BindPort(ctx sdk.Context, portID string) error {
-       cap := k.portKeeper.BindPort(ctx, portID)
-       return k.ClaimCapability(ctx, cap, host.PortPath(portID))
+       someoneElsesPort := "transfer"
+       _, cap, _ := k.portKeeper.LookupModuleByPort(ctx, someoneElsesPort)
+       return k.ClaimCapability(ctx, cap, host.PortPath(someoneElsesPort))
 }
 
 // GetPort returns the portID for the transfer module. Used in ExportGenesis
diff --git a/x/ibc/applications/transfer/types/expected_keepers.go b/x/ibc/applications/transfer/types/expected_keepers.go
index 284463350..5350c66bd 100644
--- a/x/ibc/applications/transfer/types/expected_keepers.go
+++ b/x/ibc/applications/transfer/types/expected_keepers.go
@@ -45,4 +45,5 @@ type ConnectionKeeper interface {
 // PortKeeper defines the expected IBC port keeper
 type PortKeeper interface {
        BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability
+       LookupModuleByPort(ctx sdk.Context, portID string) (string, *capabilitytypes.Capability, error)
 }

The same holds true for the 04-channel/keeper/Keeper which exposes a LookupModuleByChannel method. Any IBC module could thus grab the capability associated with any channel, and send/recv on another modules channel.

Fix

Keeper methods should be restricted from outside the module - whoever is composing modules, presumably in app.go, should explicitly define which methods of a keeper each module gets. Note this implies that expected keeper interfaces may end up duplicated (ie. once in the actual application for security and once in the module for local clarity/convenience), or may come to live in a trusted alternative place outside the control of an individual module. In any case, we may consider an external Secure interface for the expected keeper (from outside the module), and an insecure Local one (inside the module). So long as keeper variables inhabit a variable of the Secure type before being passed into modules, that should be sufficient. Ie.:

/*
	Some keeper defined elsewhere with two methods. We only want modules to access the Hi method
*/

type A struct{}

func (A) Hi()  {}
func (A) Bye() {}

/*
	Inside the Module
*/

// Malicious module tries to access the Bye method
type Local interface {
	Hi()
	Bye()
}

// Local function within the module
func LocalA(a Local) {}

/*
	Outside the Module
*/

// We only want modules to get the Hi method
type Secure interface {
	Hi()
}

func TestHelloWorld(t *testing.T) {

	// if we don't restrict a, the module can access all its methods
	a := A{}
	LocalA(a)

	// if we restrict a to the Secure interface, the module can't access other methods
        // and this fails to compile :)
	var a Secure = A{}
	LocalA(a)
}

Related

• cosmos/cosmos-sdk#5931 points out an issue around module accounts being access by other modules. Even if module accounts were gated by ocaps, as that issue proposes, a general method which allowed those ocaps to be looked up (like we have in channel and port keepers) would still lead to compromise.
• cosmos/cosmos-sdk#7093 points out various issues specific to the BankKeeper (ie. maintaining supply invariants), but also mentions that Keeper methods are not restricted before being passed into modules. So this issue is in some sense a duplicate of that issue, but specific for issues with Port and Channel Keeper.

For Admin Use

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

Summary

ref: comment

For Admin Use

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

Similar to cosmos/cosmos-sdk#5826 , we should have a Relayer guide that walks through the relevant parts of IBC for relayers to understand and write their own relayer implementations.

TODO:

• How to relay handshakes
• How to pass in Proofs correctly
• How to relay packets
• document race condition comment
• document update client workflow comment

Summary

A guide to implementing a light client for IBC

• overview (vocabulary - client/consensus states, client message etc ie each interface) #1850
• client_state.go and its functions #1851
• consensus_state.go and its functions #1852
• existence/non-existence proofs #1853
• update function #1854
• misbehaviour handling #1854
• upgrade handling
• proposal handling
• genesis metadata

For Admin Use

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

Summary

In parse.go in 24-host we currently use strings.Split to parse out identifiers from a key used to store IBC data. We should refactor this to use regex to verify the format and parse out the identifiers needed.

For Admin Use

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

Summary

Lots of major changes have occurred to the interaction of 07-tendermint with 02-client. We should take this time to update specs and add/modify testing to be more rigorous.

Proposal

See related comment from the pr that removed the last header from client state.

We should also apply the validator set -> validator hash in consensus state change in each of the the areas mentioned in the comment (specs, testing etc)

For Admin Use

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

Summary

Capabilities in the core spec for IBC needs to be finished

For Admin Use

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

Summary

There are no specs for ICS20 atm, I think because the specs were added to IBC and then ICS20 was moved up a level.

Specs should be added following the module specification structure

• 01_concepts
• 02_state
• 03_state_transitions
• 04_messages
• 05_events
• 06_params

For Admin Use

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

For example, the check of whether we are allowing empty feature set intersections could be abstracted.

Surfaced from Informal Systems IBC Audit of cosmos-sdk hash cosmos/cosmos-sdk@6cbbe0d

Description

In multiple places throughout the Cosmos-SDK, functions contain several sub-calls that may fail due to various reasons. At the same time, the sub-calls update the shared state, and the functions do not backtrack the state changes done by the first sub-calls if one of the subsequent sub-calls fails.

Consider this example of SubtractCoins():

for _, coin := range amt {
    balance := k.GetBalance(ctx, addr, coin.Denom)
    locked := sdk.NewCoin(coin.Denom, lockedCoins.AmountOf(coin.Denom))
    spendable := balance.Sub(locked)
    _, hasNeg := sdk.Coins{spendable}.SafeSub(sdk.Coins{coin})
    if hasNeg {
        return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "%s is smaller than %s", spendable, coin)
    }
    newBalance := balance.Sub(coin)
    err := k.SetBalance(ctx, addr, newBalance)
    if err != nil {
        return err
    }
}

The function iterates over the set of coins and for each coin checks whether enough coins are available for the current denomination. The balance is updated for each coin as the loop progresses. Consider the scenario where the balance for the first coin is updated successfully, but for the second coin setting of the balance fails because of the negative amount of coins. The function returns the error, but the balance for the first coin remains updated in the shared state.

This violates one of the most basic assumptions of function atomicity, namely that either

1. the function updates the state and succeeds, or
2. the function returns an error, but the shared state is unmodified.

We have found multiple occasions of such non-atomic functions; here are some, besides the example above:

Bank:

• AddCoins similar issue with the difference it panics when overflow happens for some denomination.
• SendCoins first subtracts from the sender account and then adds to the receiver. If subtract is successful and add fails the state is changed.
• DelegateCoins: delegation is processed denomination by denomination: the insufficient funds is check inside the loop allowing state updates even when an error is reached.
• UndelegateCoins similar scenario.
• BurnCoins and MintCoins use SubtractCoins and AddCoins.

ICS20:

• OnRecvPacket: first minting then moving into the account. Each step modifies the bank state. Each step potentially reaches errors. Therefore it is not an atomic write to the bank as given in the spec.
• SendTransfer: similar to OnRecvPacket, first SendCoinsFromAccountToModule() is called, that modifies the state, and after that, if BurnCoins() fails, the error is returned, but the state is left modified.

The problem is that all above functions have implicit assumption on the behavior of the caller. This implicit assumption is that whenever any such function returns an error, the only correct behavior is to propagate this error up the stack.

While this assumption seems indeed to be satisfied by the present Cosmos SDK codebase (with one exception, see the problem scenario below), it is not documented anywhere in the Cosmos SDK developer documentation. There are only hints to this in the form similar to this paragraph in the Cosmos SDK handler documentation:

The Context contains all the necessary information needed to process the msg, as well as a cache-wrapped copy of the latest state. If the msg is succesfully processed, the modified version of the temporary state contained in the ctx will be written to the main state.

Such hints do not constitute enough developer guidance to avoid introducing severe bugs, especially for Cosmos SDK newcomers.

Problem Scenario

We have found one particular place where this non-atomicity has almost led to the real bug. Namely, in ICS20 OnRecvPacket we have two consecutive operations, minting and sending.

// mint new tokens if the source of the transfer is the same chain
if err := k.bankKeeper.MintCoins(
    ctx, types.ModuleName, sdk.NewCoins(voucher),
); err != nil {
    return err
}


// send to receiver
if err := k.bankKeeper.SendCoinsFromModuleToAccount(
    ctx, types.ModuleName, receiver, sdk.NewCoins(voucher),
); err != nil {
    return err
}

If the minting succeeds, but the sending fails, the function returns an error, while the funds are moved to the module account.

This is how this function is called in applications/transfer/module.go:

err := am.keeper.OnRecvPacket(ctx, packet, data)
if err != nil {
    acknowledgement = channeltypes.NewErrorAcknowledgement(err.Error())
}

We see that the error is turned into a negative acknowledgement. If sending of the coins above was to fail with an error, then the negative acknowledgement would be sent, but also the funds would be silently moved to the module account under the hood. We have carefully examined the code of SendCoinsFromModuleToAccount, and found out that its current implementation can only panic, e.g. when the receiver account overflows. But if there was a possibility for it to return an error this scenario would constitute a real problem. Please note that these two functions are probably written by two different developers, and it would be perfectly legitimate for SendCoinsFromModuleToAccount to return an error -- this is how close it comes to being a real bug. Please see also the https://github.com/cosmos/ics/issues/504 for more details on this issue.

Recommendation

• Short term: properly explain in the developer documentation the implicit assumption of propagating all returned errors up the stack , as well as in the inline documentation for all functions exhibiting non-atomic behavior.
• Long term: one of the following needs to be done:
  • either make the SDK functions atomic as described above;
  • or introduce a separate explicit step for handlers, say CommitState, that the handler will need to call to write state changes to the store.

For Admin Use

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

Reverse key order from leaf to root so that it follows proof and spec order.

Clients should still pass keys normally (root-to-leaf), and then we reverse order internally before calling verify code

Summary

During internal audit we noticed that message handling was missing from handler.go. This means it was not possible to successfully execute an upgrade message (since it wasn't being routed). We should add tests to ensure upgrade works from end to end.

For Admin Use

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