QuickSwap and StellaSwap contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L880

Vulnerability details

Impact

Undercounted liquidity leads to incorrect trading volume reporting and affects the adaptive fee calculation. In most cases (when the majority of liquidity in a pool is concentrated around the current price) trading volume is overstated, which results in lowered swap fees.

Proof of Concept

According to the Tech Paper, trading volume is one of the three indicators that are used to compute the adaptive swap fee:

To find the optimal commission amount, depending on the nature of the asset's behavior, the following indicators are monitored:

1. Volatility
2. Liquidity
3. Trading Volume

Further in the paper, it's shown how trading volume per liquidity is calculated: $I_t = \frac{\widetilde{L}_t}{L_t}$

Where $\widetilde{L}_t = \sqrt{V_1 * V_2}$ is the average trading volume of assets 1 and 2; $L_t$ is the liquidity that was used during a trade.

In Uniswap V3 (on which Algebra Finance is based), liquidity is concentrated in discrete price ranges–this can clearly be seen on the official analytics dashboard. During swapping, the current price moves within a price range and, when there's not enough liquidity in the price range to satisfy the swap, jumps to adjacent liquidity positions. In the case of big swaps and narrow liquidity positions, the current price can traverse over multiple intermediary liquidity positions before the swap is fulfilled and end up in a different price range. In such situation, the liquidity required to fulfil the swap is made of:

1. liquidity of the initial price range;
2. liquidity of the intermediary price ranges;
3. liquidity of the final price range the price has stopped at.

According to the Tech Paper, it's expected that $L_t$ takes into account the liquidity in the initial and final price ranges, as well as liquidity of the intermediary price ranges (since this is the liquidity required to make the swap). However, in the code, it only counts the liquidity of the final price range (calculateVolumePerLiquidity is called after the main swap loop):
https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L880

By the time the calculateVolumePerLiquidity function is called, currentLiquidity is the liquidity that's available at the new price (the price after a swap), which includes only the price ranges that include the new price. During a swap, currentLiquidity is updated multiple times whenever new liquidity is added or removed while the price is traversing over liquidity positions. The liquidity of the initial price range and all the intermediary price ranges is not counted by calculateVolumePerLiquidity.

The idea of volume per liquidity calculation is somewhat similar to swap fees calculation: swap fees are also calculated on the liquidity engaged in a swap. In the code, swap fees are calculated in the movePriceTowardsTarget function, which is called inside the main swap loop and which receives currentLiquidity multiple times as the price moves from one price range to another and liquidity gets added and removed. Thus, swap fees are always calculated on the entire liquidity required for a swap while the trading volume computation counts only the price ranges that include the new price after a swap is done.

Recommended Mitigation Steps

Calculated volume per liquidity on the entire liquidity required for a swap. Use the swap fee amount calculation as a reference.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L753 https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L833

Vulnerability details

Impact

In situations when calls to AlgebraVirtualPool fail, swapping would fail as well until the issues with AlgebraVirtualPool are resolved or activeIncentive is unset.

Proof of Concept

During swapping, external calls are made to a AlgebraVirtualPool contract when an activeIncentive address is set:

• https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L753
• https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L833

Either of these calls can fail, which will result in a failed swap. In the case when AlgebraVirtualPool fails consistently (due to a misconfiguration or a bug in the contract), swapping would be not possible until the issues in the AlgebraVirtualPool contract are resolved or until activeIncentive is unset.

Recommended Mitigation Steps

Short term, handle reverts in the external calls to AlgebraVirtualPool. Long term, consider using the pull pattern to synchronize changes: instead of pushing changes to AlgebraVirtualPool from AlgebraPool, pull necessary data from AlgebraPool by calling it from AlgebraVirtualPool when needed.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L604 https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L610

Vulnerability details

Impact

Pool users can be tricked into buying tokens from a destroyed token contract. Due to a missing contract existence check when pool transfers tokens, traders would send their tokens to a pool and get no tokens in exchange because the other token contract was destroyed by a malicious actor.

Proof of Concept

During swapping, the Pool contract exchanges tokens: it takes some amount of one pool token from a user and sends some amount of the other pool token in exchange. Depending on the swap direction, it sends either token0 or token1:

1. when user providers some amount of token0, token1 is sent from the pool to the user;
2. when user providers some amount of token1, token0 is sent from the pool to the user.

