search

hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 3 forks source link

non-EVM and EVM chain ids can collide in the current wormhole implementation #72

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0xbbc3bc4e6bf3a5002cc3cec2a5616cfe3cd36c45e1f71d9cc77de47086174071 Severity: medium

Description: Description\ The wormhole implementation, unlike it's mock counter-part, uses the chainId() parameter of it's chain, instead of the wormhole-provided internal chain ids. Thus, non-EVM chains matching an EVM chain's id or that does not posses an id function, could generate a replayable or wrong identifier.

Attack Scenario\ A similiar issue regarding wormhole chain ids: https://solodit.xyz/issues/wormhole-bridge-chain-ids-are-different-than-evm-chain-ids-spearbit-lifi-pdf Even though this issue talks about mismatch from wormhole, it provides a solid fix to our issue here, since not all non-EVM chains have an id and even if they do, it could return a value already held by an EVM chain. E.g a message signed on a non-EVM chain with an id of 4 could be replayed on an EVM chain with id since their identifiers would match for the same block (and toAccount and amount of course). Generally chainId() is inconsistent. Another one of my issues #69 also highlights an issue with hardcoding the chainId without a method to change it, leading to the same impact, but from a different root cause, thus I split them. The recommendation below could work as a fix to both issues.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Recommendation would be to use the wormhole-provided chain ids (like the linked report), which group both EVM and non-EVM chains. If you choose to go with hardcoding like in the mock, make sure to add an admin setter function for it to avoid issue #69

reednaa commented 5 months ago

I do not understand how this is an issue. Please provide PoC.

PlamenTSV commented 5 months ago

Names: Let non-EVM chain be X, EVM chain be Y and destination chain be D If we sign a message from X->D, it will be signed with X's chainId, the message itself, block and toAccount on D If a chain Y matches the id of X, we can provide the same message with the block in processPacket and replay it.

After reading the other issue once again, I believe these 2 issues are pretty much the same root cause and fix, it is just 2 impacts/scenario is which chainId's can match. You can mark this a dupe of #69 and discuss it there.