MEV bots can be tricked with fake pools

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/main/src/V3FactoryOwner.sol#L190

Vulnerability details

Impact

There will be two types of MEV users that will call the claimFees function:

1. MEV searchers who keep track of a list of pools and call claimFees on any of those pools when it is profitable. These users are not the target of this issue.
2. MEV bots that check the mempool, fishing for profitable claimFees calls from other MEV searchers or bots. These are the main targets.

Anyone can trick MEV bots into calling the claimFees function with a fake pool, as the function doesn't check if it is a legit pool.

This can result in:

• Potential loss of funds for MEV users
• Less participation from MEV bots that will result in rewards being less liquid/distributed less often
• Loss of reputation for the protocol

Proof of Concept

MEV bots can be tricked with the following steps:

1. Bob owns MaliciousPool, a IUniswapV3PoolOwnerActions pool that he controls. It is a legit USDC/USDT pool (it can be any credible token), but Bob controls this pool and he can deactivate it at will.
2. Bob calls claimFees with MaliciousPool as a target and provides a small amount of gas to this TX.

Now, two scenarios may happen.

1st scenario: No one replicates his TX to get the MaliciousPool fees. In this case:

3a. Bob frontruns his own TX, and he disables MaliciousPool (e.g. he calls a MaliciousPool function so that any subsequent calls to MaliciousPool.collectProtocol fail) so that the TX on point 2 will revert. This is to avoid wasting the payoutAmount that would be sent in case it succeeds. Bob will try again with a different token combination and/or a higher amount of fees.

or, 2nd scenario:

3b. Another MEV user, Alice, replicates Bob's profitable transaction (which is simulated locally, and Alice can see that fees are transferred to the _recipient), and she frontruns the TX on point 2, changing the _recipient to her address.

4. Bob now frontruns Alice so that fees are not really transferred when collectProtocol is called. _amount0 and _amount1 will be correct to avoid reverting during the slippage protection check, but the pool won't transfer any fees.

Supposing the MEV user doesn't have a slippage check themselves they will lose money, as only the return value of _pool.collectProtocol is validated inside V3FactoryOwner.

This slippage protection check is misleading as the asset balance is not checked afterward (i.e. _pool.collectProtocol return value can differ from the actual amount that was trasferred):

    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);

    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
      revert V3FactoryOwner__InsufficientFeesCollected();
    }

5. Finally, Bob backruns Alice and frontruns his own transaction, (same action as point 3a) to avoid paying payoutAmount to the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Consider keeping track of authorized pools, and let bots choose to execute the claim function only when a pool is authorized, for example:

      function claimFees(
        IUniswapV3PoolOwnerActions _pool,
        address _recipient,
        uint128 _amount0Requested,
        uint128 _amount1Requested,
+       bool onlyAuthorizedPool
      ) external returns (uint128, uint128) {
+       if (onlyAuthorizedPool && !authorizedPools[address(_pool)]) {
+         revert V3FactoryOwner__PoolNotAuthorized();
+       }
        PAYOUT_TOKEN.safeTransferFrom(msg.sender, address(REWARD_RECEIVER), payoutAmount);
        REWARD_RECEIVER.notifyRewardAmount(payoutAmount);
        (uint128 _amount0, uint128 _amount1) =
          _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested);

        // Protect the caller from receiving less than requested. See `collectProtocol` for context.
        if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
          revert V3FactoryOwner__InsufficientFeesCollected();
        }
        emit FeesClaimed(address(_pool), msg.sender, _recipient, _amount0, _amount1);
        return (_amount0, _amount1);
      }

As alternative, consider checking the delta balance of each pool token before and after _pool.collectProtocol is called, instead of checking its return value.

Assessed type

MEV

[c4-judge]

MarioPoneder marked the issue as primary issue

[MarioPoneder]

On the one hand this is user error when MEV searchers fall for this. But on the other hand the protocol has a responsibility protect its users (including good faith MEV searchers), especially when it can be mitigated so easily by the protocol.
Leaning more towards QA, but leaving as it is for now.

[c4-sponsor]

apbendi (sponsor) disputed

[apbendi]

The scenario described here is both unrealistic and outside the scope of our contracts to mitigate. To briefly summarize only a few of the reasons:

