search

code-423n4 / 2024-02-uniswap-foundation-findings

2 stars 3 forks source link

Claiming all fees collected will revert because of not clearing slot check #34

Open c4-bot-4 opened 9 months ago

c4-bot-4 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

Stakers earn yields when someone claims the Uniswap pool fees and pays the payoutAmount WETH, and if the amount of tokens either the first pair or the second is less than the amount requested by the user the transaction reverts.

V3FactoryOwner.sol#L181-L198 

  function claimFees(IUniswapV3PoolOwnerActions _pool, address _recipient, uint128 _amount0Requested, uint128 _amount1Requested) external returns (uint128, uint128) {
    ...
    (uint128 _amount0, uint128 _amount1) =
      _pool.collectProtocol(_recipient, _amount0Requested, _amount1Requested); // @audit Claiming pool fees

    // Protect the caller from receiving less than requested. See `collectProtocol` for context.
    if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) { // @audit check that the amount received is not smaller than requested
      revert V3FactoryOwner__InsufficientFeesCollected();
    }
    ...
  }

This check protects the caller of the function from getting less amount than expected, as he pays WETH to take these fees. But if the caller requests to claim all protocol fees, which is the best case for him to earn the max profit, the transaction will get reverted, as the receiving amount will get subtracted by 1 when calling pool::collectProtocol() with the amount*Requested == protocolFees.token*.

v3-core/UniswapV3Pool.sol#L848-L868

    function collectProtocol(address recipient, uint128 amount0Requested, uint128 amount1Requested) external override lock onlyFactoryOwner returns (uint128 amount0, uint128 amount1) {
        amount0 = amount0Requested > protocolFees.token0 ? protocolFees.token0 : amount0Requested;
        amount1 = amount1Requested > protocolFees.token1 ? protocolFees.token1 : amount1Requested;

        if (amount0 > 0) {
            // @audit the amount received will get subtracted by 1 if it is all fees collected
            if (amount0 == protocolFees.token0) amount0--; // ensure that the slot is not cleared, for gas savings
            protocolFees.token0 -= amount0;
            TransferHelper.safeTransfer(token0, recipient, amount0);
        }
        if (amount1 > 0) {
            // @audit the amount received will get subtracted by 1 if it is all fees collected
            if (amount1 == protocolFees.token1) amount1--; // ensure that the slot is not cleared, for gas savings
            protocolFees.token1 -= amount1;
            TransferHelper.safeTransfer(token1, recipient, amount1);
        }

        emit CollectProtocol(msg.sender, recipient, amount0, amount1);
    }

So the transaction will get reverted and claiming all protocol fees from either the first or the second token will get reverted.

Since the caller pays the payoutAmount to claim the fees, the caller will simply request to take all fees from both tokens. And there are MEVs (Uniswap may have one) that will track the amount of fees collected and compare it to the payoutAmount, and call the function by reading the values from the pool itself (to claim all the fees). So firing the function with the maximum fees taken by the pool will be the default behavior, and it will get reverted as we illustrated.

Proof of Concept

We created a Foundry test that simulates claiming fees from a real uni-V3 pool, here is how to set it up.

  1. Add the word virtual to test/mocks/MockUniswapV3Pool.sol::collectProtocol()
  2. Add the next contract in test/V3FactoryOwner.t.sol
    Contract
...
import {MockUniswapV3Factory} from "test/mocks/MockUniswapV3Factory.sol";

// Add the following contract after the upper line
contract Auditor_MockUniswapV3Pool is MockUniswapV3Pool {

  // UniswapV3 pool `collectProtocol()` function
  function collectProtocol(address /* recipient */, uint128 amount0Requested, uint128 amount1Requested)
    external
    override
    returns (uint128, uint128)
  {
    uint128 amount0 = amount0Requested > mockFeesAmount0 ? mockFeesAmount0 : amount0Requested;
    uint128 amount1 = amount1Requested > mockFeesAmount1 ? mockFeesAmount1 : amount1Requested;

    if (amount0 > 0) {
        if (amount0 == mockFeesAmount0) amount0--; // ensure that the slot is not cleared, for gas savings
        mockFeesAmount0 -= amount0;
        // TransferHelper.safeTransfer(token0, recipient, amount0);
    }
    if (amount1 > 0) {
        if (amount1 == mockFeesAmount1) amount1--; // ensure that the slot is not cleared, for gas savings
        mockFeesAmount1 -= amount1;
        // TransferHelper.safeTransfer(token1, recipient, amount1);
    }

    return (amount0 , amount1);
  }
}

  1. Add this following script in V3FactoryOwner.t.sol after testFuzz_RevertIf_CallerExpectsMoreFeesThanPoolPaysOut test
