Lack of deadline check in claimFees will result in large losses for caller

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/V3FactoryOwner.sol#L181

Vulnerability details

Description

The new fee claiming method relies on bots / searchers to call claimFees() when it becomes profitable. Callers pass amount0Requested, amount1Requested, and are guaranteed to receive those amounts in fees, in exchange for a provided payoutAmount.

It is important to note that the value of fee tokens is constantly fluctuating in the vast majority of pools (non-stables). A trade could be profitable at time X but heavily losing at time X + Y. Unfortunately claimFees() does not include a deadline parameter similar to most swapping functions like in Uniswap periphery.

Validators can delay the execution of the TX until (amount0, amount1) amounts are available again to be collected, this time with a different evaluation.

Impact

Participants in the claimFees() race can lose funds.

Proof of Concept

• A profitable claimFees() opportunity arises
• Victim searcher calls claimFees()
• Another searcher's claimFees() executes first
• A day passes, the tokens in the pool lose 15% of their value
• Enough fees accrue again to execute claimFees()
• Victim's claimFees() is executed, thereby losing 15% in value.

Tools Used

Manual audit

Recommended Mitigation Steps

Include a deadline parameter to claimFees()

Assessed type

Timing

[c4-judge]

MarioPoneder marked the issue as duplicate of #8

[MarioPoneder]

Selected for report due to best explanation of how a user could be subject to losses due to a missing deadline check.
Edit: Amended decision after further thought.

See #8 for discussion.

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[c4-judge]

MarioPoneder marked the issue as not selected for report

[c4-judge]

MarioPoneder removed the grade

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-b

[MarioPoneder]

Thank you for your comment!

Everyone be assured that my thoughts have circled a lot around this one for the last 48h considering what's a stake. Here are some snippets of my thought process:

• Even honest MEV searchers using the correct pool address, etc. can be affected by this
• The issue is unlikely but not highly unlikely. In the end, it's a possible problem / attack vector and the following statement bears validity:

Validators can delay the execution of the TX until (amount0, amount1) amounts are available again to be collected, this time with a different evaluation.

• I absolutely do not feel pressured because of historical precedent concerning the judgement of similar issues since every issue is unique in the context of its respective contest.
• Deadline checks (similar to slippage checks) are state-of-the art and the protocol bears a responsibility to provide such protection measures for honest users / MEV searchers.
• The current implementation requires users to monitor & cancel dangerously pending transactions which incurs an additional mainnet transaction fee and adds additional complexity by requiring another cancel transaction.

Due the above reasons and given Low likelihood & High impact, Medium severity is justified.

[c4-judge]

This previously downgraded issue has been upgraded by MarioPoneder

[c4-judge]

MarioPoneder marked the issue as selected for report

[c4-judge]

MarioPoneder marked the issue as satisfactory

[MarioPoneder]

Anonymous amicus brief:
https://gist.github.com/CloudEllie/9b8507b8a36a452e5aea7731e5b0d279

[aktech297]

Anonymous amicus brief: https://gist.github.com/CloudEllie/9b8507b8a36a452e5aea7731e5b0d279

Already there is protection from slippage.

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5a2761c8277541a24bc551fbd624413b384bea94/src/V3FactoryOwner.sol#L192-L195

user's already given with an option to protect from unintended losses.

I don't think so deadline check is really needed.

@MarioPoneder pls check

[dmvt]

Summary of the issue

Participants in the claimFees() race can lose funds due to a missing deadline parameter.

gzeon’s view

Similar to issue 45, I consider this is a failure of the user (MEV bot) that calls claimFees. In most cases those bots would use a bundle to ensure the tx is executed with enough gain to pay the proposer. If the failing transaction can be unbundled and replayed later, this is an unbundling attack irrelevant to this audit.

If the failing MEV bot is not using bundle, their transaction would revert onchain and burn the nonce so it won't be replayable either. The warden described scenario is only possible when the transaction is gas underpriced for so long, without a replacement transaction, that the pool accumulated enough fees to be claimed again which is very unlikely.

The function behaved according to spec by returning amount0 and amount1. It is up to the user to value the amount they received as worth the gas cost. If they no longer like that trade, they can always cancel the transaction.

There is an edge case where claimOnBehalf is used and the signature nonce won’t be burnt, in such case I still think the signer is responsible to invalidate the signature manually, maybe Low/QA in that case then.

Picode’s view

I agree with @gzeon's reasoning about the fact that this will very likely be used within a bundle, however compared to #205 we are typically in the case where the outcome of the transaction is time sensitive so I'm more hesitant

I am also not very convinced by the "amicus brief", and I wonder if there is still a case where searchers send this tx for example through flashbot with a low gas price and it gets included later on if they don't cancel it somehow. In this case a deadline would help as these RPCs in theory protect against the inclusion of reverting txs.

To me, it boils down to if we're able to come up with a convincing scenario of "leak value with a hypothetical attack path with stated assumptions" where a searcher does everything correctly and its tx is used against him

LSDan’s view

I'm inclined towards low risk here. Similarly to 205, I think we need to put the burden on the user to cancel their transaction by incrementing the nonce if they no longer want it to go through. In this case we're talking about a more sophisticated than average user. They should know better. I'm not inclined to invalidate it because there is a small but present risk of a loss if everything aligns against the searcher and a chance that the loss would be caught by an expiration. That said, they are playing in an area of the ecosystem where the occasional loss is to be expected, so medium risk is excessive.

Result

Issue #331 will be marked as valid, but low risk / QA.

[MarioPoneder]

Thanks for clarifying the technical validity of this one!
Although I've provided clear reasoning for Medium severity (also as response to the amicus brief) due to the realm of operational conditions allowed by the sponsor's design decisions, there is no directly applicable standardized rule for this finding which could either support or refute that decision.
Therefore, I agree with the presented opinions arguing for QA level, which are mainly based on the assumption that the caller is sophisticated enough to deal with this (MEV bot).
For reference, I've argued similarly on #380 which is a MEV-only issue to begin with.

Summary:
gzeon: QA
Picodes: Med (if convincing scenario)
LSDan: QA

Kudos to the Appellate Court for their reevaluation.

See also: #417 (comment)

[c4-judge]

MarioPoneder marked the issue as grade-a