Signature replay allows user to mint any amount of tokens on a chosen destination chain

[hats-bug-reporter[bot]]

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

Description: Description\ This report targets the mock implementation provided. The core of the issue is the possibility of replaying valid signatures inside the mock's _verifyPacket. It uses a provided metadata parameter, that's entirely user input, in order to retrieve a signer. By using a combination of arbitrarily crafter parameters, an old valid signature and the processPacket to send liquidity, a malicious actor would be able to mint himself tokens on any chosen destination chain for free.

Attack Scenario\

1. We do processPacket with an arbitrarily crafted message and a context(metadata) that has been valid in the past, thus we pass the signature check in the mock and decode a valid chainIdentifier and implementationIdentifier(since we can just copy them from a valid transaction) and also our malicious message.
2. From there we go into _handleMessage. We can set any unused identifier so we bypass the if (messageState) revert MessageAlreadySpent(). We bypass if (expectedSourceImplementationHash != keccak256(sourceImplementationIdentifier)) because the sourceImplementation is the implementationIdentifier from step 1 which we can just copy-paste from a valid previous transaction.
3. We go into ICrossChainReceiver(toApplication).receiveMessage. We bypass the address checks by covering the conditions(length of 20, 44 empty bytes) with the crafted data (Ethernaut flashback), and we go into _handleReceiveLiquidity .
4. In there we go to ICatalystV1Vault(toVault).receiveLiquidity. In there all of the checks would pass if we did not enter some extreme parameters due to the complex math, but we know what parameters would not revert since we can scan past working transaction. And thus, we are able to mint ourselves however much vaultTokens we want on the destination chain. This transaction would not revert, but the ack it produces would not be able to be relayed since there is no real escrow on the source but it does not affect how we get the tokens, it's just a bonus that any ack relay attempt would fail.

The greater impact of the issue is obviously users' ability to just mint themselves vault tokens for free. This is due to the packet processing being a 2 step process - message and ack. We use an arbitrary message to mint the token and an ack to burn them on the source, but these 2 actions are seperate from one another and thus it does not mattter than we do not have the tokens on the source.

If there is a missed check or guard for this, I still believe in LOW due to the existing signature replay which is undoubtfully there. Otherwise this is a HIGH severity issue.

Attachments

1. Proof of Concept (PoC) File Will be included in a later comment, until then I hope the explanation above suffices.

2. Revised Code File (Optional)

mapping(bytes => bool) public metadatas; Then inside _verifyPacket we do: if(metadatas(_metadata)) revert("Signature already used")

Recommendation - quite simple, in the mock implementation you provide, include a mapping from bytes to bool and any time a valid metadata passes the signature check, add it to the mapping as used, thus preventing it's usage. I am unable to find the metadata creation in the code, but if it is done somewhere else, make sure it includes the chainId to prevent cross-chain replay as well.

[reednaa]

Not a dublicate but relevant: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/3

[reednaa]

For simplicity, you don't need to replay a Catalyst swap if you manage to replay any transaction it is enough for high severity.

[PlamenTSV]

Yes #3 is relevant, but I think less than you think. It talks about parameter swap and the usage of ECDSA. There was another one mentioning also a check for address 0, but these all are not valid fixes and do not suffice here. When I come up with the PoC we can discuss it further

[PlamenTSV]

I missed an important check that protects against the above issue, you can mark it as invalid.