1. It's highly unlikely a valid searcher call to claim fees would ever land in the mempool, because such a transaction would be immediately frontrun, and searchers know this. All valid claims are likely to go directly to block builders via MEV providers like Flashbots.
2. Any MEV searcher can see what pools have been activate or not by observing onchain data.
3. Any MEV bot that tries to snipe profitable transactions out of the mempool is at risk from carefully crafted attacks like the one described—no amount of protections can be put in place to prevent all of these from happening.
4. The attacker would have to set up the full Uniswap V3 infrastructure (a factory, a pool with liquidity, etc...) to have a chance to carry out this attack, and configure that infrastructure to point toward our contracts as the factory owner. All of this would require a large amount of capital at risk for a very low probability of a very small reward.

There are many more reasons why we find scenario to be unlikely, and why we believe mitigating it is outside of the scope of our contracts anyway, but this briefly summarizes the biggest reasons.

[MarioPoneder]

The sponsor's points are solid.
Nevertheless, I'll refrain from invalidation and downgrade to QA due the proposed mitigation measures.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-a

[DadeKuma]

First of all, thank you, everyone, for the in-depth replies. I still believe that this is a serious issue that should be mitigated by the sponsor, as it can seriously harm users. I will explain why this is the case.

Why would anyone want to trick MEV users?

The answer is simple: stakers have a high financial incentive to do so. That's because all the funds stolen from the MEV users will be redistributed as a reward to the stakers.

The higher their stake, the more likely a user would want to perform this attack, as they would receive a higher reward.

Why is this scenario not unrealistic?

I will explain by citing the sponsor's reasons, one by one:

1) It's highly unlikely a valid searcher call to claim fees would ever land in the mempool, because such a transaction would be immediately frontrun, and searchers know this. All valid claims are likely to go directly to block builders via MEV providers like Flashbots.

I will describe three possible scenarios:

• 1st scenario: I expect that at least in the early period after deployment, the function will be actually called by "normal" users, and not only MEV users/bots. Building a MEV bot tailored for a specific contract takes a lot of time for developing and testing.

So, in the meanwhile, it will be called by any user, as it's profitable to do so. 85% of all transactions use the public mempool, so it makes sense for already existing MEV bots to copy these transactions to snipe those profits.

• 2nd scenario: let's suppose that the following statement is true:

All valid claims are likely to go directly to block builders via MEV providers like Flashbots.

This is an interesting game theory problem. Private transactions are slower than public ones. A MEV bot could use the previous statement to get an advantage in terms of speed if everyone uses private transactions. If they submit their TX into the public mempool, no one will frontrun them, as they would assume that it won't be a valid claim, and the bot will easily win the reward.

• 3rd scenario: let's suppose that the statement described in scenario 2 is false. In this case, what is described in this issue is highly likely to occur.

2) Any MEV searcher can see what pools have been activate or not by observing onchain data.

Like I said and described in the Impact section, this issue targets a specific subset of MEV users: MEV bots that snipe profitable transactions. These users simply replicate and frontrun transactions; they are not aware of the context.

3) Any MEV bot that tries to snipe profitable transactions out of the mempool is at risk from carefully crafted attacks like the one described—no amount of protections can be put in place to prevent all of these from happening.

MEV bots are indeed vulnerable to this attack. But I disagree with the latter part; there is a very simple mitigation that would ensure that this attack is impossible to carry out.

A duplicate of this issue suggests using IUniswapV3PoolOwnerActions.getPool as a form of validation.

If we use a wrong combination of input, the function would return the zero address, as no valid pool exists. The mitigation would look like this:

  function claimFees(
-   IUniswapV3PoolOwnerActions _pool,
+   address tokenA,
+   address tokenB,
+   uint24 fee,
    address _recipient,
    uint128 _amount0Requested,
    uint128 _amount1Requested
  ) external returns (uint128, uint128) {
+   IUniswapV3PoolOwnerActions _pool = FACTORY.getPool(tokenA, tokenB, fee);
+   if(_pool == address(0))
+       revert V3FactoryOwner__InvalidPool();
    ...
  }

4) The attacker would have to set up the full Uniswap V3 infrastructure (a factory, a pool with liquidity, etc...) to have a chance to carry out this attack, and configure that infrastructure to point toward our contracts as the factory owner. All of this would require a large amount of capital at risk for a very low probability of a very small reward.

I disagree with this statement. The only requirement for the attacker is to have a contract that implements the collectProtocol function; it's not necessary to deploy the entire Uniswap V3 infrastructure to trick MEV bots.

There are many more reasons why we find scenario to be unlikely, and why we believe mitigating it is outside of the scope of our contracts anyway, but this briefly summarizes the biggest reasons.

• This issue targets a valid and expected group of users
• This function is part of the scope for the contest
• This issue is not considered out-of-scope in the Known Issues section in the README

For these reasons, I believe this issue is in-scope for this contest.

Why is this not a user mistake?

Citing the Supreme Court verdict for the definition of user mistake:

