QuickSwap and StellaSwap contest - delfin454000's results

Named return variables are not used when a function returns

The existence of a named return variable that is never used (here, mostBitPos) is potentially perplexing

TickTable.sol: L46

  function getMostSignificantBit(uint256 word) internal pure returns (uint8 mostBitPos) {

---

---

Missing @param statement

PriceMovementMath.sol: L125-143

  /// @notice Computes the result of swapping some amount in, or amount out, given the parameters of the swap
  /// @dev The fee, plus the amount in, will never exceed the amount remaining if the swap's `amountSpecified` is positive
  /// @param currentPrice The current Q64.96 sqrt price of the pool
  /// @param targetPrice The Q64.96 sqrt price that cannot be exceeded, from which the direction of the swap is inferred
  /// @param liquidity The usable liquidity
  /// @param amountAvailable How much input or output amount is remaining to be swapped in/out
  /// @param fee The fee taken from the input amount, expressed in hundredths of a bip
  /// @return resultPrice The Q64.96 sqrt price after swapping the amount in/out, not to exceed the price target
  /// @return input The amount to be swapped in, of either token0 or token1, based on the direction of the swap
  /// @return output The amount to be received, of either token0 or token1, based on the direction of the swap
  /// @return feeAmount The amount of input that will be taken as a fee
  function movePriceTowardsTarget(
    bool zeroToOne,
    uint160 currentPrice,
    uint160 targetPrice,
    uint128 liquidity,
    int256 amountAvailable,
    uint16 fee
  )

@param statement missing for zeroToOne

---

---

Typos

---

The same typo (an) occurs in all four lines referenced below. I normally wouldn't flag "an" versus "a", but an timestamp comes across as awkward and should be corrected.

DataStorage.sol: L193

DataStorage.sol: L199

DataStorage.sol: L269

DataStorage.sol: L393

Example (DataStorage.sol: L193):

  /// @dev Reverts if an timepoint at or before the desired timepoint timestamp does not exist.

Change an to a in each case

---

TickManager.sol: L27

    bool initialized; // these 8 bits are set to prevent fresh sstores when crossing newly initialized ticks

Change sstores to stores

---

---

Long single line comments

In theory, comments over 79 characters should wrap using multi-line comment syntax. Even if somewhat longer comments are acceptable and a scroll bar is provided, very long comments can interfere with readability. Below are five examples of code with lines that could benefit from wrapping, as shown:

---

AlgebraPool.sol: L345

      // if liquidityDelta is negative and the tick was toggled, it means that it should not be initialized anymore, so we delete it

Suggestion:

      // If liquidityDelta is negative and the tick was toggled, it means that
      //   it should not be initialized anymore, so we delete it.

---

DataStorage.sol: L360

  /// @notice Initialize the dataStorage array by writing the first slot. Called once for the lifecycle of the timepoints array

Suggestion:

  /// @notice Initialize the dataStorage array by writing the first slot.
  ///   Called once for the lifecycle of the timepoints array.

---

DataStorage.sol: L265-276

  /// @notice Returns the accumulator values as of each time seconds ago from the given time in the array of `secondsAgos`
  /// @dev Reverts if `secondsAgos` > oldest timepoint
  /// @param self The stored dataStorage array
  /// @param time The current block.timestamp
  /// @param secondsAgos Each amount of time to look back, in seconds, at which point to return an timepoint
  /// @param tick The current tick
  /// @param index The index of the timepoint that was most recently written to the timepoints array
  /// @param liquidity The current in-range pool liquidity
  /// @return tickCumulatives The tick * time elapsed since the pool was first initialized, as of each `secondsAgo`
  /// @return secondsPerLiquidityCumulatives The cumulative seconds / max(1, liquidity) since the pool was first initialized, as of each `secondsAgo`
  /// @return volatilityCumulatives The cumulative volatility values since the pool was first initialized, as of each `secondsAgo`
  /// @return volumePerAvgLiquiditys The cumulative volume per liquidity values since the pool was first initialized, as of each `secondsAgo`

Suggestion:

  /// @notice Returns the accumulator values as of each time seconds ago
  ///   from the given time in the array of `secondsAgos`.
  /// @dev Reverts if `secondsAgos` > oldest timepoint
  /// @param self The stored dataStorage array
  /// @param time The current block.timestamp
  /// @param secondsAgos Each amount of time to look back, in seconds,
  ///   at which point to return an timepoint.
  /// @param tick The current tick
  /// @param index The index of the timepoint that was most recently
  ///   written to the timepoints array.
  /// @param liquidity The current in-range pool liquidity
  /// @return tickCumulatives The tick * time elapsed since the pool was 
  ///   first initialized, as of each `secondsAgo`.
  /// @return secondsPerLiquidityCumulatives The cumulative seconds / max(1, liquidity) 
  ///   since the pool was first initialized, as of each `secondsAgo`.
  /// @return volatilityCumulatives The cumulative volatility values since the
  ///   pool was first initialized, as of each `secondsAgo`.
  /// @return volumePerAvgLiquiditys The cumulative volume per liquidity values
  ///   since the pool was first initialized, as of each `secondsAgo`.

---

TickTable.sol: L9

/// @dev The mapping uses int16 for keys since ticks are represented as int24 and there are 256 (2^8) values per word.

Suggestion:

/// @dev The mapping uses int16 for keys since ticks are represented as int24
///   and there are 256 (2^8) values per word

---

TokenDeltaMath.sol: L22

  /// @return token0Delta Amount of token0 required to cover a position of size liquidity between the two passed prices

Suggestion:

  /// @return token0Delta Amount of token0 required to cover a position of
  ///   size liquidity between the two passed prices.

---

---

Missing require revert string

A require message should be included to enable users to understand the reason for failure

AlgebraFactory.sol: L43

    require(msg.sender == owner);

Recommendation: Add a brief (<= 32 char) explanatory message.

Similarly for each of the 26 requires referenced below. Also, consider using custom error codes (which would be cheaper than revert strings).

AlgebraFactory.sol: L60

AlgebraFactory.sol: L62

AlgebraFactory.sol: L63

AlgebraFactory.sol: L78

AlgebraFactory.sol: L85

AlgebraFactory.sol: L92

AlgebraPool.sol: L55

AlgebraPool.sol: L122

AlgebraPool.sol: L134

AlgebraPool.sol: L229

AlgebraPool.sol: L953

AlgebraPool.sol: L960

AlgebraPool.sol: L968

AlgebraPoolDeployer.sol: L22

AlgebraPoolDeployer.sol: L27

AlgebraPoolDeployer.sol: L37

AlgebraPoolDeployer.sol: L38

DataStorageOperator.sol: L43

DataStorage.sol: L369

PriceMovementMath.sol: L52

PriceMovementMath.sol: L53

PriceMovementMath.sol: L70

PriceMovementMath.sol: L71

PriceMovementMath.sol: L87

TokenDeltaMath.sol: L30

TokenDeltaMath.sol: L51

---

---

Require message is inadequate

A revert string should provide enough information for users to understand the reason for failure

AlgebraPool.sol: L60

    require(topTick < TickMath.MAX_TICK + 1, 'TUM');

Recommendation: Add a brief (<= 32 char) explanatory message instead of using an acronym that not all users may understand.

Similarly for each of the 23 requires referenced below. Again, consider using custom error codes (which would be cheaper than revert strings).

AlgebraPool.sol: L61

AlgebraPool.sol: L62

AlgebraPool.sol: L194

AlgebraPool.sol: L224

AlgebraPool.sol: L434

AlgebraPool.sol: L469

AlgebraPool.sol: L474

AlgebraPool.sol: L475

AlgebraPool.sol: L608

AlgebraPool.sol: L614

AlgebraPool.sol: L636

AlgebraPool.sol: L641

AlgebraPool.sol: L645

AlgebraPool.sol: L731

AlgebraPool.sol: L733

AlgebraPool.sol: L739

AlgebraPool.sol: L743

AlgebraPool.sol: L898

AlgebraPool.sol: L921

AlgebraPool.sol: L935

DataStorage.sol: L238

TickManager.sol: L96

PoolState.sol: L41

---

---

Avoid use of '&&' within a require function

Splitting into separate require() statements saves gas

AlgebraFactory.sol: L110

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

Recommendation:

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

Similarly for the following five require statements:

AlgebraPool.sol: L739

AlgebraPool.sol: L743

AlgebraPool.sol: L953

AlgebraPool.sol: L968

DataStorageOperator.sol: L46

---

---

Use != 0 instead of > 0 in a require statement if variable is an unsigned integer (uint)

!= 0 should be used where possible since > 0 costs more gas

AlgebraPool.sol: L224

      require(currentLiquidity > 0, 'NP'); // Do not recalculate the empty ranges

Recommendation: Change currentLiquidity > 0 to currentLiquidity != 0

Similarly for the following five require statements:

AlgebraPool.sol: L434

AlgebraPool.sol: L469

AlgebraPool.sol: L898

PriceMovementMath.sol: L52

PriceMovementMath.sol: L53

---

---

Array length should not be looked up during every iteration of a for loop

Since calculating the array length costs gas, it's best to read the length of the array from memory before executing the loop, as demonstrated below:

DataStorage.sol: L307-315

    for (uint256 i = 0; i < secondsAgos.length; 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
      );
    }

Suggestion:

    uint256 totalSecondsAgo = secondsAgos.length; 
    for (uint256 i = 0; i < totalSecondsAgo; 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
      );
    }

---

---

Use ++i instead of i++ to increase count in a for loop

Since use of i++ (or equivalent counter) costs more gas, it is better to use ++i, as demonstrated below:

DataStorage.sol: L307-315

    for (uint256 i = 0; i < secondsAgos.length; 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
      );
    }

Suggestion:

    uint256 totalSecondsAgo = secondsAgos.length; 
    for (uint256 i = 0; i < totalSecondsAgo; ++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
      );
    }

---

---

Avoid use of default "checked" behavior in a for loop

Underflow/overflow checks are made every time ++i (or equivalent counter) is called. Such checks are unnecessary since i is already limited. Therefore, use unchecked {++i} instead, as shown below:

DataStorage.sol: L307-315

    for (uint256 i = 0; i < secondsAgos.length; 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
      );
    }

Suggestion:

    uint256 totalSecondsAgo = secondsAgos.length; 
    for (uint256 i = 0; i < totalSecondsAgo;) {
      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
      );

        unchecked {
          ++i;
      }
    }

---

---