Open hats-bug-reporter[bot] opened 5 months ago
This function is only supposed to be called whenever a chain is forked. I don't understand how we would mitigrate against this or really how we are supposed to do anything about this.
I also disagree about the ack, the first byte is not included: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L465
But yes, if the change is made on the sending side the ack wouldn't be deliverable. But this change wouldn't be worse than Wormhole Guardians creating wrong VAAs.
Thanks for the correction, still the outcome is the same as you said the ack wouldn't be deliverable.
I understand the preconditions for the Wormhole function to be called, but if that occurs your system gets broken.
I need to think about the proposed fix. My biggest concern is how you lock in the configuration inside your contracts while the underlying messaging protocol can change it.
Proposed fix:
Each time someone sends a packet you query the messageFee()
. This is done as the underlying messageFee
can be changed.
While deploying IncentivizedWormholeEscrow
save the evmChainId
and chainId
and compare them to the values stored inside the Storage.WormholeState
struct, i.e. provider.chainId
and evmChainId
.
If they don't match revert()
;.
This will ensure no damage can be done, and a proper fix can be devised.
In case of a chain fork, it is expected that the minority chain is the one forking. We would recommend users to not have inflight swaps while there is a chance of forks.
The main chain will continue functioning as intended.
Furthermore, the audit competition details exclude administration "exploits".
Github username: -- Twitter username: -- Submission hash (on-chain): 0x61e756f17fc91d930f9df3ec4a25684a5b2a43e82dfd18cb1b236b601afee914 Severity: high
Description: Description\ Describe the context and the effect of the vulnerability.
Description\ Wormhole governance can change the chainId: https://github.com/wormhole-foundation/wormhole/blob/a048c9ac83129c25dffa4919d07931208e83d866/ethereum/contracts/Governance.sol#L146.
_verifyPacket
reads thevm.emitterChainId
, and passes it to internal functions inside theprocessPacket
.This is used to fetch the expected sourceImplementationHash from the implementation mapping: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L285.
If there is no hash found the message is sent back with the
NO_AUTHENTICATION
flag.This is the case with
_handleMessage
flow.With
_handleAck
the message couldn't even be delivered: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L372.Scenario\ If we take the example of
CatalystChainInterface
and the above scenario has occurred, the ack message would be received back on the sending chain. As the first byte would be different than0x00
: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L463. The execution would continue inside the_onPacketFailure
function: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystChainInterface.sol#L421C14-L421C30.It could never be delivered as context is neither
CTX0_ASSET_SWAP
norCTX1_LIQUIDITY_SWAP
.This would mean permanent loss of funds for the user.