Findings that require the user to be careless or enter the wrong information into a contract call are QA or Invalid. Non-privileged users are expected to preview their transactions to protect against input mistakes. Anyone using modern tooling will have previews built into their toolset.

• The MEV user provides the right information into the contract call, and they replicate only transactions where pools use legitimate (and valuable) tokens.
• They are not careless, as they preview their transaction, which runs as expected (and it's profitable), so they execute it. The problem is that this is not enough with the current implementation, as the behavior can change when their transaction is in midflight, and the protocol offers no way to protect against this.

The only way to protect against this behavior for the user is to deploy a smart contract and bundle this transaction with a slippage check executed on the balance itself. I don't think this is reasonable, and I will offer this counterexample to prove it:

Imagine a scenario where a project offers a swap without any slippage protection; users provide the correct data, but they can be frontrun nevertheless, which will result in a loss of funds.

Surely, they could deploy a smart contract and interact with the project's contract by adding their own slippage protection on top of the swap call, if the project does not provide it. But is it considered reasonable from a security standpoint? Personally, I don't think so. From a competitive contest perspective, these types of issues are usually considered valid, and not a user error.

In conclusion

1) This function is essentially made to be used by MEV users 2) MEV frontrunning is very common and expected in this case 3) The protocol doesn't protect against the behavior described in this issue 4) This issue can cause a large loss of funds to MEV bots 5) Stakers are incentivized to trick MEV users, as they will be rewarded in case they succeed

For these reasons, given the high impact (total loss of funds for MEV bots) and low likelihood (the attack is difficult to craft and perform), I think Medium severity is the most appropriate for this issue.

[Haxatron]

There are 2 types of MEV: i) Honest MEV that discovers pool that have high enough fees worth claiming ii) Dishonest MEV that copies the tx.

Why should we care about dishonest MEV? And its common for dishonest MEV to get tricked in other protocols anyway.

[quentin-abei]

That's because all the funds stolen from the MEV users will be redistributed as a reward to the stakers.

By doing so attacker is also rewarding other stakers because rewarda will evenly get sistributed to all stakers. Nobody will do this unless he is "robin des bois".

[DadeKuma]

Why should we care about dishonest MEV? And its common for dishonest MEV to get tricked in other protocols anyway.

What you are probably referring to is sandwich MEVs that extract value from users due to slippage, which is not the case here.

This is an auction between MEVs: the faster one wins the fees accrued; that's how MEV works.

A faster liquidation of rewards is actually highly beneficial for users. I honestly don't see how they would be considered malicious/dishonest.

By doing so attacker is also rewarding other stakers because rewarda will evenly get sistributed to all stakers. Nobody will do this unless he is "robin des bois".

Respectfully, but the protocol doesn't work in this way.

The reward potential for a staker depends on how much they stake. Rewards are not distributed uniformly among all users.

They would not care about other stakers, as long as it is financially profitable for themselves.

[quentin-abei]

Like I said and described in the Impact section, this issue targets a specific subset of MEV users: MEV bots that snipe profitable transactions. These users simply replicate and frontrun transactions; they are not aware of the context.

Warden is suggesting to protect MEV frontrunners. Why would the protocol protect frontrunners? It's frontrunners responsibility to check the tx outcome themselves. And as such this issue is user own error

[MarioPoneder]

Thank you for all the comments!
First of all, kudos to the Wardens, this an amazing finding regardless of how it is to be judged in the context of the current contest.

We can expect 2 types of MEV bots:

1. Honest searchers which are designed to work with the UniStaker Intrastructure. In their case, I do not see any risk since the pools' addresses are deterministic and I consider interaction with fake pools an external risk and user error.
2. MEV bots that frontrun every transaction for profit. It would be astonishing if such MEV opportunists, that are not aware of the context , didn't emend their calls into a contract which performs reasonable slippage/profit checks. In the present highly competitive MEV environment, they'd constantly become a victim to such tricks/attacks. Furthermore, I also consider this an external risk / user error and cannot reasonably defend Medium severity from a judging perspective although I highly appreciate this finding from a personal perspective.

Thank you for your understanding!

[DadeKuma]

@MarioPoneder Lack of context applies for generic MEVs; these wouldn't be users but external actors, so I see your point here.

But I think some Uniswap users may decide to monitor this specific contract, as it seems to be a viable strategy to become the race winner by frontrunning, rather than polling the price of every single pool to ensure a profit, which is much more complex.

I don't believe these users are being dishonest, and they can fall victim to what I've described here.

So hopefully, the sponsor decides to fix this issue regardless of the judging outcome.