✅Uniswap V3 - Trail of Bits

Progress

[H-1] Incorrect comparison enables swapping and token draining at no cost

Description

An incorrect comparison in the swap() function allows the swap to succeed even if no tokens are paid. This issue could be used to drain any pool of all of its tokens at no cost.

// transfer the output
if (amountOut != 0) TransferHelper.safeTransfer(tokenOut, recipient, uint256 (-amountOut));

// callback for the input
uint256 balanceBefore = balanceOfToken(tokenIn);
zeroForOne
    ? IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amountIn, amountOut, data)
    : IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amountOut, amountIn, data);'
// @audit-issue This formula can be rewritten as:
// balanceBefore + amountIn >= balanceBefore + msg.value
// This check always passes if `msg.value == 0` and `amountIn` is some huge number
// The formula should be `balanceBefore + amountIn <= balanceBefore + msg.value`
// User can send more money to the pool, not less
require(balanceBefore.add(uint256(amountIn)) >= balanceOfToken(tokenIn), 'IIA');

The swap() function calculates how many tokens the initiator (msg.sender) needs to pay (amountIn) to receive the requested amount of tokens (amountOut). It then calls the uniswapV3SwapCallback() function on the initiator’s account, passing in the amount of tokens to be paid. The callback function should then transfer at least the requested amount of tokens to the pool contract. Afterward, a require inside the swap function verifies that the correct amount of tokens (amountIn) has been transferred to the pool.

However, the check inside the require is incorrect. Instead of checking that at least the requested amount of tokens has been transferred to the pool, it checks that no more than the requested amount has been transferred. In other words, if the callback does not transfer any tokens to the pool, the check, and the swap, will succeed without the initiator having paid any tokens.

Exploit Scenario

Bob deploys a pool for USDT/DAI. The pool holds 1,000,000 DAI. Eve calls a swap for 1,000,000 DAI but transfers 0 USDT, stealing all of the DAI from the pool.

Recommendation

Short term, replace the >= with <= inside the require in the swap function. Add at least one unit test checking that the IIA error is thrown when too few tokens are transferred from the initiator’s contract to the pool.

Long term, consider adding at least one unit test for each error that can be thrown by the contracts. With a unit test, an error would be thrown when it should be, at least in a simple case. Also consider adding more properties and using Echidna or Manticore to verify that initiators are correctly transferring tokens to the pool.

[H-2] Failed transfer may be overlooked due to lack of contract existence check

Description

Because the pool fails to check that a contract exists, the pool may assume that failed transactions involving destructed tokens are successful.

