No access control on IncentivzedMessageEscrow's ``submitMessage`` can lead to a short-term dos of users

[hats-bug-reporter[bot]]

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

Description: Description\ The submitMessage function of the escrow is a follow up function when a user sends assets/liquidity cross-chain and is meant to set the message's bounty/reward for the relayer and to send the packet of data. Due to it lacking access control and some input validation, it is vulnerable to several vectors.

Attack Scenario The first scenario that comes to mind:

1. User A decides to send some assets cross-chain, thus he initiates a TX using the current pool settings (prices, U, weights, block number, etc)
2. User B/MEV bot notices the mempool transaction and decides to front-run the original TX by directly calling submitMessage
3. His ability to directly do so allows him to do the following: craft the exact same destinationIdentifier, destinationAddress and message since the encoding they need to be made is known thus can be replicated (for the message he would simply replace the msg.sender with the address of the front-runned user) ; provide any kind of incentive he wants, due to the missing checkRouteDescription modifier that validates the incentives ; set the bounty of the front-runned message to his arbitrary incetive, since the identifier relies only on the destination and the message itself which are both crafter maliciously.

The above scenario has a few limitations which are the reasons I give it an initial LOW severity:

1. Yes, we can block an incoming message, but it is only for the current block since the encoding uses block.number thus a user can retry again next block.
2. User funds do not get stuck, because the escrow creation and submitting of the message are a part of the same call chain which would revert.
3. Cost of the attack is unknown, due to differing implementations. If the cost is very low even on one of the supported chains, I am willing to argue for MEDIUM since MEV can do this for EVERY MESSAGE

The current impacts, disregarding the potential MEV spam are:

1. Short-term DoS of 1 block
2. The maliciously created and potentially overinflated bounty is unremovable, since any attempts to process it would revert. This means relayers could easily get tricked into attempting to claim it's rewards and wasting their gas on doing so.

I would love if the sponsors could comment what the wormhole's message fee and the mock's cost of messages would be so we can discuss further severity/impact. Currently LOW is fine IMO Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

The recommendation is quite simple in 2 modifiers: onlyChainInterface and checkRouteDescription

[reednaa]

Mock is not intended for mainnet so it shouldn't be based its parameters. Wormhole's fee is currently free but exepected to be non-zero in the future.

I do not agree with the fix. The fix is to either add msg.sender or msg.origin to https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L81.

We will figure out severity.

[PlamenTSV]

That would be a partial fix imo, because he would still be able to create a bounty of any incentive size, be it incredibly large. Thus he can still trick any relayer into wasting gas attempting to process it. I believe at least the incentive should be validated as well.

[reednaa]

But then the only person who could DoS you would be yourself.

Relayers are aware of the incentive and won't process messages with a too low incentive.

[PlamenTSV]

Yes but they would attempt to relay those with high incentives, which we can create, even above the max limit due to the missing validation check. And I do not know if the off-chain system has a way to indicate to relayers which bounties have been failed or are unclaimable, so any relayer could get tricked.

[reednaa]

Sorry, I don't understand that case.

It is very likely that there is going to be a perpetual number of bounties active since the incentives have been set too low. However, there are not going to cause any problem since they do not block other messages. Relayer's also doesn't care about these but will generally just discard them from their list until they see increaseBounty.

[PlamenTSV]

A few key notes that come to mind for when time for judging comes:

1. In our message we can set any block.number we want, so we can DoS future messages as well
2. Since the incentive is not checked because of the direct call to submitMessage we can set our incentive to be with 0 values for the relay prices, making us pay no fees (because wormhole is free per the above comments). So the attack is pretty much automatable for MEV and a greater scale of DoS.
3. The fromApplication is set to msg.sender but that can be our own contract we do the attack from. toApplication is set inside the initial message we have crafted. Thus even if there is a price in the future, if we process our own message we used for the DoS and just return successful ack's from our contract, we can reclaim our own bounty, thus making this attack free again.

[reednaa]

I think you misunderstood how the inputs work. https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/apps/mock/IncentivizedMockEscrow.sol#L33-L45

1. The only tainted inputs here is message and destinationIdentifier. The other are only soft-tainted but cannot be controled in the past or the future. block.number is uncorrolated of message in the hash.

3. Aware. That is why the fix is to add msg.sender to the message identifier.

[PlamenTSV]

4. https://docs.arbitrum.io/for-devs/concepts/differences-between-arbitrum-ethereum/block-numbers-and-time#example greater impact on Arbitrium, due to how the block number is derived and updated.

[reednaa]

We have decided to classify this issue as Medium. Our decision is based on the following arguments:

• There is a real risk that anyone could DoS a user.

The proposed fix is bad. msg.sender will be added to the message identifier. It is then up to the application to ensure that they themselves do not DoS users.