However, the TransferHelper.safeTransfer function doesn't check if a token contract exists. As per Solidity documentation:

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Thus, when the token contract a pool sends tokens from was destroyed (assuming it's a malicious token contract), transferring tokens from the contract to a user would always succeed. No actual token transfer would be made because there would be no code at the address where the malicious token contract was deployed.

Exploit Scenario

1. a malicious actor deploys an ERC20 token contract with a hidden call to selfdestruct;
2. the token can have a symbol of a legit token (e.g. "USDC", "WETH", etc.) to trick users into thinking this is a valid token;
3. the same malicious actor deploys an AlgebraPool with one token being the malicious token and the other token being a real token;
4. users provide liquidity into the pool and use it to swap tokens;
5. at some point, the malicious actors calls selfdestruct on the malicious token contract;
6. all further swaps in the pool would lead to users losing funds because they get no tokens in return.

Recommended Mitigation Steps

In the TransferHelper.safeTransfer function, consider adding a contract existence check. This will guarantee that, when a token contract doesn't exist, no swapping is made and users' funds don't get stuck in a pool contract without users receiving no tokens in exchange.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L193 https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPoolDeployer.sol#L51

Vulnerability details

Impact

A malicious actor can initialize a newly deployed pool and set an incorrect price, without providing liquidity. A depositor of initial liquidity could mistakenly provide liquidity at the price set by the malicious actor, allowing the latter to swap tokens at the incorrect price and extract profit.

Proof of Concept

Pool contracts are initialized via the initialize function, which is not called during deployment:

1. it's not called in the constructor;
2. it's not called right after a pool is deployed;
3. it's also not called in the factory. In the initialize function, the initial price is set.

However, the caller is not required to deposit initial liquidity. Thus, the initial price can be set arbitrarily.

In a case when a malicious actor sets an incorrect initial price, an initial liquidity depositor will be forced to deposit liquidity at the incorrect price:

1. in the mint function, the amount of provided liquidity is specified as liquidityDesired, not actual token amounts;
2. actual token amounts are calculated later in the function;
3. actual token amounts are calculated based on the current price, which is set by a malicious actor;
4. initial liquidity depositor is then required to deposit the token amounts calculated above.

Exploit Scenario

1. Alice deploys a pool of tokens X and Y; the current market price is 1 X = 10 Y;
2. Bob calls initialize and sets the current price to 1 X = 100 Y;
3. Alice calls mint to deposit liquidity; the amounts she has to deposit are calculated from the price set by Bob;
4. Bob swaps tokens at the incorrect price, gaining profit from the unbalanced liquidity provided by Alice.

Recommended Mitigation Steps

Consider following solutions:

1. call initialize during pool deployment and let deployer choose the price;
2. disallow setting the initial price without providing liquidity.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/libraries/PriceMovementMath.sol#L157

Vulnerability details

Impact

Pool price can be manipulated and set to an arbitrary value after pool was initialized and before initial liquidity was provided. A manipulation is also possible when the liquidity was removed from a tick.

Proof of Concept

AlgebraPool p = AlgebraPool(f.createPool(address(token0), address(token1)));

// Initialize to some price.
p.initialize(TickMath.MIN_SQRT_RATIO+1);
// Swap when there's zero liquidity. The swap amount is 1 wei, the target
// price is different from the current one, and it's much bigger.
p.swap(
    address(this),
    false, // zeroToOne
    1,
    TickMath.MAX_SQRT_RATIO-1, // limit price
    ""
);

(uint160 price, int24 tick, , , , ,) = p.globalState();
// The swap allowed to manipulate the current price of the pool and set
// it to an arbitrary value.
assertEq(uint256(price), TickMath.MAX_SQRT_RATIO-1);
assertEq(tick, TickMath.MAX_TICK-1);

In the above PoC, the swap amount is 1 wei and the limit price is arbitrary. When a swap amount is 1 wei:

1. amountAvailableAfterFee is 0 on this line ((1 wei * (1e6 - fee)) // 1e6 == 0);
2. resultPrice equals targetPrice here; targetPrice is the limit price provided by user in the swap call;
3. resultPrice is returned as currentPrice here;
4. the new current price is stored in globalState here.

Exploit Scenario

1. Alice deploys a pool of tokens X and Y;
2. Alice calls initialize and sets the current price to 1 X = 10 Y;
3. Bob calls swap with 1 wei and sets the limit price to 1 X = 100 Y;
4. Alice calls mint to deposit liquidity; the amounts she has to deposit are calculated from the price set by Bob;
5. Bob swaps tokens at the incorrect price, gaining profit from the unbalanced liquidity provided by Alice.

Recommended Mitigation Steps

Consider disallowing swaps on ticks that don't have liquidity. This seems reasonable because it's liquidity that makes swaps possible, so, when there's no liquidity, swapping shouldn't be possible.

[L-01] Misleading comment

Targets

• https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/libraries/DataStorage.sol#L49 The comment says there's a multiplication by dt, however it's not there.

[L-02] Missing modifier

Targets

• https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L494 Missing onlyValidTicks modifier. The function takes bottomTick and topTick in arguments.