QuickSwap and StellaSwap contest - 0xmatt's results

Lines of code

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

Vulnerability details

Impact

When a pool is deployed, the pool's initial globalState.price value is unset (and therefore 0). The initial price value is set via a call to AlgebraPool.sol#initialize. As the function is accessible by anyone, it could be front-run to set an arbitrary price that could be used to drain initial deposits.

Proof of Concept

Assuming the real value of USDC/DAI is at a point in time 1 USDC == 1 DAI:

1. Alice creates a USDC-DAI Algebra pool. After creation, Alice calls initialize() via a separate transaction.
2. Bob front-runs Alice's initialize() transaction, setting a price such that 1 USDC = 10 DAI.
3. Alice calls mint() and deposits $100,000 of assets, with a DAI:USDC ratio of 10:1.
4. Bob swaps USDC for DAI at an unfair price.

This finding is inherited from UniSwap V3's pool initialization pattern.

Tools Used

VSCode, Remix

Recommended Mitigations

Because of the structure of AlgebraSwap, passing parameters at the factory level to set price during the deployment process may be suboptimal as real-world prices could change by the time initial deposits are made. Implementing a modifier restricting access to the pool creator would stop rogue initialize() calls. The modifier may also benefit from including the factory owner's address, in case of pools being created but never initialized (e.g. through griefing with popular tokens where pools don't yet exist).

DataStorageOperator.sol's initialize() function has an onlyPool modifier to stop others from front-running data storage initialization.

There are two obvious approaches to this, each with pros and cons.

The originating EOA could be identified in the pool constructor via tx.origin. The disadvantage is that if a pool is created via a contract, tx.origin may not be available or accurate under all circumstances. Using tx.origin also comes with it's own issues.

The other approach is to pass msg.sender from the factory via the deployer call as an argument, which in turn would pass this on to the pool's constructor. This is preferable as the pool creation source is properly preserved.

These approaches retain the contract's current structure, which may be preferable to moving price initialization purely to the constructor.

QA Report (low / non-critical)

L-01: Missing Zero-Address Checks

Zero-address checks are a common best practice for input validation of critical address parameters, particularly in constructors and setters. Accidental use of zero-addresses may result in unexpected exceptions, failures or require the redeployment of contracts.

The project already uses zero-address checks in some places.

If it is necessary to be able to set an address to 0 consider implementing a two-step or renounce function if the address is to be subject to access control.

The following instances were identified where zero-address checks should be added.

In AlgebraFactory.sol:

• Constructor - _poolDeployer, _vaultAddress
• createPool() - tokenA, tokenB, token1 (tokenA and tokenB if require is moved before assignment, otherwise token0 and token1 require the zero check)
• setOwner() - _owner
• setFarmingAddress() - _vaultAddress

In AlgebraPool.sol:

• mint() - Neither sender nor recipient are checked for zero values. Sender is used in a TransferHelper.safeTransfer() call. Recipient is passed to _updatePositionTicksAndFees().
• collect() - recipient
• swap() - recipient
• setIncentive() - virtualPoolAddress

L-02: ## Single-Step transfer patterns in use

Several address parameters in AlgebraFactory are changeable in a single step. Although these functions are locked to the owner, in the event that the owner sets an incorrect or null value, some of these settings could have a significant impact on the contract's function, such as permanently losing ownership. This could require a redeployment to address, which could be disruptive.

Consider a two-step nominate and claim process, where the change is nominated by the owner, but must be claimed by the process recipient to succeed. This ensures the updated address is valid.

Instances found:

• AlgebraFactory#setOwner() - Changes ownership
• AlgebraFactory#setFarmingAddress() - Changes farming contract address, which could lead to failed farming functionality.
• AlgebraFactory#setVaultAddress - Changes vault address, which could lead to stuck tokens

In other files:

• AlgebraPoolDeployer#setFactory() - Changes Factory ownership, which is address(0) after construction. Can only be set when zero. Setting factory via parameter at construction may be more efficient.

CodeArena Gas Optimization Report

G01 - Use !=0 Instead of >0 On Uints

Details

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0. This should only be done with unsigned variables, immutables and constants, unless the author knows that the value will always be 0 or positive.

Instances

src/core/contracts/AlgebraPool.sol::224 => require(currentLiquidity > 0, 'NP'); // Do not recalculate the empty ranges src/core/contracts/AlgebraPool.sol::228 => if (_liquidityCooldown > 0) { src/core/contracts/AlgebraPool.sol::434 => require(liquidityDesired > 0, 'IL'); src/core/contracts/AlgebraPool.sol::469 => require(liquidityActual > 0, 'IIL2'); src/core/contracts/AlgebraPool.sol::617 => if (communityFee > 0) { src/core/contracts/AlgebraPool.sol::667 => if (communityFee > 0) { src/core/contracts/AlgebraPool.sol::808 => if (cache.communityFee > 0) { src/core/contracts/AlgebraPool.sol::814 => if (currentLiquidity > 0) cache.totalFeeGrowth += FullMath.mulDiv(step.feeAmount, Constants.Q128, currentLiquidity); src/core/contracts/AlgebraPool.sol::898 => require(_liquidity > 0, 'L'); src/core/contracts/AlgebraPool.sol::924 => if (paid0 > 0) { src/core/contracts/AlgebraPool.sol::927 => if (_communityFeeToken0 > 0) { src/core/contracts/AlgebraPool.sol::938 => if (paid1 > 0) { src/core/contracts/AlgebraPool.sol::941 => if (_communityFeeToken1 > 0) { src/core/contracts/DataStorageOperator.sol::46 => require(_feeConfig.gamma1 != 0 && _feeConfig.gamma2 != 0 && _feeConfig.volumeGamma != 0, 'Gammas must be > 0'); src/core/contracts/DataStorageOperator.sol::138 => if (volume >= 2**192) volumeShifted = (type(uint256).max) / (liquidity > 0 ? liquidity : 1); src/core/contracts/DataStorageOperator.sol::139 => else volumeShifted = (volume << 64) / (liquidity > 0 ? liquidity : 1); src/core/contracts/libraries/DataStorage.sol::80 => last.secondsPerLiquidityCumulative += ((uint160(delta) << 128) / (liquidity > 0 ? liquidity : 1)); // just timedelta if liquidity == 0 src/core/contracts/libraries/PriceMovementMath.sol::52 => require(price > 0); src/core/contracts/libraries/PriceMovementMath.sol::53 => require(liquidity > 0);

G02 - Cache Array Length Outside Of Loop

Details

Enumerating storage array length inside a loop creates an extra SLOAD operation for each iteration after the first. It is more gas efficient to use a temporary in-function variable to store the length, then read that value in the loop.

Instances

src/core/contracts/libraries/DataStorage.sol::307 => for (uint256 i = 0; i < secondsAgos.length; i++) {

G03 - unchecked { ++i;} is cheaper than i++ or i=i+1

Gas costs vary for different types of increment. In loops, the most gas-optimal option is unchecked {++i};, followed by ++i. In DataStorage.sol the following loop is defined:

for (uint256 i = 0; i < secondsAgos.length; i++) {

Aside from storing array length outside of the loop (G02), this loop can be gas optimized as follows:

for (uint256 i = 0; i < secondsAgos.length; ++i) {
	// More readable, predecrement

If the authors are confident that secondsAgos.length will never exceed uint256 size using unchecked will provide further gas savings:

for (uint256 i = 0; i < secondsAgos.length;) {
	// ... rest of loop code to end of L314
	unchecked { ++i; }
}