Uniswap: Protocol Fees

Description

The V3FeeAdapter.setDefaultFeeByFeeTier() function accepts any uint8 value (0-255) for defaultFeeValue without validating that it conforms to the constraints required by the Uniswap V3 pool's setFeeProtocol() function.

When _setProtocolFee() is called, the stored defaultFeeValue is decomposed into two 4-bit components:

IUniswapV3PoolOwnerActions(pool).setFeeProtocol(feeValue % 16, feeValue >> 4);

However, the Uniswap V3 pool's setFeeProtocol() function enforces strict validation:

require(      (feeProtocol0 == 0 || (feeProtocol0 >= 4 && feeProtocol0 <= 10)) &&          (feeProtocol1 == 0 || (feeProtocol1 >= 4 && feeProtocol1 <= 10))  );

If an invalid defaultFeeValue is set (e.g., 255 → decomposes to 15, 15), all calls to _setProtocolFee() for that fee tier will revert. Protocol fee updates are temporarily unavailable for the affected fee tier until corrected.

Recommendation

Add validation in setDefaultFeeByFeeTier() to ensure the defaultFeeValue will decompose into valid components.

function setDefaultFeeByFeeTier(uint24 feeTier, uint8 defaultFeeValue) external onlyFeeSetter {      require(_feeTierExists(feeTier), InvalidFeeTier());+      // Extract the two 4-bit values+      uint8 feeProtocol0 = defaultFeeValue % 16;+      uint8 feeProtocol1 = defaultFeeValue >> 4;+      // Validate both values match pool requirements: must be 0 or in range [4, 10]+      require(+          (feeProtocol0 == 0 || (feeProtocol0 >= 4 && feeProtocol0 <= 10)) &&+          (feeProtocol1 == 0 || (feeProtocol1 >= 4 && feeProtocol1 <= 10)),+          InvalidFeeValue()+      );      defaultFees[feeTier] = defaultFeeValue;  }

Description

The release function intends to serve as an exchange function where the searchers need to send the threshold of the UNI token in exchange for a basket of assets. These assets are the tokens which were claimed as fees and now they are resting in the TokenJar contract.

function release(uint256 _nonce, Currency[] calldata assets, address recipient)    external    handleNonce(_nonce)  {    require(assets.length <= MAX_RELEASE_LENGTH, TooManyAssets());    RESOURCE.safeTransferFrom(msg.sender, RESOURCE_RECIPIENT, threshold);    TOKEN_JAR.release(assets, recipient);    _afterRelease(assets, recipient);  }

It is expected to have a very large variety of tokens inside TokenJar. As discussed, the Uniswap team looked at all of the pools from univ3 that have 90% or over market share over other DEXs, which amounts to thousands of pools. These pools should be the ones included inside the merkle tree.

Each pool has its volume and fees from pools with very low volume will be minimal. Given there will be so many assets, and the MAX_RELEASE_LENGTH = 20 there will be a situation for multiple tokens where its simply not worth release as the value of UNI that needs to be burned in exchange for the tokens released its superior, leaving such tokens with value inside the contract until it is profitable to release them, if that ever happens.

Recommendation

This is mostly an architectural issue. Increasing the value of MAX_RELEASE_LENGTH could help but it should not be an overly big value either. We recommend to document and understand that this will happen and take into account that it could need a governance vote at some point to lower the threshold low enough so that its profitable to sweep some of these low value tokens to then increase the threshold again.

Uniswap

Documented here PR 101

Description

Throughout the codebase, multiple instances of missing docstrings were identified:

• In ArrayLib.sol, the ArrayLib library is completely undocumented, including the includes() function.
• In Deployer.sol, the Deployer contract is also completely undocumented, including all the state variables and constants.
• In Nonce.sol, the Nonce contract is missing @title and @notice for the contract. Also, the modifier handleNonce is undocumented.
• In V3Adapter.sol, the function setFactoryOwner() is missing docs alongside most of the internal functions including _setProtocolFeesForPair(), _setProtocolFee(), _doubleHash() and _feeTierExists().

Recommendation

Thoroughly document all functions and the contract mentioned above.

Description

Multiple functions across the protocol lack address(0) validation for critical address parameters.

Functions with higher impact: (would recommend fixing)

1. TokenJar.sol

• setReleaser(address _releaser): No validation before setting the releaser address. If the releaser is set to zero address, then funds in the token jar will be temporarily locked.

2. UNIVesting.sol

• updateRecipient(address _recipient): No validation before updating the vesting recipient. Setting the recipient to address (0) is equivalent to burning UNI tokens intended for the Uniswap Labs team.

3. ExchangeReleaser.sol

• release(uint256 _nonce, Currency[] calldata assets, address recipient): No validation of recipient parameter, could lead to searchers losing funds.

4. V3FeeAdapter.sol

• setFactoryOwner(address newOwner): No validation before transferring factory ownership, could permanently lock claiming fees from Uni V3.

Functions with relatively lower impact:

1. ResourceManager.sol

• setThresholdSetter(address _thresholdSetter): No validation before setting threshold setter

2. V3FeeAdapter.sol

• setFeeSetter(address newFeeSetter): No validation before setting the fee setter

Recommendation

Consider adding address(0) validation to all affected functions.

Description

The updateVestingAmount() function allows the owner to change the quarterly vesting amount, but only when quartersPassed() == 0 (i.e., no quarters are currently available to claim).

However, when the vesting amount is changed, any tokens previously approved by the owner that don't align with the new quarterly amount become difficult to claim and may remain permanently unclaimed without manual intervention.

Consider the following timeline:

1. Owner sets initial quarterlyVestingAmount = 5,000,000 UNI
2. Owner approves 40,000,000 UNI to the contract
3. Recipient withdraws 6 quarters: 6 × 5,000,000 = 30,000,000 UNI
4. Owner updates quarterlyVestingAmount to 3,000,000 UNI (perhaps due to budget changes)
5. Recipient withdraws 3 more quarters: 3 × 3,000,000 = 9,000,000 UNI
6. Total withdrawn: 39,000,000 UNI
7. Remaining allowance: 1,000,000 UNI - temporarily stuck unless the owner changes the quarterlyVestingAmount to 1,000,000 UNI (or) approves 2,000,000 more UNI tokens.

Recommendation

Consider documenting this limitation and always ensure that the owner changes the quarterlyVestingAmount carefully to not run into these kinds of edge cases.

Alternatively, add a sweep function, allowing the recipient to sweep any residual approvals.

Uniswap

Added documentation explaining this behavior in PR-101

Description

Multiple functions across the protocol lack event emissions for key state changes:

1. V3Adapter.sol

• setMerkleRoot
• setFeeSetter
• setDefaultFeeByFeeTier

2. TokenJar.sol

• setReleaser

3. ResourceManager.sol

• setThresholdSetter
• setThreshold

Recommendation

Emit an event to reflect the state changes for each one of the above functions.

Description

In the following contracts:

• V3FeeAdapter.sol
• TokenJar.sol
• UNIVesting.sol
• ResourceManager.sol

A single-step ownership transfer pattern is used, where if a mistake is made when transferring ownership there will not be a way of reverting such state.

It is a recommended practice to use a 2-step ownership transfer pattern where the address that has been set as the "pending owner" needs to accept ownership first, before officially transferring the ownership to that addres.

Recommendation

Use Ownable2Step from Open Zeppelin as your inherited ownership contract.

Description

The Deployer contract stores fee tiers in the V3FeeAdapter but fails to configure the corresponding default fee values for these tiers before transferring ownership to the DAO. This oversight leaves all protocol fees effectively disabled (set to 0%) until the DAO addresses this through a time-delayed governance process.

Recommendation

Configure default fee values for all stored fee tiers in the Deployer constructor before transferring ownership.

Description

The collect() function performs unnecessary type conversions that waste gas. The IUniswapV3PoolOwnerActions.collectProtocol() function returns (uint128, uint128), but the return values are assigned to uint256 variables and then immediately cast back down to uint128 when populating the Collected struct.

Recommendation

Declare the return variables as uint128 to match the actual return type from collectProtocol(), eliminating both casting operations.