Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Factory incentive structure is deeply flawed due to sharing of payoutAmount across all pools #327

Closed
c4-bot-10 opened this issue Mar 4, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_327_group AI based duplicate group recommendation unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

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

Vulnerability details

Description

The new fee collection operates by any Bot / searcher calling claimFees() on the V3FactoryOwner. They pay the global payoutAmount defined in the contract (in WETH) in exchange for receiving the protocol fees from the pool. Note that for each claimFees() call, the fee recipients collectively and implicitly give up the cost of delivering the TX by the searcher. That cost is comprised of the gas expenses, bribing fees for frontrunning and any infrastructure overhead. To give an example, if the bot costs are $20 and payoutAmount is $50, the bot would start being incentivized to call claimFees() when the fee value is $70+. In that scenario, the racer overhead is $20/$70 = ~28% before the racer is even profitable.

It is hopefully clear that the payoutAmount needs to be very big in relation to the racer costs so that a massive chunk of the fees won't go to the race overhead. However, note that payoutAmount is the same for collection of all pools, while the rate of accumulation varies by several orders of magnitude between the pools.
For example, USDC/USDT could make $5,000 per day in fees, while MEME/ETH would make $15 per day.
The existence of wide fee ranges makes choosing a reasonable payoutAmount impossible. The higher it is, the lower the yield paid off for the racer, but it would make it take too long (if ever) for the low-capacity pairs to generate enough fees to make a racer pay for it.
Therefore it's important to be able to set payoutAmount individually for each pool, or have it calculated automatically based on pool trading volume in some way. This way the large pools could be set with a high amount to minimize leak of rewards, while it would be set low for small pools to not block off their fee stream.

Note that the origin fee collection function didn't leak any value out of the protocol fees, the issue is introduced by the upgrade.

Impact

The fee claiming design significantly reduces the yield paid to UNI stakers.

Tools Used

Manual audit

Recommended Mitigation Steps

payoutAmount should be a mapping item per pool which the admin can set.

Assessed type

Other

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 4, 2024
c4-bot-3 added a commit that referenced this issue Mar 4, 2024
@c4-bot-11 c4-bot-11 added the 🤖_327_group AI based duplicate group recommendation label Mar 5, 2024
@c4-judge c4-judge closed this as completed Mar 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Mar 7, 2024

MarioPoneder marked the issue as duplicate of #37

@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Mar 14, 2024
@c4-judge
Copy link
Contributor

MarioPoneder marked the issue as unsatisfactory:
Invalid

@MarioPoneder
Copy link

Hi and thanks for your comment!

I acknowledge that the present core issue (payoutAmount is not adjustable per pool) is indeed different from the primary one (having 1 payoutAmount can lead to stuck fees) although both issues are disputing the same design decision.

A few aspects to consider:

  • The reduced protocol complexity of having only one payoutAmount is clearly a fundamental design decision which I cannot consider as a bug/oversight per se.
  • According to the README, the sponsor is aware about the impact of fee collection issues due to pool fee value vs. payout token value, from leaking value to rarely/never collecting fees. These fluctuations can naturally lead to the payoutAmout being suboptimal which is known/acknowledged.
  • The protocol allows to selectively set fees per pool.
  • Concerning They can no longer perform any other logic for fee collection.:
    The protocol has a way to allow the governance to claim pool fees irrespective of their current value (see comment on previous primary issue).
  • I agree, that a low payoutAmout is unreasonable due to the resulting overhead and it was it was not known to wardens that only high-volume pools are targeted.
    Actually, no assumptions can be made about the payoutAmount except for what we know from the known issues section, which reveals the sponsors awareness about further implications of the choice of payoutAmount.

All in all, I appreciate the value & insights provided by this submission, but cannot justify Medium severity from a judging perspective as we are arguing against a fundamental design decision which was carefully considered.

Thank you for your understanding!

@c4-judge
Copy link
Contributor

MarioPoneder marked the issue as not a duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working 🤖_327_group AI based duplicate group recommendation unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

4 participants