Open andrey-kuprianov opened 2 years ago
Thanks @andrey-kuprianov!
Maybe instead of a seed
we can attach the controller channelID (it would use the next available channel sequence since the channel is created after the port is binded, but in the same state transition so it'll be atomic)? But this would complicate the existing active channel design
The reason why I think a seed is unnecessary is because the SDK currently functions assuming all modules are honest. They may be faulty, but we can assume there is no module trying to DoS interchain accounts since they could simply access the bank keeper without any issues ie the SDK doesn't protect against malicious modules
Appending the channelID is useful because then it allows multiple controller modules on the controller chain to use the same account addresses without collision
This will have an additional benefit of allowing an account owner on a controller chain to create multiple interchain accounts on a host chain for the same owner address
This is interesting. Is there a benefit to the host chain allowing for multiple accounts associated with a single address as opposed to the controller chain mapping one form of authentication for multiple addresses ie mapping a public key to many addresses via a nonce?
@colin-axner thanks for the write-up!
I understand the assumption that the modules are honest. But we should consider also a scenario not of a malicious module being added to IBC, but of a honest module that might be abused for attack purposes. Imagine this scenario:
AwesomeWallet
is added to IBC in the future. The module allows users to register a named wallet, and to perform operations on it from other chains connected via IBC.AwesomeWallet
, we register the port with the same name; this allows us to easily track which accounts are already taken, and accept connections to the wallet via the port with the same name"What does addition of AwesomeWallet
means for Interchain Accounts
? Right, the developers of AwesomeWallet
unintentionally created a security vulnerability: anyone can register an awesome wallet with the ICA-specific name, thus preventing ICA from functioning.
If you weigh the two alternatives:
I believe the first alternative is much more secure and bullet-proof.
This is interesting. Is there a benefit to the host chain allowing for multiple accounts associated with a single address as opposed to the controller chain mapping one form of authentication for multiple addresses ie mapping a public key to many addresses via a nonce?
Could you please clarify this? Sorry, don't quite get what you meant:)
Thanks for the further clarification @andrey-kuprianov!
In this situation, any IBC application module which binds to ports dynamically is affected by this byzantine module. Rather than always using a seed, I think the longer term fix would be to allow IBC applications to claim a port prefix. This would allow only the interchain accounts controller module to bind to ports under ics27
In the short term, I think it is fine to release interchain accounts with this DoS edge case possible. It seems reasonable to expect IBC application modules which allow user inputted ports to be prefixed as a security precaution
Could you please clarify this?
Yup. So lets say we have a private key, pk, on the controller chain. The controller chain could generate multiple addresses from this pk (via a seed). That is, I use my pk to authenticate transactions on the controller chain and the controller chain allows me to derive multiple addresses from this pk (binding multiple ports). If our pk is really a governance proposal, the governance proposal could provide a seed used to derive the address
The other alternative is deriving multiple addresses on the host chain. So instead of the controller chain generating multiple addresses from a single pk, the host chain is doing this. My personal opinion is that it is cleaner from an architecture point of view to allow the controller chain to do this address derivation, as it reduces overall complexity in the ics specification. Does this make sense?
Rather than always using a seed, I think the longer term fix would be to allow IBC applications to claim a port prefix.
Totally support this! It is indeed a stable long-term solution to the problem.
Wrt address derivation: now I see what you mean. I think my confusion stems from the fact is that ICS27 outlines only part of the design, leaving the authentication module beyond the scene.
Probably you are right, and deriving multiple addresses on the controller chain is a fine and better solution than deriving them on the host chain. But this whole business with ports still makes me uneasy, to be honest... I will think about it more, and return to this later.
We will not be addressing this issue in the first release of interchain accounts. We will add documentation and in a future release provide a long term fix that can be reused for all IBC applications via port prefixing.
Surfaced from @informalsystems audit of ICS27 InterchainAccounts at hash b8bc1a8c
The creation of an interchain account starts in the function InitInterchainAccount():
The function GeneratePortID generates the port identifier in the following format:
As can be seen from the above fragment, interchain account creation will fail if the port is already bound. But according to the ICS05 (port allocation), we have:
Combined, this creates a possibility of DOS attacks on Interchain Accounts:
Note: the problem has been surfaced via formal modeling of Interchain Accounts in TLA+: see the ICA_0 TLA+ model.
Recommendation
Change the port identifier format to include some kind of a random seed, generated upon port creation, e.g. like that:
This will have an additional benefit of allowing an account owner on a controller chain to create multiple interchain accounts on a host chain for the same owner address, which may be used e.g. for differentiating interchain accounts for incompatible purposes, without forcing the user to create separate accounts on the controller chain.
For Admin Use