Testing Script ```solidity contract ClaimFees is V3FactoryOwnerTest { ... Auditor_MockUniswapV3Pool auditor_pool; function test_auditor_claimMaximumFeesReverts() public { auditor_pool = new Auditor_MockUniswapV3Pool(); vm.label(address(pool), "Pool"); uint256 payoutAmount = 1e18; // paying 1e18 to claim reward address caller = makeAddr("caller"); // The caller of the `claimFee` address receipent = makeAddr("receipent"); // The receipent of the pool token pairs fees uint128 amount0 = 0.5e18; // first token pait fees received uint128 amount1 = 0.5e18; // second token pait fees received _deployFactoryOwnerWithPayoutAmount(payoutAmount); payoutToken.mint(caller, payoutAmount); // Updating fees to 0.5e18 for each token pair auditor_pool.setNextReturn__collectProtocol(0.5e18, 0.5e18); vm.startPrank(caller); payoutToken.approve(address(factoryOwner), payoutAmount); console2.log("protocolFees.token0:", auditor_pool.mockFeesAmount0()); console2.log("protocolFees.token1:", auditor_pool.mockFeesAmount1()); console2.log("amount0Requested:", amount0); console2.log("amount1Requested:", amount1); // vm.expectRevert(V3FactoryOwner.V3FactoryOwner__InsufficientFeesCollected.selector); factoryOwner.claimFees(auditor_pool, receipent, amount0, amount1); console2.log(""); console2.log("Call reverted"); vm.stopPrank(); } } ```
  1. Run the script forge test --mt test_auditor_claimMaximumFeesReverts

Output:

  protocolFees.token0: 500000000000000000
  protocolFees.token1: 500000000000000000
  amount0Requested: 500000000000000000
  amount1Requested: 500000000000000000

  // Calling Factory::claimFees() ...

  [FAIL. Reason: V3FactoryOwner__InsufficientFeesCollected()]

    ├─ [45140] Factory Owner::claimFees(Auditor_MockUniswapV3Pool: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], receipent: [0x0a4d4851029426cF1CC7AFa6E8031D5b4BeA2Be2], 500000000000000000 [5e17], 500000000000000000 [5e17])
    │   ├─ [20686] Payout Token::transferFrom(caller: [0xA5cd91e65Fb56f2f6bD848E546B259249c6F1695], Reward Receiver: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1000000000000000000 [1e18])
    │   │   ├─ emit Transfer(from: caller: [0xA5cd91e65Fb56f2f6bD848E546B259249c6F1695], to: Reward Receiver: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], value: 1000000000000000000 [1e18])
    │   │   └─ ← true
    │   ├─ [22290] Reward Receiver::notifyRewardAmount(1000000000000000000 [1e18])
    │   │   └─ ← ()
    │   ├─ [2578] Auditor_MockUniswapV3Pool::collectProtocol(receipent: [0x0a4d4851029426cF1CC7AFa6E8031D5b4BeA2Be2], 500000000000000000 [5e17], 500000000000000000 [5e17])
    │   │   └─ ← 499999999999999999 [4.999e17], 499999999999999999 [4.999e17]
    │   └─ ← V3FactoryOwner__InsufficientFeesCollected()
    └─ ← V3FactoryOwner__InsufficientFeesCollected()

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

Make 1 wei tolerance when checking for the received value in V3FactoryOwner::claimFees()

  function claimFees( ... ) external returns (uint128, uint128) {
    ...

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

    ...
  }

Assessed type

Uniswap

c4-judge commented 8 months ago

MarioPoneder marked the issue as primary issue

c4-sponsor commented 8 months ago

apbendi (sponsor) confirmed

c4-sponsor commented 8 months ago

apbendi marked the issue as disagree with severity

apbendi commented 8 months ago

This is an issue that, at a minimum, warrants an update to the comments for clear documentation. However, even if deployed exactly as-is, the protocol would function, and an MEV searcher developing a bot would have likely seen the issue an accounted for it during testing. No funds were at risk, nor was the protocol's availability. We consider this a Low Severity issue.

MarioPoneder commented 8 months ago

The likelihood of the present issue is High as nearly every MEV searcher will eventually run into this problem when claming fees and has to figure out the cause, which was neither documented not excepted at the time of this audit contest (source of truth).
The impact is Low since it's just a temporary DoS scenario until the MEV searcher has adapted to that.
In the end, it's more than a simple UX issue and natural for MEV searchers to try claiming the maximum fee amounts.
Furthermore, given the very High likelihood despite Low impact, an overall Medium severity is justified,

c4-judge commented 8 months ago

