QuickSwap and StellaSwap contest - Aymen0909's results

QA Report

Summary

Findings

1- Solidity version less than 0.8 should use safeMath library :

Solidity version less than 0.8 doesn't come with implicit overflow and underflow checks on unsigned integers which will cause arithmetic errors on operations such as addition or substraction, this could provoke vulnerabilities in the code and could cause loss of funds or protocol logic errors.

Impact - Low Risk

Mitigation

There are 2 options to fix this issue :

• Consider using more recent solidity versions 0.8+.

• Use openzeppelin safeMath library.

2- Changing ownership missing check for address(0) in setOwner function :

The setOwner function from the AlgebraFactory contract is missing a check for address(0) when setting a new owner which could lead to the loss of the ownership of the contract if an error occurs.

Impact - Low Risk

Proof of Concept

Instances include:

File: src/core/contracts/AlgebraFactory.sol

function setOwner(address _owner)

Mitigation

There are 2 options to fix this issue :

• The simple one is to add non-zero address check on the new owner address in the setOwner function to avoid burning ownership by accident.

• The safer option is to use a two-step ownership transfer process in order to avoid giving ownership to a wrong address or zero address by accident, because with the two-step ownership the new owner must approve the transfer.

3- Missing checks for address(0) when assigning values to address state variables :

Constructors should check that the values written in an immutable addresses variables are not the zero address.

Impact - Low Risk

Proof of Concept

Instances include:

File: src/core/contracts/AlgebraFactory.sol

poolDeployer = _poolDeployer;

File: src/core/contracts/DataStorageOperator.sol

factory = msg.sender;

pool = _pool;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

4- Setters should check the input value and revert if it's the zero address or zero :

When a setting a new value to a state variable the setter function should check the new value and revert if it's the zero address or zero.

Impact - Low Risk

Proof of Concept

Instances include:

File: src/core/contracts/AlgebraFactory.sol

function setOwner(address _owner)

function setFarmingAddress(address _farmingAddress)

function setVaultAddress(address _vaultAddress)

File: src/core/contracts/AlgebraPool.sol

function setIncentive(address virtualPoolAddress)

Mitigation

Add non-zero address checks in the setters for the instances aforementioned.

5- require()/revert() statements should have descriptive reason strings :

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/core/contracts/libraries/PriceMovementMath.sol

52       require(price > 0);
53       require(liquidity > 0);
70       require((product = amount * price) / amount == price);
71       require(liquidityShifted > product);
87       require(price > quotient);

File: src/core/contracts/libraries/TokenDeltaMath.sol

30       require(priceDelta < priceUpper);
51       require(priceUpper >= priceLower);

File: src/core/contracts/AlgebraFactory.sol

43       require(msg.sender == owner);
60       require(tokenA != tokenB);
62       require(token0 != address(0));
63       require(poolByPair[token0][token1] == address(0));
78       require(owner != _owner);
85       require(farmingAddress != _farmingAddress);
92       require(vaultAddress != _vaultAddress);

File: src/core/contracts/AlgebraPool.sol

55       require(msg.sender == IAlgebraFactory(factory).owner());
122      require(_lower.initialized);
134      require(_upper.initialized);
229      require((_blockTimestamp() - lastLiquidityAddTimestamp) >= _liquidityCooldown);
953      require((communityFee0 <= Constants.MAX_COMMUNITY_FEE) && (communityFee1 <= Constants.MAX_COMMUNITY_FEE));
960      require(msg.sender == IAlgebraFactory(factory).farmingAddress());
968      require(newLiquidityCooldown <= Constants.MAX_LIQUIDITY_COOLDOWN && liquidityCooldown != newLiquidityCooldown);

File: src/core/contracts/AlgebraPoolDeployer.sol

22       require(msg.sender == factory);
27       require(msg.sender == owner);
37       require(_factory != address(0));
38       require(factory == address(0));

File: src/core/contracts/DataStorageOperator.sol

43       require(msg.sender == factory || msg.sender == IAlgebraFactory(factory).owner());

Mitigation

Add reason strings to the aforementioned require statements for better comprehension.

6- Use more recent solidity versions :

Impact - NON CRITICAL

All contracts contained in this project use an old solidity version pragma solidity =0.7.6, consider using a more recent solidity version to get access to new features which save gas and to avoid old versions vulnerabilities (such as under/overflows) :

• Solidity version of at least 0.8.4 to get access to Custom errors which are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

• Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers which will prevent arithmetic errors in the code and the developers will not have to use safeMath library.

7- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/core/contracts/libraries/AdaptiveFee.sol

returns (uint16 fee)

File: src/core/contracts/libraries/DataStorage.sol

returns (Timepoint memory targetTimepoint)

returns (uint88 volatilityAverage, uint256 volumePerLiqAverage)

File: src/core/contracts/libraries/PriceMovementMath.sol

returns (uint160 resultPrice)

returns (uint160 resultPrice)

returns (uint160 resultPrice)

File: src/core/contracts/libraries/TickManager.sol

returns (int128 liquidityDelta)

File: src/core/contracts/libraries/TickTable.sol

returns (uint8 mostBitPos)

returns (int24 nextTick, bool initialized)

File: src/core/contracts/AlgebraPool.sol

returns (bool initialized, uint32 blockTimestamp, int56 tickCumulative, uint160 secondsPerLiquidityCumulative, uint88 volatilityCumulative, int24 averageTick, uint144 volumePerLiquidityCumulative)

returns (int56 innerTickCumulative, uint160 innerSecondsSpentPerLiquidity, uint32 innerSecondsSpent)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

8- 2**<N> - 1 should be re-written as type(uint<N>).max :

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/core/contracts/libraries/PriceMovementMath.sol

if (volume >= 2**192)

Mitigation

Replace the aforementioned statements for better readability.

9- constant should be defined rather than using magic numbers :

It is best practice to use constant variables rather than hex/numeric literal values to make the code easier to understand and maintain, but if they are used those numbers should be well docummented.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/core/contracts/libraries/PriceMovementMath.sol

1e6

File: src/core/contracts/DataStorageOperator.sol

return AdaptiveFee.getFee(volatilityAverage / 15, volumePerLiqAverage, feeConfig);

Mitigation

Replace the hex/numeric literals aforementioned with constants.

Gas Optimizations

Findings

Findings

1- State variables only set in the constructor should be declared immutable :

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There is 1 instance of this issue:

File: src/core/contracts/AlgebraPoolDeployer.sol Line 19

address private owner;

Since there no way of changing the AlgebraPoolDeployer contract owner after deployment, it's better to declare the owner address as immutable.

2- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

There is 1 instance of this issue:

File: src/core/contracts/AlgebraPool.sol Line 675-690

struct SwapCalculationCache {
    uint256 communityFee; // The community fee of the selling token, uint256 to minimize casts
    uint128 volumePerLiquidityInBlock;
    int56 tickCumulative; // The global tickCumulative at the moment
    uint160 secondsPerLiquidityCumulative; // The global secondPerLiquidity at the moment
    bool computedLatestTimepoint; //  if we have already fetched _tickCumulative_ and _secondPerLiquidity_ from the DataOperator
    int256 amountRequiredInitial; // The initial value of the exact input\output amount
    int256 amountCalculated; // The additive amount of total output\input calculated trough the swap
    uint256 totalFeeGrowth; // The initial totalFeeGrowth + the fee growth during a swap
    uint256 totalFeeGrowthB;
    IAlgebraVirtualPool.Status incentiveStatus; // If there is an active incentive at the moment
    bool exactInput; // Whether the exact input or output is specified
    uint16 fee; // The current dynamic fee
    int24 startTick; // The tick at the start of a swap
    uint16 timepointIndex; // The index of last written timepoint
    }

This should be rearranged as follow to save gas :

struct SwapCalculationCache {
    uint256 communityFee; // The community fee of the selling token, uint256 to minimize casts
    uint128 volumePerLiquidityInBlock;
    int56 tickCumulative; // The global tickCumulative at the moment
    uint160 secondsPerLiquidityCumulative; // The global secondPerLiquidity at the moment
    uint16 fee; // The current dynamic fee
    int24 startTick; // The tick at the start of a swap
    uint16 timepointIndex; // The index of last written timepoint
    bool computedLatestTimepoint; //  if we have already fetched _tickCumulative_ and _secondPerLiquidity_ from the DataOperator
    int256 amountRequiredInitial; // The initial value of the exact input\output amount
    int256 amountCalculated; // The additive amount of total output\input calculated trough the swap
    uint256 totalFeeGrowth; // The initial totalFeeGrowth + the fee growth during a swap
    uint256 totalFeeGrowthB;
    IAlgebraVirtualPool.Status incentiveStatus; // If there is an active incentive at the moment
    bool exactInput; // Whether the exact input or output is specified
    }

In this case the fee, startTick, timepointIndex variables are saved in the same slot as secondsPerLiquidityCumulative.

3- Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead :

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size as you can check here.

So use uint256/int256 for state variables and then downcast to lower sizes where needed.

There are 3 instances of this issue:

File: src/core/contracts/base/PoolState.sol Line 26

uint128 public override liquidity;

File: src/core/contracts/base/PoolState.sol Line 27

uint128 internal volumePerLiquidityInBlock;

File: src/core/contracts/base/PoolState.sol Line 30

uint32 public override liquidityCooldown;

4- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

There are 4 instances of this issue:

File: src/core/contracts/AlgebraPool.sol Line 257

_position.fees0 += fees0;

File: src/core/contracts/AlgebraPool.sol Line 258

_position.fees1 += fees1;

File: src/core/contracts/AlgebraPool.sol Line 931

totalFeeGrowth0Token += FullMath.mulDiv(paid0 - fees0, Constants.Q128, _liquidity);

File: src/core/contracts/AlgebraPool.sol Line 945

totalFeeGrowth1Token += FullMath.mulDiv(paid1 - fees1, Constants.Q128, _liquidity);

5- Splitting require() statements that uses && saves gas (saves 8 gas per &&) :

There are 6 instances of this issue :

File: src/core/contracts/AlgebraFactory.sol

110      require(gamma1 != 0 && gamma2 != 0 && volumeGamma != 0, 'Gammas must be > 0');

File: src/core/contracts/AlgebraPool.sol

739      require(limitSqrtPrice < currentPrice && limitSqrtPrice > TickMath.MIN_SQRT_RATIO, 'SPL');
743      require(limitSqrtPrice > currentPrice && limitSqrtPrice < TickMath.MAX_SQRT_RATIO, 'SPL');
953      require((communityFee0 <= Constants.MAX_COMMUNITY_FEE) && (communityFee1 <= Constants.MAX_COMMUNITY_FEE));
968      require(newLiquidityCooldown <= Constants.MAX_LIQUIDITY_COOLDOWN && liquidityCooldown != newLiquidityCooldown);

File: src/core/contracts/DataStorageOperator.sol

46       require(_feeConfig.gamma1 != 0 && _feeConfig.gamma2 != 0 && _feeConfig.volumeGamma != 0, 'Gammas must be > 0'); 

6- It costs more gas to initialize variables to zero than to let the default of zero be applied (saves ~3 gas per instance) :

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There is 1 instance of this issue:

File: src/core/contracts/libraries/DataStorage.sol Line 307

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

7- Use of ++i cost less gas than i++/i=i+1 in for loops :

There is 1 instance of this issue:

File: src/core/contracts/libraries/DataStorage.sol Line 307

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

8- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There is 1 instance of this issue:

File: src/core/contracts/libraries/DataStorage.sol Line 307

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

9- Use more recent solidity versions :

All contracts contained in this project use an old solidity version pragma solidity =0.7.6, consider use a solidity version of at least 0.8.4 to get access to Custom errors which are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

10- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 12 instances of this issue:

File: src/core/contracts/AlgebraFactory.sol

77       function setOwner(address _owner) external override onlyOwner
84       function setFarmingAddress(address _farmingAddress) external override onlyOwner
91       function setVaultAddress(address _vaultAddress) external override onlyOwner
98       function setBaseFeeConfiguration(uint16 alpha1, uint16 alpha2, uint32 beta1, uint32 beta2, uint16 gamma1, uint16 gamma2, uint32 volumeBeta, uint16 volumeGamma, uint16 baseFee) external override onlyOwner

File: src/core/contracts/AlgebraPoolDeployer.sol

36       function setFactory(address _factory) external override onlyOwner
44       function deploy(address dataStorage, address _factory, address token0, address token1) external override onlyFactory

File: src/core/contracts/DataStorageOperator.sol

37        function initialize(uint32 time, int24 tick) external override onlyPool
53        function getSingleTimepoint(uint32 time, uint32 secondsAgo, int24 tick, uint16 index, uint128 liquidity) external view override onlyPool
88        function getTimepoints(uint32 time, uint32[] memory secondsAgos, int24 tick, uint16 index, uint128 liquidity) external view override onlyPool
110       function getAverages(uint32 time, int24 tick, uint16 index, uint128 liquidity) external view override onlyPool
120       function write(uint16 index, uint32 blockTimestamp, int24 tick, uint128 liquidity, uint128 volumePerLiquidity) external override onlyPool
150       function getFee(uint32 _time, int24 _tick, uint16 _index, uint128 _liquidity) external view override onlyPool