TransferHelper.safeTransfer() performs a transfer with a low-level call without confirming the contract’s existence:

) internal {
    (bool success , bytes memory data) =
        token.call(abi.encodeWithSelector(IERC20Minimal.transfer.selector, to, value));
    require (success && (data.length == 0 || abi.decode(data, (bool))), 'TF');

The Solidity documentation includes the following warning:

The low-level call, delegatecall, and callcode will return success if the calling account is non-existent, as part of the design of EVM. Existence must be checked prior to calling if desired.

As a result, if the tokens have not yet been deployed or have been destroyed, safeTransfer() will return success even though no transfer was executed.

If the token has not yet been deployed, no liquidity can be added. However, if the token has been destroyed, the pool will act as if the assets were sent even though they were not.

Exploit Scenario

The pool contains tokens A and B. Token A has a bug, and the contract is destroyed. Bob is not aware of the issue and swaps 1,000 B tokens for A tokens. Bob successfully transfers 1,000 B tokens to the pool but does not receive any A tokens in return. As a result, Bob loses 1,000 B tokens.

Recommendation

Short term, check the contract’s existence prior to the low-level call in TransferHelper.safeTransfer(). This will ensure that a swap reverts if the token to be bought no longer exists, preventing the pool from accepting the token to be sold without returning any tokens in exchange.

Long term, avoid low-level calls. If such a call is not avoidable, carefully review the Solidity documentation, particularly the “Warnings” section.

[M-1] Missing validation of _owner argument could indefinitely lock owner role

Description

A lack of input validation of the _owner argument in both the constructor and setOwner() functions could permanently lock the owner role, requiring a costly redeploy.

constructor(address _owner) {
    owner = _owner;
    emit OwnerChanged(address(0), _owner);
    _enableFeeAmount(600 , 12);
    _enableFeeAmount(3000 , 60);
    _enableFeeAmount(9000 , 180);
}
function setOwner(address _owner) external override {
    require(msg.sender == owner, 'OO' );
    emit OwnerChanged(owner, _owner);
    owner = _owner;
}

The constructor calls _enableFeeAmount() to add three available initial fees and tick spacings. This means that, as far as a regular user is concerned, the contract will work, allowing the creation of pairs and all functionality needed to start trading. In other words, the incorrect owner role may not be noticed before the contract is put into use.

The following functions are callable only by the owner:

  • UniswapV3Factory.enableFeeAmount()

    • Called to add more fees with specific tick spacing.

  • UniswapV3Pair.setFeeTo()

    • Called to update the fees’ destination address.

  • UniswapV3Pair.recover()

    • Called to withdraw accidentally sent tokens from the pair.

  • UniswapV3Factory.setOwner()

    • Called to change the owner.

To resolve an incorrect owner issue, Uniswap would need to redeploy the factory contract and re-add pairs and liquidity. Users might not be happy to learn of these actions, which could lead to reputational damage. Certain users could also decide to continue using the original factory and pair contracts, in which owner functions cannot be called. This could lead to the concurrent use of two versions of Uniswap, one with the original factory contract and no valid owner and another in which the owner was set correctly.

Trail of Bits identified four distinct cases in which an incorrect owner is set:

  • Passing address(0) to the constructor

  • Passing address(0) to the setOwner() function

  • Passing an incorrect address to the constructor

  • Passing an incorrect address to the setOwner() function.

Exploit Scenario

Alice deploys the UniswapV3Factory contract but mistakenly passes address(0) as the _owner.

Recommendation

Several improvements could prevent the four abovementioned cases:

  • Designate msg.sender as the initial owner, and transfer ownership to the chosen owner after deployment.

  • Implement a two-step ownership-change process through which the new owner needs to accept ownership.

  • If it needs to be possible to set the owner to address(0), implement a renounceOwnership() function.

Long term, use Slither, which will catch the missing address(0) check, and consider using two-step processes to change important privileged roles.

[M-2] Unbound loop enables denial of service

The swap() function relies on an unbounded loop. An attacker could disrupt swap operations by forcing the loop to go through too many operations, potentially trapping the swap due to a lack of gas.

UniswapV3Pool.swap() iterates over the tick:

while (state.amountSpecifiedRemaining != 0 && state.sqrtPriceX96 != sqrtPriceLimitX96) {
    StepComputations memory step;
    
    step.sqrtPriceStartX96 = state.sqrtPriceX96;
    [..]
        state.tick = zeroForOne ? step.tickNext - 1 : step.tickNext;
    } else if (state.sqrtPriceX96 != step.sqrtPriceStartX96) {
        // recompute unless we're on a lower tick boundary (i.e. already transitioned ticks), and haven't moved
        state.tick = TickMath.getTickAtSqrtRatio(state.sqrtPriceX96);
     }
 }

On every loop iteration, there is a swap on the current tick’s price, increasing it to the next price limit. The next price limit depends on the next tick:

(step.tickNext, step.initialized) = tickBitmap.nextInitializedTickWithinOneWord(
    state.tick,
    tickSpacing,
    zeroForOne
);

// ensure that we do not overshoot the min/max tick, as the tick bitmap is not aware of these bounds
if (step.tickNext < TickMath.MIN_TICK) {
    step.tickNext = TickMath.MIN_TICK;
} else if (step.tickNext > TickMath.MAX_TICK) {
    step.tickNext = TickMath.MAX_TICK;
}

// get the price for the next tick
step.sqrtPriceNextX96 = TickMath.getSqrtRatioAtTick(step.tickNext);

The next tick is the next initialized tick (or an uninitialized tick if no initialized tick is found).

A conservative gas cost analysis of the loop iteration returns the following estimates:

  1. ~50,000 gas per iteration if there is no previous fee on the tick (7 SLOAD, 1 SSTORE from non-zero to non-zero, 2 SSTORE from zero to non-zero).

  2. ~20,000 gas per iteration if there are previous fees on the tick (7 SLOAD, 3 SSTORE from non-zero to non-zero).

The current block gas limit is 12,500,000. As a result, the swap operation will not be doable if it requires more than 2,500 (scenario 1) or 6,250 (scenario 2) iterations.

An attacker could create thousands of positions with 1 wei to make the system very costly and potentially prevent swap operations.

An attacker would have to pay gas to create the position. However, an Ethereum miner could create a position for free, and if the system were deployed on a layer 2 solution (e.g., optimism), the attacker’s gas payment would be significantly lower.

Exploit Scenario

Eve is a malicious miner involved with a Uniswap competitor. Eve creates thousands of positions in every Uniswap V3 pool to prevent users from using the system.

Recommendation

Short term, to mitigate the issue, determine a reasonable minimum tick spacing requirement, or consider setting a minimum for liquidity per position.

Long term, make sure that all parameters that the owner can enable (such as fee level and tick spacing) have bounds that lead to expected behavior, and clearly document those bounds, such as in a markdown file or in the whitepaper.

[M-3] Front-running pool’s initialization can lead to draining of liquidity provider’s initial deposits