MarioPoneder marked the issue as satisfactory

c4-judge commented 8 months ago

MarioPoneder marked the issue as selected for report

DadeKuma commented 8 months ago

The impact is Low since it's just a temporary DoS scenario until the MEV searcher has adapted to that.

I think there is a small misconception about this issue: it won't cause a DoS. Users are free to request 1 wei less themselves, even in the same block. I do not think that leaving a 1 wei reward on the table can be considered a reasonable loss.

The Sponsor is correct. No funds are at risk (this applies to both the protocol and the users), and the protocol's availability is not affected.

This is a user error, as the user should have tested and simulated their transaction before executing it:

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.

But even if they execute this transaction, it will simply revert. They could retry immediately with a smaller _amount0Requested/_amount1Requested. There isn't any impact here.

Due to these reasons, I believe this is a low-severity issue, as it simply improves the UX for the users.

Haxatron commented 8 months ago

The impact is not limited to a DoS. It will waste the MEV gas fees which are expensive in mainnet and the MEV will lose the auction.

quentin-abei commented 8 months ago

It will waste the MEV gas fees which are expensive in mainnet and the MEV will lose the auction.

This impact was never stated in the original submission and moreover the report should be labelled "Gas-saving" and classified as such .

SovaSlava commented 8 months ago

Agree with other wardens, that this issue looks like gas-saving issue or maybe qa. Its exactly not medium.

The text describes only a feature of the protocol, not a vulnerability

d3e4 commented 8 months ago

The premise of this issue is false or OOS.

This issue is based on the assumption/expectation of how much can be claimed. V3FactoryOwner is completely agnostic about this amount. The caller would therefore have to refer to the UniswapV3Pool, and should discover that the maximum claimable amount is protocolFees.tokenX - 1. If this is unclear enough to be considered an issue, then it pertains to UniswapV3Pool, which is OOS for this audit.

About half of the duplicates of this issue were reported as QA, and I am certain many more thought of this very issue but chose not to report it (myself included), assessing it as QA/OOS, for the above reasons.

merlinboii commented 8 months ago

As I reported this issue as QA, this issue should be considered of Low severity.

Before collecting the fee, fee collectors (EOA/MEV) should understand the details of the maximum claimable amount when making fee claims.

Therefore, this issue only presents ambiguity regarding the claimed amounts due to the absence of documentation outlining this specific point.

PFAhard commented 8 months ago

As many have stated the impact is overinflated for the reasons listed above, I just want to add that in all test cases written by sponsor teams, they subtract 1 from the fee, which leads to the conclusion that the problem is known to the sponsor and accepted before the contests start.

KuTuGu commented 8 months ago

This is not only a DOS problem, as described in #372 , when multiple MEV searchers auction, due to transaction execution timing issues, tx executed with low gas delay may be executed normally, and the auctioneer pays high gas but loses the auction, low gas wins the auction, which affects the auctioneer's income.

Al-Qa-qa commented 8 months ago

I want to answer some points that are mentioned by some wardens, related to the issue, its severity, and its validity.

1. The likelihood of the issue

the caller will simply request to take all fees from both tokens. And there are MEVs (Uniswap may have one) that will track the amount of fees collected and compare it to the payoutAmount, and call the function by reading the values from the pool itself (to claim all the fees).

So the value requested will be all fees collected to get the maximum profit, and as we said MEW will track the amount of fees collected, and it will read the value from the pool itself. which makes calling the function with all fees is the command case and most of the callers will call it with the max fees collected.

So as the judger said the likelihood of this issue is HIGH.

2. Not stating the loss of gas of the MEV (caller)

And reverting tx consumes gas (most of us know this point). And since the context is an MEV, the gas provided will be HIGH, and again all of us know that MEV transactions are subjected to race-condition, which means the provided gas is HIGH. Besides that, the judger and the Sponsor did not dispute this point.

3. Can this lead to a DoS, And how long will it remain?

The DoS is temporal for MEVs that call the function by providing the value itself, and this is true. But other MEVs will be in permanent DoS, which are the MEVs that are designed to read the fees value before calling.

call the function by reading the values from the pool itself (to claim all the fees)

So the MEVs that are designed as smart contracts to read the values before calling (to extract the maximum value), will be in permanent DoS not temporal.

These MEVs will be designed to provide a check of the profit, as they will depend on always passing the notifier check.

Conclusion and Final Thoughts

Likelihood is HIGH, severity bounds between MEDIUM and LOW, MEDIUM severity is fair.

Haxatron commented 8 months ago

To further add on to Al-Qa-Qa point, a DoS will lead to fees being accrued in the pools being unclaimed and that will lead to a loss of funds for staker.

