BohdanHrytsak
commented
8 months ago Thank you for the submission.
In the case of invalid bottomTick or topTick, this condition should return true, which will throw an error revert tickIsNotSpaced()
The | operator has a priority of 9, % operator has a priority 4, and the != operator has a priority of 11, a lower number has a higher priority
Accordingly, the condition % will be fulfilled first, after |, and at the end !=
This corresponds to the expected behavior, in the case of your example we get:
bottomTick % _tickSpacing | topTick % _tickSpacing != 0
20 % 10 | 25 % 10 != 0 -> 0 | 5 != 0 -> 5 != 0 -> true -> revert
You can also see tests that show that everything works correctly: https://github.com/Satsyxbt/Fenix-dex-v3/blob/ff54544c2a8bcc49252ee0ec9b32aea7998e41ca/src/core/test/AlgebraPool.spec.ts#L297
If we use || (priority 13), it will result in an error and damage
Github username: -- Twitter username: datamcrypto Submission hash (on-chain): 0xa4b4d2002970bcbdd868691d1fa5375230b3ebc7c59d93b6882cf89f165eb347 Severity: high
Description: Description\ AlgebraPool.sol::mint The tick spacing validation issue in the provided code snippet revolves around a common programming pitfall related to operator precedence and the use of bitwise operations in conditional statements. Specifically, the issue lies in this line
if (bottomTick % _tickSpacing | topTick % _tickSpacing != 0) revert tickIsNotSpaced();The intent of this line is to ensure that both bottomTick and topTick are perfectly divisible by _tickSpacing, meaning that when each is divided by _tickSpacing, there's no remainder, ensuring that ticks align with the protocol's defined granularity. However, the way the condition is written leads to a logical error due to how operators are evaluated in Solidity (and many other programming languages).Operator precedence determines the order in which operators are evaluated in expressions. In Solidity, as in many C-like languages, the inequality operator (!=) has a higher precedence than the bitwise OR operator (|). Therefore, the expression is evaluated in two steps:
First, it evaluates topTick % _tickSpacing != 0, which results in a boolean value (true or false, equivalent to 1 or 0 in the context of bitwise operations). Then, it performs the bitwise OR operation between bottomTick % _tickSpacing and the result of the previous step. This could lead to scenarios where the condition mistakenly passes, even if one of the ticks is not correctly spaced.
Attack Scenario\ The purpose of tick spacing is to concentrate liquidity within certain bounds, making the pool more efficient and improving the trade execution for users. If positions can be created at any tick, it may lead to more fragmented liquidity distribution across the pool, potentially resulting in worse prices for traders and less efficient capital utilization for liquidity providers.
Identify a Vulnerable Contract: An attacker identifies a liquidity pool smart contract with the tick spacing validation issue described.
Crafting Malicious Ticks: The attacker selects a pair of ticks for a new liquidity position, bottomTick and topTick, where one of the ticks (say topTick) does not adhere to the tick spacing requirement. For example, if _tickSpacing is 10, the attacker might choose bottomTick = 100 (which is valid) and topTick = 105 (which is invalid).
Executing the Mint Function: The attacker calls the mint function with their selected ticks. Due to the logical error in tick spacing validation, the contract incorrectly evaluates the condition as true (passing the check) even though topTick does not comply with the tick spacing rules.
Creating an Invalid Position: The mint operation succeeds, creating a liquidity position with a topTick that violates the protocol's tick spacing rules. This could disrupt the orderly function of the liquidity pool, as the pricing and liquidity might now be managed in a way that was not intended by the pool's design.
Potential Manipulation: With the creation of improperly spaced positions, the attacker might be able to manipulate the market in several ways. For example, if the protocol's logic or incentives depend on certain assumptions about tick spacing (such as fee calculations, slippage estimations, or liquidity mining rewards), the attacker could exploit these assumptions. They might be able to initiate trades that benefit from unexpected price impacts, arbitrage opportunities, or manipulate reward mechanisms.
Extraction of Value: By exploiting these manipulations, the attacker could potentially extract value from honest liquidity providers or traders who rely on the integrity of the tick spacing rules for predictable trading and liquidity provision conditions. Attachments
The intended logic is to check if both bottomTick and topTick are multiples of _tickSpacing. For bottomTick = 20, bottomTick % _tickSpacing equals 0 (correctly spaced). For topTick = 25, topTick % _tickSpacing equals 5 (not correctly spaced). According to the intended logic, the condition should fail because topTick is not a multiple of _tickSpacing. However, due to the operator precedence issue:
topTick % _tickSpacing != 0 evaluates to true (or 1 in bitwise context). bottomTick % _tickSpacing | true evaluates to true, because 0 | 1 equals 1. As a result, the condition incorrectly passes, and the function execution continues instead of reverting as intended.
-if (bottomTick % _tickSpacing | topTick % _tickSpacing != 0) revert tickIsNotSpaced();+if (bottomTick % _tickSpacing != 0 || topTick % _tickSpacing != 0) revert tickIsNotSpaced();