The claimFees caller attempting to claim all protocol fees is reverted

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/V3FactoryOwner.sol#L193-L195
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L856-L865

Vulnerability details

Description

The claimFees function is tasked with claiming protocol fees by paying out PAYOUT_TOKEN instead of fees collected by the pool. It includes validation to protect the caller from receiving less amount of tokens than what was requested.

if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
    revert V3FactoryOwner__InsufficientFeesCollected();
}

Due to the UniswapV3Pool collecting protocol fees, it transfers 1 wei less if the caller attempts to collect all protocol fees. At this point, this could potentially disrupt the claimFees function in V3FactoryOwner.

if (amount0 > 0) {
    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) {
    if (amount1 == protocolFees.token1) amount1--; // ensure that the slot is not cleared, for gas savings
    protocolFees.token1 -= amount1;
    TransferHelper.safeTransfer(token1, recipient, amount1);
}

Exmaple

1. We assume there are 1000 tokens of USDC and 1000 tokens of USDT in protocol fees.
2. Someone is attempting to claim all fees, which amounts to 1000 USDC and 1000 USDT, by paying 1 WETH.
3. The collectProtocol function in UniswapV3Pool returns 1000e6 - 1 USDC and 1000e6 - 1 USDT, and the transaction is reverted because the returned amount is less than the requested amount.

Impact

• The caller is reverted if attempting to collect the maximum amount of fees.

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import {Vm, Test, console} from "forge-std/Test.sol";
import {UniStaker} from "src/UniStaker.sol";
import {IUniswapV3FactoryOwnerActions} from "src/interfaces/IUniswapV3FactoryOwnerActions.sol";
import {IUniswapV3PoolOwnerActions} from "src/interfaces/IUniswapV3PoolOwnerActions.sol";
import {INotifiableRewardReceiver} from "src/interfaces/INotifiableRewardReceiver.sol";
import {IERC20Delegates} from "src/interfaces/IERC20Delegates.sol";
import {ERC20, IERC20} from "openzeppelin/token/ERC20/ERC20.sol";
import {V3FactoryOwner} from "src/V3FactoryOwner.sol";
import {TransferHelper} from "v3-core/libraries/TransferHelper.sol";

contract PausableToken is ERC20{
    bool public isPaused = false;
    address public owner;

    constructor(address _owner, string memory _name, string memory _symbol) ERC20(_name, _symbol){
        owner = _owner;
    }

    function transfer(address to, uint256 value) public override returns (bool) {
        require(!isPaused, "Contract is paused!");
        return super.transfer(to, value);
    }

    function transferFrom(address from, address to, uint256 value) public override returns (bool){
        require(!isPaused, "Contract is paused!");
        return super.transferFrom(from, to, value);
    }

    function setIsPaused(bool _status) public {
        require(owner == msg.sender, "Only owner");
        isPaused = _status;
    }

    function mint(address _account, uint256 _amount) public {
        _mint(_account, _amount);
    }
}

contract MockERC20 is ERC20{
    constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol){}

    function mint(address _account, uint256 _amount) public {
        _mint(_account, _amount);
    }
}

contract UniswapV3PoolMock{
    struct ProtocolFees {
        uint128 token0;
        uint128 token1;
    }

    ProtocolFees public protocolFees;
    address public token0;
    address public token1;

    event CollectProtocol(address indexed sender, address indexed recipient, uint128 amount0, uint128 amount1);

    constructor(address _token0, address _token1) {
        token0 = _token0;
        token1 = _token1;
    }

    function setProtocolFees(uint128 _amount0, uint128 _amount1) public {
        protocolFees = ProtocolFees({token0: _amount0, token1: _amount1});
    }

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

        if (amount0 > 0) {
            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) {
            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);
    }
}

contract UniStakerPOC is Test {
  // Addresses
  address public admin = makeAddr("ADMIN");
  address public pausableTokenOwner = makeAddr("PAUSABLE_OWNER");
  address public uniV3Factory = makeAddr("UniswapV3Factory");

  // Values
  uint256 public payoutAmount = 1e18;

  PausableToken pausableToken;
  MockERC20 weth;
  MockERC20 uni;
  UniStaker uniStaker;
  V3FactoryOwner factoryOwner;
  UniswapV3PoolMock uniV3Pool;

  function setUp() public {
      // Deploy contracts
      pausableToken = new PausableToken(pausableTokenOwner, "Pausable Token", "PT");
      weth = new MockERC20("WETH token", "WETH");
      uni = new MockERC20("Uniswap Gov token", "UNI");
      uniStaker = new UniStaker(IERC20(address(weth)), IERC20Delegates(address(uni)), address(admin));
      factoryOwner = new V3FactoryOwner(admin, IUniswapV3FactoryOwnerActions(address(uniStaker)), IERC20(weth),
          payoutAmount, INotifiableRewardReceiver(address(uniStaker)));
      // Deploy a pool
      uniV3Pool = new UniswapV3PoolMock(address(pausableToken), address(weth));

      // Allow the V3FactoryOwner contract to call the `setRewardNotifier` function.
      vm.prank(admin);
      uniStaker.setRewardNotifier(address(factoryOwner), true);
  }

  function test_claimFeesRevertIfRequestedMaximumAmount() public {
      // Protocol fees: 100 PT + 1 WETH
      pausableToken.mint(address(uniV3Pool), 100e18);
      weth.mint(address(uniV3Pool), 1e18);
      uniV3Pool.setProtocolFees(100e18, 1e18);
      (uint128 token0Fees, uint128 token1Fees) = uniV3Pool.protocolFees();

      // Attempting to claim all 100 PT and 1 WETH.
      address caller = makeAddr("CALLER");
      weth.mint(address(caller), 1e18);
      vm.startPrank(caller);
      weth.approve(address(factoryOwner), payoutAmount);
      // The transaction was reverted with `V3FactoryOwner__InsufficientFeesCollected()`
      factoryOwner.claimFees(IUniswapV3PoolOwnerActions(address(uniV3Pool)), caller, token0Fees, token1Fees);
      vm.stopPrank();
  }
}

POC setup

foundry.toml:

[profile.default]
  optimizer = true
  optimizer_runs = 10_000_000
  remappings = [
    "openzeppelin/=lib/openzeppelin-contracts/contracts",
    "v3-periphery=node_modules/@uniswap/v3-periphery/contracts",
    "v3-core=node_modules/@uniswap/v3-core/contracts"
  ]
  solc_version = "0.8.23"

forge test --mt test_claimFeesRevertIfRequestedMaximumAmount

Tools Used

Manual Review
Foundry

Recommended Mitigation Steps

- if (_amount0 < _amount0Requested || _amount1 < _amount1Requested) {
-    revert V3FactoryOwner__InsufficientFeesCollected();
- }
+ if (_amount0 < _amount0Requested - 1 || _amount1 < _amount1Requested - 1) {
+    revert V3FactoryOwner__InsufficientFeesCollected();
+ }

Assessed type

DoS

[c4-judge]

MarioPoneder marked the issue as duplicate of #34

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as grade-b