Add IBC and CosmWasm security comments.#4683
Open
mdulin2 wants to merge 1 commit intowormhole-foundation:mainfrom
Open
Add IBC and CosmWasm security comments.#4683mdulin2 wants to merge 1 commit intowormhole-foundation:mainfrom
mdulin2 wants to merge 1 commit intowormhole-foundation:mainfrom
Conversation
Contributor
mdulin2
commented
Feb 20, 2026
mdulin2
commented
- Comments for various security invariants
- A few variable changes to make the code easier to reason about.
* Comments for various security invariants * A few variable changes to make the code easier to reason about.
mdulin2
requested review from
evan-gray,
fergarrui and
panoel
as code owners
johnsaigle
reviewed
Feb 23, 2026
| eventType := gjson.Get(event.String(), "type") | ||
| if eventType.String() == "recv_packet" && chainID != vaa.ChainIDWormchain { | ||
|
|
||
| // SECURITY: Fix for IBC hallucinations vulnerability. Likely unnecessary now. |
Contributor
There was a problem hiding this comment.
Could you add a link if you have one? I'm not sure future maintainers will know what this refers to.
| continue | ||
| } | ||
|
|
||
| // SECURITY: Parsing must be more or the other at a time. |
Contributor
There was a problem hiding this comment.
Suggested change
| // SECURITY: Parsing must be more or the other at a time. | |
| // SECURITY: Parsing must be one or the other at a time. |
?
| // SECURITY: There is no 'action' attribute to differentiate between types on this message. | ||
| // In practice, all other events in CosmWasm contract will fail parsing | ||
| // because they are missing fields. So, there's a type confusion issue here | ||
| // but it's unexploitable. These contracts are unlikely to change as well. |
Contributor
There was a problem hiding this comment.
Suggested change
| // but it's unexploitable. These contracts are unlikely to change as well. | |
| // but it's unexploitable. |
Maybe this part could be more action-oriented? Like "This code should be revisited if the underlying CosmWasm contracts change."
| } | ||
| chainId, ok := mappedAttributes["message.chain_id"] | ||
|
|
||
| // Effectively ignored |
| // SECURITY: The ibc watcher is the only watcher that can handle multiple chain IDs | ||
| // To make this safe, it has a mapping from channel ID to chain IDs that it uses | ||
| // This prevents chains like Solana and Ethereum from being emitted from. | ||
| // Prevents cross-chain attacks where Osmosis can emit an event for Injective as well. |
Contributor
There was a problem hiding this comment.
Is this just an example, or is there something specific to know about Osmosis and Injective?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.