QuickSwap and StellaSwap contest - Aeros's results

Low Severity Findings:

The contracts should be deployed using the most up-to-date, recent version of Solidity. Consider switching to a 0.8.x version, preferably a stable one (e.g. 0.8.4 - 0.8.7).

Using =0.7.6:

• src/core/contracts/AlgebraFactory.sol
• src/core/contracts/AlgebraPool.sol
• src/core/contracts/AlgebraPoolDeployer.sol
• src/core/contracts/DataStorageOperator.sol
• src/core/contracts/libraries/AdaptiveFee.sol
• src/core/contracts/libraries/Constants.sol
• src/core/contracts/libraries/DataStorage.sol
• src/core/contracts/libraries/PriceMovementMath.sol
• src/core/contracts/libraries/TickManager.sol
• src/core/contracts/libraries/TickTable.sol
• src/core/contracts/libraries/TokenDeltaMath.sol
• src/core/contracts/base/PoolImmutables.sol
• src/core/contracts/base/PoolState.sol

In src/core/contracts/AlgebraFactory.sol, the setOwner(), setFarmingAddress(), and setVaultAddress() methods should have checks to ensure the address is not accidentally set to the 0-address.

Original:

/// @inheritdoc IAlgebraFactory
function setOwner(address _owner) external override onlyOwner {
  require(owner != _owner); // should check to ensure it isn't 0 address
  emit Owner(_owner);
  owner = _owner;
}

/// @inheritdoc IAlgebraFactory
function setFarmingAddress(address _farmingAddress) external override onlyOwner {
  require(farmingAddress != _farmingAddress); // should check to ensure it isn't 0 address
  emit FarmingAddress(_farmingAddress);
  farmingAddress = _farmingAddress;
}

/// @inheritdoc IAlgebraFactory
function setVaultAddress(address _vaultAddress) external override onlyOwner {
  require(vaultAddress != _vaultAddress); // should check to ensure it isn't 0 address
  emit VaultAddress(_vaultAddress);
  vaultAddress = _vaultAddress;
}

Fix:

/// @inheritdoc IAlgebraFactory
function setOwner(address _owner) external override onlyOwner {
  require(owner != _owner);
  require(_owner != address(0));
  emit Owner(_owner);
  owner = _owner;
}

/// @inheritdoc IAlgebraFactory
function setFarmingAddress(address _farmingAddress) external override onlyOwner {
  require(farmingAddress != _farmingAddress);
  require(_farmingAddress != address(0));
  emit FarmingAddress(_farmingAddress);
  farmingAddress = _farmingAddress;
}

/// @inheritdoc IAlgebraFactory
function setVaultAddress(address _vaultAddress) external override onlyOwner {
  require(vaultAddress != _vaultAddress);
  require(_vaultAddress != address(0));
  emit VaultAddress(_vaultAddress);
  vaultAddress = _vaultAddress;
}

Non-Critical Findings:

Use scientific notation rather than exponentiation for big numbers. Places where this could be implemented:

• src/core/contracts/DataStorageOperator.sol (line 139)
• src/core/contracts/libraries/AdaptiveFee.sol (lines 53 and 60)
• src/core/contracts/libraries/DataStorage.sol (line 53)

Gas Optimizations:

Splitting up require()/revert() statements that use && saves gas.

Places where require() statements exist with && (and should be split up):

• src/core/contracts/AlgebraFactory.sol (line 110)
• src/core/contracts/AlgebraPool.sol (lines 739, 743, 953)
• src/core/contracts/DataStorageOperator.sol (line 46)

In src/core/contracts/libraries/DataStorage.sol, the getTimePoints() method calls secondsAgos.length multiple times (including in a for loop). It should be cached in a stack variable rather than being accessed every time (especially in the loop).

Original:

function getTimepoints(
    Timepoint[UINT16_MODULO] storage self,
    uint32 time,
    uint32[] memory secondsAgos,
    int24 tick,
    uint16 index,
    uint128 liquidity
  )
    internal
    view
    returns (
      int56[] memory tickCumulatives,
      uint160[] memory secondsPerLiquidityCumulatives,
      uint112[] memory volatilityCumulatives,
      uint256[] memory volumePerAvgLiquiditys
    )
  {
    tickCumulatives = new int56[](secondsAgos.length); // since this is called 4 times, it should be cached in stack variable
    secondsPerLiquidityCumulatives = new uint160[](secondsAgos.length);
    volatilityCumulatives = new uint112[](secondsAgos.length);
    volumePerAvgLiquiditys = new uint256[](secondsAgos.length);

    uint16 oldestIndex;
    // check if we have overflow in the past
    uint16 nextIndex = index + 1; // considering overflow
    if (self[nextIndex].initialized) {
      oldestIndex = nextIndex;
    }

    Timepoint memory current;
    for (uint256 i = 0; i < secondsAgos.length; i++) {
      // .length should not be called on every loop, cache it in a stack variable
      current = getSingleTimepoint(self, time, secondsAgos[i], tick, index, oldestIndex, liquidity);
      (tickCumulatives[i], secondsPerLiquidityCumulatives[i], volatilityCumulatives[i], volumePerAvgLiquiditys[i]) = (
        current.tickCumulative,
        current.secondsPerLiquidityCumulative,
        current.volatilityCumulative,
        current.volumePerLiquidityCumulative
      );
    }
  }

Fix:

function getTimepoints(
    Timepoint[UINT16_MODULO] storage self,
    uint32 time,
    uint32[] memory secondsAgos,
    int24 tick,
    uint16 index,
    uint128 liquidity
  )
    internal
    view
    returns (
      int56[] memory tickCumulatives,
      uint160[] memory secondsPerLiquidityCumulatives,
      uint112[] memory volatilityCumulatives,
      uint256[] memory volumePerAvgLiquiditys
    )
  {
    uint256 secondsAgosLength = secondsAgos.length;
    tickCumulatives = new int56[](secondsAgosLength);
    secondsPerLiquidityCumulatives = new uint160[](secondsAgosLength);
    volatilityCumulatives = new uint112[](secondsAgosLength);
    volumePerAvgLiquiditys = new uint256[](secondsAgosLength);
 
    uint16 oldestIndex;
    // check if we have overflow in the past
    uint16 nextIndex = index + 1; // considering overflow
    if (self[nextIndex].initialized) {
      oldestIndex = nextIndex;
    }

    Timepoint memory current;
    for (uint256 i = 0; i < secondsAgosLength; i++) {
      current = getSingleTimepoint(self, time, secondsAgos[i], tick, index, oldestIndex, liquidity);
      (tickCumulatives[i], secondsPerLiquidityCumulatives[i], volatilityCumulatives[i], volumePerAvgLiquiditys[i]) = (
        current.tickCumulative,
        current.secondsPerLiquidityCumulative,
        current.volatilityCumulative,
        current.volumePerLiquidityCumulative
      );
    }
  }

In src/core/contracts/libraries/DataStorage.sol (at line 307), i++ is used. ++i uses less gas, and so it could be switched to this.