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.
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
We assume there are 1000 tokens of USDC and 1000 tokens of USDT in protocol fees.
Someone is attempting to claim all fees, which amounts to 1000 USDC and 1000 USDT, by paying 1 WETH.
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();
}
}
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 outPAYOUT_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.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.Exmaple
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
Proof of Concept
POC setup
foundry.toml:
Tools Used
Manual Review Foundry
Recommended Mitigation Steps
Assessed type
DoS