Uniswap V3 - Trail of Bits
Last updated
Was this helpful?
Last updated
Was this helpful?
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.
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.
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.
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.
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:
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.
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.
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.
_owner argument could indefinitely lock owner roleA 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.
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.
Alice deploys the UniswapV3Factory contract but mistakenly passes address(0) as the _owner.
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.
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:
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:
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:
~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).
~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.
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.
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.
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:
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.
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.
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.
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:
As amountRemainingLessFee == amountIn, the next square root ratio will be the square root target ratio:
The next square root ratio assignment results in updates to the pool’s price and tick:
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.
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.
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.
