hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 2 forks source link

Arbitrary external addresses in payload could allow for griefing of unexpecting relayers #61

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

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

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

Description: Description\ The CCI function sendCrossChainLiquidity calls the IME function submitMessage, allowing the caller to specifying any arbitrary parameters, in the case of IME even allowing for an incentive/bounty that's outside of the allowed limit. This could allow the creation of a bounty that when processed could intentionally call external addresses that grief the relayer through gas.

Attack Scenario\ We start from sendCrossChainLiquidity, then our fromVault address would be that of msg.sender, toApplication will be the CCI. Additionally, we would be able to specify the toVault and toApplication to any address we want, which can too be contracts. Our message identifier will surely be valid (we can append dirty calldata just to be sure), so we will get this message signed successfully and put an incentivizing bounty on it. When a relayer sees this(or the structure detects so) bounty, he will naturally try to process it, providing the gas it needs. Thus we reach 2 instances of gas consumption:

  1. _handleMessage -> ICrossChainReceiver(toApplication).receiveMessage{gas: maxGas}. The problem here is that we cannot burn more than maxGas, but still if we create a gas eating function at the toApplication we can easily make a let's say, unbound loop and hit OOG, making the relayer waste his funds.
  2. We can choose to just instantly return an ack here, thus leading the relayer into processing this ack and getting to the CCI's receiveAck function, where we have either ICatalystV1Vault(fromVault).onSendLiquiditySuccess or ICatalystV1Vault(fromVault).onSendLiquidityFailure. But since fromVault is our address, we can choose to force the OOG here. Right here there is no gas limit on the external call, so we can easily eat up the entire block gaslimit and grief the relayer even greater.

I do not see (and am led to believe) there is no way to detect bounties which revert, so relayers have no way of being protected againts gas-draining packets. They can only detect if their incentive is good enough for process. There is also no way to clear such failing bounties, so there would always be relayers getting griefed from attempting to process them.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

As a recommendation, I believe in the CCI sendCrossChainLiquidity and sendCrossChainAsset should be protected by access only from vaults. An admin function which can delete bounties that are failing can deem useful. A sanity check to see if the provided from and to addresses in the messages point to real instances of vaults and CCIs.

reednaa commented 8 months ago

Yes, you can use all of the gas but the relayer will be paid for it. There is no way for you to use gas which the relayer isn't being paid for. (except https://github.com/catalystdao/GeneralisedIncentives/issues/8)

It is true that if an application uses ALL of the allocated gas, then the relayer has to evaluate the bounty carefully.

This is also the case on the ack side.

A relayer can lazily evaluate packages by using the fact that verification cost is roughly constant. Thus the gas used by the application is verficationCost + maxGasDelivery + fixedTXcost and the bounty is maxGasDelivery * gasPriceDelivery.

A better designed relayer can simulate the packages and figure out how much gas the application spends. This can be done by forking the network and calling the application by faking the Incentive contract. This also allows you to get the ack and you can do a similar simulation on the source chain.

Let me know if I misunderstood something.

PlamenTSV commented 8 months ago

So in order to accidentally avoid wasting gas on a malicious packet, relayers should verify each packet on a forked enviroment? Even if that was the case, I believe this behavior should have been given to us as to avoid such speculations. Being able to set the external addresses screams vulnerability, this is why I reported this in the first place.

reednaa commented 8 months ago

The latter is an example of how to optimise for what you described not a "solution". The former is the solution.

You havn't convinced me that the current implementation and documentation is insufficient given common sense. (see everything but last portion of my previous comment)

reednaa commented 8 months ago

Specifically: If a relayer assumes they will get paid for all gas an application spends but nothing else then this wouldn't be an issue. We actually provide a slighly stronger incentive: A relayer is paid for all gas an application spends AND if there is unspent gas they are also paid for verification.