We audited the uncx-private-repos/liquidity-locker-univ3-contracts repository at commit 342c621.
In scope were the following files:
contracts
├── UNCX_ProofOfReservesV2_UniV3.sol
└── IUNCX_ProofOfReservesV2_UniV3.sol
UniswapV3 is an Automated Market Maker (AMM) that allows users to buy tokens from token-specific pools at automatic prices. These pools have become so ubiquitous that it is often the case for new token projects to offer their tokens through them. However, in some cases, malicious actors have offered their tokens using these pools precisely because they provide them with a quick and convenient way to profit from the enthusiasm for their token. Such an attack leaves counterparties holding worthless tokens that they had hoped would become valuable.
With this in mind, if a token offeror could somehow assure counterparties that the liquidity in the market was to be permanent or would remain in the market for a long time, the risk of the token offeror "cashing out" could be credibly mitigated. The system in scope aims to do just that: allow users to lock liquidity (i.e., tokens) in a "UniswapV3-like" market for a specified time. This is achieved by first converting the market liquidity position into an NFT using the market's NonfungiblePositionManager and then transferring this NFT to the scoped contract (UNCX_ProofOfReservesV2_UniV3).
With the tokens locked, a locker can still collect transaction fees from their underlying market position. However, the locker is not able to take back ownership of its liquidity position (and therefore not able to remove the liquidity from the market) until a pre-specified time in the future. The scoped (locking) contract charges fees for providing these services: an LP fee that is collected upon locking a position and a collect fee that is collected when users collect market fees for their underlying position.
It is worth noting that the locking contract is intended to work with any "UniswapV3-like" market. Thus, such a market could be a UniswapV3 market or any fork of UniswapV3 that implements the same ecosystem of contracts with the same interfaces and operating assumptions.
In many ways, the system is a wrapper around contracts that implement UniswapV3's INonfungiblePositionManager interface. If a position manager contract is not secure, then any functionality for a lock that intends to use that contract is also not secure. For example, see H-03 below.
There are several roles within the UNCX_ProofOfReservesV2_UniV3 contract:
AUTO_COLLECT_ACCOUNT) is a contract that can collect the underlying market fees for any locked market positions. Ostensibly, this contract will only call if users opt in. However, this contract was out of scope.FEE_ADDR_LP) is an address that receives a portion of the position when locking tokens.FEE_ADDR_COLLECT) is an address that receives tokens (determined by the collect fee) when locked market positions collect their market fees.MIGRATOR) is an address that allows the lock owners, and only the lock owners, to migrate from this scoped contract to another lock-holding contract.MIGRATE_IN) is an address that will allow users to migrate existing locked market positions from other contracts.MIGRATOR addresscollect function)The system itself relies on UNCX controlling all of these roles and acting in the best interest of the users. For instance, the contract owner could set the migrator contract (MIGRATOR) to a malicious contract that effectively burns the token positions of users who attempt to migrate.
When a UniswapV3 position is locked that is not full-range, the liquidity position is removed and redeposited in the _convertPositionToFullRange function. In this situation, the liquidity between the tickUpper and tickLower decreases while the liquidity in any tick outside of that range increases. These changes in the liquidity distribution of the UniswapV3 pool can be sandwiched for a profit by manipulating the AMM with a large buy order of one token and then selling that token back into the new liquidity distribution.
The sandwiching attack requires manipulating the pool tick such that it is outside the victim's tick range (tickLower to tickUpper) when they lock their liquidity. However, when the liquidity is removed, the liquidity position is comprised entirely of one token and none of the other token. When this liquidity gets redeposited in the mint call, since there is an amount of 0 for one of the tokens, the contract will attempt to mint 0 liquidity and revert.
The attacker can still make the victim deposit the full amount of liquidity by transferring a small amount of the missing token directly to the contract before the lock call. At this point, since the AMM is already manipulated to an extreme tick, only a small transfer is required to complete the liquidity deposit. The cost of the direct transfer ends up being negligible compared to the attacker's profit.
Consider adding slippage parameters to the lock function. These slippage parameters can either be determined by input parameters or hard coded to require the pool's price tick to be within a locker's position's tick range.
Update: Resolved in pull request #1 at commit 56a8037.
Owners of a lock can set collector addresses through the setCollectAddress and setAdditionalCollector functions. However, these addresses are not reset when the lock is transferred. This means that the previous owner can still call collect on a lock that they previously owned and collect the market fees.
Consider resetting the previous collector and additionalCollector addresses or forcing the new owner to set them whenever a lock is transferred.
Update: Resolved in pull request #1 at commit c1e2718.
The protocol is meant to only lock full-range UniswapV3 positions.
The logic in the lock function can be described as:
If the NFT represents a full range position, collect the fees and lock it in. If it is not a full range, then convert the position before locking it
However, the logic block does not match this precisely. A tickLower of -maxTick and a tickUpper of 0 is a counter-example. It is not a full range and will move execution into the else block and get locked as a non-full-range position.
Consider changing the proposition check to ensure that only full ranges get locked.
Update: Resolved in pull request #1 at commit 56a8037. The UNCX team removed this design requirement from the lock function and implemented this fix in a new FullConvertor contract.
The UNCX_ProofOfReservesV2_UniV3 contract is meant to work with any "UniswapV3-like" protocol. It does this by allowing users to specify their own position manager contract. This gives complete execution control to an arbitrary contract that could be tailored specifically to attack the UNCX_ProofOfReservesV2_UniV3 contract.
Consider a contract that simply refuses to cooperate. It wraps all of its methods around a well-behaved NFT position manager contract (e.g., UniswapV3's). However, if it sees a call to collect from the UNCX contract, it simply does nothing and does not pass the call on to the actual NFT position manager. In this manner, the contract can bypass the lock creation fee, the collect fee, and can supply the wrong maxTick to pass the non-full-range positions from the correct manager to the UNCX locker contract. It can even refuse to safeTransferFrom the NFT to the UNCX contract, rendering the lock contract completely ineffective.
Consider implementing a whitelist system for the position manager contracts with the contract owner having the power to add (but not remove) permitted position managers.
Update: Resolved in pull request #1 at commit ea8b60a.
The adminRefundEth and adminRefundERC20 functions can be used by the owner to withdraw tokens which are accidentally sent to the contract. However, anybody can withdraw tokens in the contract by calling lock on a non-full-range NFT position where one pool token corresponds to the token which is stuck in the contract.
Within the lock function, the call to _convertPositionToFullRange will send the entire balanceOf both pool tokens to either the new locked liquidity position or refund it to the dustRecipient. Thus, the user who calls lock gets all the tokens that are stuck in the contract credited to themselves by setting the dustRecipient to an address they control.
Consider implementing such logic whereby only the tokens which come from the liquidity position during the liquidity removal are redeposited during _convertPositionToFullRange instead of the entire token balance of the contract.
Update: Resolved in pull request #3 at commit c750641.
In the codebase, there is an auto-collector role (AUTO_COLLECT_ACCOUNT) that can call collect on behalf of the lock owners to collect their underlying market position fees. However, the tokens being transferred and the position manager being called by the contract are arbitrary and provided by users.
A malicious user can supply the address of a smart contract tailored to take advantage of the free gas. Such attacks have been seen in the past with FTX's withdrawal mechanism or dYdX's metatransaction mechanism. Mitigations could include a whitelist for tokens and an auto-collect blacklist for locks, but both of these would stop the protocol from providing its core service.
Consider monitoring the AUTO_COLLECT_ACCOUNT's gas consumption in order to stop it from calling collect on problematic positions.
Update: Acknowledged, not resolved. The UNCX team stated:
The auto-collector bot is called manually at the moment and only for certain clients. We will make sure tokens fit our spec before adding them to the bot.
When users withdraw from the protocol (transfer back their NFT after the time lock has expired), there is a "collect fee" that is assessed from the user. However, users can sidestep this fee by simply calling decreaseLiquidity instead which transfers the full amount to the caller (the lock owner).
Consider reworking the logic for how the collect fee is collected.
Update: Resolved in pull request #3 at commit 8355982.
The LP fee is meant to take a portion of the tokens-to-be-locked when users call the lock function. However, this can easily be sidestepped by locking a position with minimal tokens and then later calling increaseLiquidity which does not collect the LP fee. This fee can also be sidestepped by calling increaseLiquidity directly on the UniswapV3 NonFungiblePositionManager.
Consider changing the logic for how the LP fee is assessed and collected.
Update: Acknowledged, not resolved. The UNCX team stated:
Since this is possible via the
NonFungiblePositionManageritself and is outside of our contracts, this is ok.
Throughout the codebase, there are numerous parts that do not have docstrings.
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API, including interface functions. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, not resolved.
require StatementsIn UNCX_ProofOfReservesV2_UniV3.sol, there are multiple require statements that either lack any error message or have error messages that are not descriptive:
require statements on lines 108 and 144 are missing error messages.require statements on lines 148, 276, 345, along with many others have one-word error messages.Consider including specific, informative error messages in require statements to improve the overall code clarity and facilitate troubleshooting whenever a requirement is not satisfied. If saving on gas is a priority, consider adding informative custom errors instead of require statements.
Update: Partially resolved in pull request #3 at commit 124b495. The UNCX team stated:
Partially fixed. Only added error messages where there were none before.
transfer and send Calls Are No Longer Considered Best PracticeWhen transfer or send calls are used to transfer ETH to an address, they forward a limited amount of gas. Given that the gas prices for EVM operations are sometimes repriced, code execution on the receiving end of these calls cannot be guaranteed in perpetuity. Also, the upcoming TSTORE opcode will open new reentrancy attack vectors with the use of these calls. Currently, transfer is used at lines 130 and 544 of UNCX_ProofOfReservesV2_UniV3.sol.
Instead of using transfer or send, consider using address.call{value: amount}("") or the sendValue function of the OpenZeppelin Address library to transfer ETH.
Update: Resolved in pull request #3 at commit 8225e36.
The UNCX_ProofOfReservesV2_UniV3 contract does not implement the onERC721Received function according to the EIP-721 specification. Instead of public visibility, it should be external, and instead of pure mutability, it should have default (i.e., no keyword) mutability privileges.
Consider implementing the function exactly as prescribed in EIP-721.
Update: Resolved in pull request #3 at commit e55dcf3.
In UNCX_ProofOfReservesV2_UniV3.sol, there are several sections that have an incomplete docstring.
Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of any contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Acknowledged, not resolved.
Throughout the codebase, there are multiple components that do not follow the Solidity Style Guide. For example, events such as onMigrate and onRemoveFee should be capitalized at the start. In addition, the contract name should read UNCXProofOfReservesV2UniV3 instead of UNCX_ProofOfReservesV2_UniV3.
To improve the project's overall legibility, consider adhering to the Solidity Style Guide by naming the contracts, structs, enums, events, and errors using the CapWords style.
Update: Acknowledged, not resolved.
Named return variables are a way to declare variables that are meant to be used within a function's body for the purpose of being returned as the function's output. They are an alternative to explicit in-line return statements. In UNCX_ProofOfReservesV2_UniV3's getAmountsForLiquidity function, amount0 and amount1 are unused.
Consider removing any unused named return variables.
Update: Resolved in pull request #3 at commit d619a27.
The use of non-explicit imports in the codebase can decrease the clarity of the code and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity files or when inheritance chains are long. In the UNCX_ProofOfReservesV2_UniV3 contract, global imports are being used.
Following the principle that clearer code is better code, consider using named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
Update: Resolved in pull request #3 at commit ef64ac6.
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they discover a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for the maintainers of these libraries to contact the appropriate person about the problem and provide mitigation instructions.
Consider adding a NatSpec comment containing a security contact above the scoped contracts. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Acknowledged, not resolved.
To improve the readability of the contract, use the same return style in all of it is functions. Some of UNCX_ProofOfReservesV2_UniV3's functions explicitly return while others have implicit return values.
Consider using a consistent style for all functions.
Update: Acknowledged, not resolved.
In UNCX_ProofOfReservesV2_UniV3.sol, the state variable USER_LOCKS lacks an explicitly declared visibility.
For clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #3 at commit ee4384a.
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping's purpose.
In UNCX_ProofOfReservesV2_UniV3.sol, there are multiple mappings without named parameters:
FEES state variableLOCKS state variableUSER_LOCKS state variableConsider adding named parameters to the mappings to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #3 at commit dc059e3.
No events in IUNCX_ProofOfReservesV2_UniV3.sol have indexed parameters.
Consider indexing the event parameters to improve the ability of off-chain services to search and filter for specific events.
Update: Acknowledged, not resolved.
In cryptography, a nonce is an arbitrary number that can be used just once in a cryptographic communication. In the UNCX_ProofOfReservesV2_UniV3 contract, the variable NONCE is the current lock id that will be assigned to new locks. Furthermore, fee structures have names which are hashed and those hashes are stored for quick lookup and recall. The hash of these names is sometimes called nameHash and other times feeHash.
To improve the readability and maintainability of the codebase, consider naming variables accurately and consistently.
Update: Acknowledged, not resolved.
immutableIf a variable is only ever assigned a value from within the constructor of a contract, then it could be declared as immutable. COUNTRY_LIST is such a variable.
To better convey the intended use of variables and to potentially save gas, consider adding the immutable keyword to variables that are only set in the constructor.
Update: Resolved in pull request #3 at commit 4c1e716.
constantIf a variable is only ever assigned a value when it is declared, then it could be declared as constant. The ETERNAL_LOCK is such a variable.
To better convey the intended use of variables and to potentially save gas, consider adding the constant keyword to variables that are only set when they are declared.
Update: Resolved in pull request #3 at commit 6f86778.
UniswapV3 is an AMM that allows users to trade tokens using specific pools. However, malicious actors can exploit these pools, jeopardizing the users' funds. The proposed system aims to mitigate this problem by enabling users to lock liquidity in a market, which is achieved by converting the liquidity into an NFT and then transferring it to the UNCX_ProofOfReservesV2_UniV3 contract. Users can still earn transaction fees but cannot remove the liquidity until a set time in the future, thereby reducing the possibility of pool exploits. We found several issues that need to be resolved in order for the system to work properly. However, nothing suggests that these issues should be difficult to resolve.