Description

A front-run on UniswapV3Pool.initialize() allows an attacker to set an unfair price and to drain assets from the first deposits.

UniswapV3Pool.initialize() initiates the pool’s price:

function initialize(uint160 sqrtPriceX96) external override {
    require (slot0.sqrtPriceX96 == 0 , 'AI' );

    int24 tick = TickMath.getTickAtSqrtRatio(sqrtPriceX96);
 
    (uint16 cardinality , uint16 cardinalityNext ) = 
observations.initialize(_blockTimestamp());

    slot0 = Slot0({
        sqrtPriceX96: sqrtPriceX96,
        tick: tick,
        observationIndex: 0 ,
        observationCardinality: cardinality,
        observationCardinalityNext: cardinalityNext,
        feeProtocol: 0 ,
        unlocked: true
     });

     emit Initialize(sqrtPriceX96, tick);
 }

There are no access controls on the function, so anyone could call it on a deployed pool.

Initializing a pool with an incorrect price allows an attacker to generate profits from the initial liquidity provider’s deposits.

Exploit Scenario

  • Bob deploys a pool for assets A and B through a deployment script. The current market price is 1 A == 1 B.

  • Eve front-runs Bob’s transaction to the initialize function and sets a price such that 1 A ~= 10 B.

  • Bob calls mint and deposits assets A and B worth $100,000, sending ~10 times more of asset B than asset A.

  • Eve swaps A tokens for B tokens at an unfair price, profiting off of Bob’s deployment.

Two tests that demonstrate such an attack are included in Appendix G.

Recommendation

Short term, consider:

  • moving the price operations from initialize() to the constructor,

  • adding access controls to initialize(), or

  • ensuring that the documentation clearly warns users about incorrect initialization.

Long term, avoid initialization outside of the constructor. If that is not possible, ensure that the underlying risks of initialization are documented and properly tested.

[M-4] Swapping on zero liquidity allows for control of the pool’s price

Swapping on a tick with zero liquidity enables a user to adjust the price of 1 wei of tokens in any direction. As a result, an attacker could set an arbitrary price at the pool’s initialization or if the liquidity providers withdraw all of the liquidity for a short time.

Swapping 1 wei in exactIn with a liquidity of zero and a fee enabled will cause amountRemainingLessFee and amountIn to be zero:

uint256 amountRemainingLessFee = FullMath.mulDiv(uint256(amountRemaining), 1e6 - feePips,
1e6);
amountIn = zeroForOne
    ? SqrtPriceMath.getAmount0Delta(sqrtRatioTargetX96, sqrtRatioCurrentX96, liquidity, true)
    : SqrtPriceMath.getAmount1Delta(sqrtRatioCurrentX96, sqrtRatioTargetX96, liquidity, true);

As amountRemainingLessFee == amountIn, the next square root ratio will be the square root target ratio:

if (amountRemainingLessFee >= amountIn) sqrtRatioNextX96 = sqrtRatioTargetX96;

The next square root ratio assignment results in updates to the pool’s price and tick:

// shift tick if we reached the next price
if (state.sqrtPriceX96 == step.sqrtPriceNextX96) {
    // if the tick is initialized, run the tick transition
    if (step.initialized) {
        int128 liquidityNet =
            ticks.cross(
                step.tickNext,
                (zeroForOne ? state.feeGrowthGlobalX128 : feeGrowthGlobal0X128),
                (zeroForOne ? feeGrowthGlobal1X128 : state.feeGrowthGlobalX128)
             );
        // if we're moving leftward, we interpret liquidityNet as the opposite sign
        // safe because liquidityNet cannot be type(int128).min
        if (zeroForOne) liquidityNet = -liquidityNet;
         
        secondsOutside.cross(step.tickNext, tickSpacing, cache.blockTimestamp);
    
        state.liquidity = LiquidityMath.addDelta(state.liquidity, liquidityNet);
}

On a tick without liquidity, anyone could move the price and the tick in any direction. A user could abuse this option to move the initial pool’s price (e.g., between its initialization and minting) or to move the pool’s price if all the liquidity is temporarily withdrawn.

Exploit Scenario

  • Bob initializes the pool’s price to have a ratio such that 1 token0 == 10 token1.

  • Eve changes the pool’s price such that 1 token0 == 1 token1.

  • Bob adds liquidity to the pool.

  • Eve executes a swap and profits off of the unfair price.

Appendix I contains a unit test for this issue.

Recommendation

Short term, there does not appear to be a straightforward way to prevent the issue. We recommend investigating the limits associated with pools without liquidity in some ticks and ensuring that users are aware of the risks.

Long term, ensure that pools can never end up in an unexpected state.

PreviousFei Protocol - ConsenSysNextChainlink - Sigma Prime

Last updated