quentin-abei commented 8 months ago

To further add on to Al-Qa-Qa point, a DoS will lead to fees being accrued in the pools being unclaimed and that will lead to a loss of funds for staker.

This is a blatant attempt to further inflate the issue which was never stated in the first place. I still do not see how this can be of any other severity than "gas" .

Haxatron commented 8 months ago

Dear @quentin-abei, there is no "attempt" to do anything here. What I am stating is a fact, the Unistaker model relies on MEV bots to claim the fees as fast as possible, if there is a temporal DoS then the fees will accrue without the staker being rewarded. How much fees will accrue without being claimed? I am not sure entirely, but I will leave it to the judge to determine the final ruling.

And the severity you are talking about is "gas optimization" which refers to optimizing smart contracts for gas.

osmanozdemir1 commented 8 months ago

I also would like to add a comment as the author of the #45

The whole reward logic is about creating a race between MEV bots. The result of this bug is not simply losing 1 wei, it is losing the race, which is losing all possible profits in that pool and losing gas during transaction, just because of 1 wei.

I disagree this being user's fault. It is an integration issue between two parts of Uniswap. UniStaker is not integrated with UniCore in a seamless way. Uniswap itself introduces 1 wei decrement to save gas on their pool, and the Uniswap itself should match with this behaviour in their Unistaker part.

Kind regards.

d3e4 commented 8 months ago

As many have stated the impact is overinflated for the reasons listed above, I just want to add that in all test cases written by sponsor teams, they subtract 1 from the fee, which leads to the conclusion that the problem is known to the sponsor and accepted before the contests start.

This is a good point. Add to that the fact that the comment over the slippage check in claimFees() explicitly says "See collectProtocol for context.". Add also the fact that the MEV searcher absolutely must weigh the gas costs of his operation. That is, he must look into both claimFees()and collectProtocol(), whether by inspection or simulation. I am therefore not convinced he is highly likely to miss the - 1 after all. In any case it is only a matter of documentation. The code itself is fully valid and functional as it is, and any suggested change is a design proposal rather than a mitigation of a bug.

apbendi commented 8 months ago

We strongly disagree with this issue being classified as a Medium Severity. Other wardens have done a good job laying this out, but to summarize briefly:

  1. This is not a Denial of Service attack. Calling it that means either the issue is misunderstood, or the meaning of DoS is misunderstood. The only situation in which this would play out is one in which an insufficiently tested MEV bot submitted a transaction that would revert because of the argument values they passed in. At such a time, any other bot would still be able to claim the fees if their arguments were correct. At no point is "service denied", nor are any funds at risk other than those of the insufficiently tested MEV bot.
  2. We knew that 1 wei needed to be subtracted, as is demonstrated clearly in our own integration tests. Whether our code should internally account for this, or it should be left to the caller, is a subjective discussion. We opted for the latter. We agree it ought to be better documented.

At most we believe this could be called a low severity issue, and even here we believe that is a generous characterization. Others have made the case it should be called QA or gas savings, and their arguments are reasonable.

MarioPoneder commented 8 months ago

Thanks for all the comments with alternatingly opposing opinions, that's what PJQA is about!

Please consider the following aspects:

Consequently, Medium severity will be maintained.

c4-judge commented 8 months ago

MarioPoneder marked issue #45 as primary and marked this issue as a duplicate of 45

d3e4 commented 8 months ago
  • Trying to claim max. possible fees, which is reasonable, will result in a revert and cannot be considered a user error.

@MarioPoneder, this is the core of the argument, which is otherwise sound and correct. But what is the definition of "max. possible fees"? Why is it correct to say it is protocolFees.tokenX rather than the maximum amount claimable according to collectProtocol()? I find no documentation on this, and therefore no assumptions can be made outside of the code itself. I agree it reasonably should have been protocolFees.tokenX, but that is what it should have been in collectProtocol(), which is OOS.

The only way to "fix" this issue is to allow a 1 wei slippage. But this just propagates the exact same integration issue to the next contract down the line interacting with V3FactoryOwner, expecting no less than _amountXRequested. Note also that collectProtocol() reduces any excessive amountXRequested to protocolFees.tokenX - 1. So a user might also expect to be able to claim max fees by using type(uint256).max. Should claimFees() therefore do the same and thus allow any slippage? Why should V3FactoryOwner incorporate a hidden slippage, and thus sacrifice its own integrity, to smooth out a quirk in UniswapV3Pool, only to transfer the same integration issue to the next contract?

c4-judge commented 8 months ago

MarioPoneder marked the issue as grade-b

c4-judge commented 8 months ago

MarioPoneder marked the issue as grade-a