QuickSwap and StellaSwap contest - brgltd's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L589-L623 https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/libraries/TransferHelper.sol#L16-L23

Vulnerability details

Impact

The current transfers will not check if the to address is for an existing token contract. This can cause loss of funds if an user attempts to make a swap for a tokens added to a pool and destructed later.

Proof of Concept

• TokenA gets added to a pool
• The TokenA contract gets destructed later by the token creators or an external factor, and Bob calls swap() for TokenB and TokenA, which will call safeTransfer()

  function swap(
    address recipient,
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external override returns (int256 amount0, int256 amount1) {
    uint160 currentPrice;
    int24 currentTick;
    uint128 currentLiquidity;
    uint256 communityFee;
      ...
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');

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

function safeTransfer(
  address token,
  address to,
  uint256 value
) internal {
  (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20Minimal.transfer.selector, to, value));
  require(success && (data.length == 0 || abi.decode(data, (bool))), 'TF');
}

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

• The transfer will be successful, but Bob won't receive the swap funds for TokenA.

Tools used

Manual review

Recommended Mitigation Steps

Add a check for contract existence in TransferHelper.safeTransfer().

Lines of code

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

Vulnerability details

Impact

An attacker can frontrun the initialize() function in AlgebraPool.sol to set an unexpected price and can cause loss of funds for the initial LP deposit.

Proof of Concept

function initialize(uint160 initialPrice) external override {
  require(globalState.price == 0, 'AI');
  // getTickAtSqrtRatio checks validity of initialPrice inside
  int24 tick = TickMath.getTickAtSqrtRatio(initialPrice);

  uint32 timestamp = _blockTimestamp();
  IDataStorageOperator(dataStorageOperator).initialize(timestamp, tick);

  globalState.price = initialPrice;
  globalState.unlocked = true;
  globalState.tick = tick;

  emit Initialize(initialPrice, tick);
}

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

1. An user creates a pool for tokenA and tokenB, e.g. USDC:DAI.
2. An attacker frontruns the AlgebraPool.initialize() function and set a incorrect price for the pool.
3. The original user deposits the tokens.
4. The attacker swaps the tokens at an incorrect price, at the expense of the original user and causing loss of funds.

Recommended Mitigation Steps

Ensure initialize() has access control - can only be called by the owner or by trusted addresses - e.g. by quickswap frontends or verified external addresses.

Another alternative would be to run initialize in the constructor since it should only be initialized once.

[01] Critical changes should use two-step procedure

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

function setOwner(address _owner) external override onlyOwner {

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

function setFarmingAddress(address _farmingAddress) external override onlyOwner {

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

function setVaultAddress(address _vaultAddress) external override onlyOwner {

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

function setCommunityFee(uint8 communityFee0, uint8 communityFee1) external override lock onlyFactoryOwner {

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

function setIncentive(address virtualPoolAddress) external override {

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

function setLiquidityCooldown(uint32 newLiquidityCooldown) external override onlyFactoryOwner {

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

[02] Replace assert with require

assert(false);

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

Assert should be avoided in production code. As described on the solidity docs. "The assert function creates an error of type Panic(uint256). … Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix." Even if the code on line 190 is expected to be unreacheable, consider using a require statement instead of assert.

[03] Missing zero address checks for initializer and setter functions

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.

constructor(address _poolDeployer, address _vaultAddress) {

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

function setOwner(address _owner) external override onlyOwner {

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

function setFarmingAddress(address _farmingAddress) external override onlyOwner {

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

function setVaultAddress(address _vaultAddress) external override onlyOwner {

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

function setIncentive(address virtualPoolAddress) external override {

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

[04] Avoid shadowing

The arguments liquidity and volumePerLiquidityInBlock in the function _writeTimepoint() in the contract AlgebraPool.sol are showdowed by state variables with the same name from the contract PoolState.sol. Considering renaming these function arguments to avoid shadowing.

function _writeTimepoint(
  uint16 timepointIndex,
  uint32 blockTimestamp,
  int24 tick,
  uint128 liquidity,
  uint128 volumePerLiquidityInBlock
) private returns (uint16 newTimepointIndex) {
  return IDataStorageOperator(dataStorageOperator).write(timepointIndex, blockTimestamp, tick, liquidity, volumePerLiquidityInBlock); 
}

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

[05] Inconsistent use of named return variables

Some functions return named variables, others return explicit values.

Following function return explicit value

function balanceToken0() private view returns (uint256) {

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

Following function return named variables

function collect(
  address recipient,
  int24 bottomTick,
  int24 topTick,
  uint128 amount0Requested,
  uint128 amount1Requested
) external override lock returns (uint128 amount0, uint128 amount1) {

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

Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.

[06] Add a limit for the maximum number of characters per line

The solidity documentation recommends a maximum of 120 characters.

Consider adding a limit of 120 characters or less to prevent large lines.

(uint128 currentLiquidity, uint32 lastLiquidityAddTimestamp) = (_position.liquidity, _position.lastLiquidityAddTimestamp);

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

(, int256 amount0Int, int256 amount1Int) = _updatePositionTicksAndFees(recipient, bottomTick, topTick, int256(liquidityActual).toInt128()); 

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

[07] Order of funtions

Consider modifying the order of functions in AlgebraPool.sol. The solidty documentation recommends this order:

constructor
receive function (if exists)
fallback function (if exists)
external
public
internal
private

The snippet bellow shows private above external

function balanceToken0() private view returns (uint256) {
  return IERC20Minimal(token0).balanceOf(address(this));
}

function balanceToken1() private view returns (uint256) {
  return IERC20Minimal(token1).balanceOf(address(this));
}

/// @inheritdoc IAlgebraPoolState
function timepoints(uint256 index)
  external
  view
  override
  returns (
    bool initialized,
    uint32 blockTimestamp,
    int56 tickCumulative,
    uint160 secondsPerLiquidityCumulative,
    uint88 volatilityCumulative,
    int24 averageTick,
    uint144 volumePerLiquidityCumulative
  )
{
  return IDataStorageOperator(dataStorageOperator).timepoints(index);
}

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

[08] Remove unecessary curly braces wrapped around statements or document why it was used

{
  (int256 amount0Int, int256 amount1Int, ) = _getAmountsForLiquidity(
    bottomTick,
    topTick,
    int256(liquidityDesired).toInt128(),
    globalState.tick,
    globalState.price
  );

  amount0 = uint256(amount0Int);
  amount1 = uint256(amount1Int);
}

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

{ 
  if (amount0 > 0) receivedAmount0 = balanceToken0();
  if (amount1 > 0) receivedAmount1 = balanceToken1();
  IAlgebraMintCallback(msg.sender).algebraMintCallback(amount0, amount1, data);
  if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM');
  if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM');
}

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

[09] Use SafeCast consistently

Some downcasting oprations are using the SafeCast library contract

Following examples are using SafeCast.

(, int256 amount0Int, int256 amount1Int) = _updatePositionTicksAndFees(recipient, bottomTick, topTick, int256(liquidityActual).toInt128()); 

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

-int256(amount).toInt128()

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

Following examples are not using SafeCast.

fees0 = uint128(FullMath.mulDiv(innerFeeGrowth0Token - _innerFeeGrowth0Token, currentLiquidity, Constants.Q128)); 

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

liquidityActual = uint128(FullMath.mulDiv(uint256(liquidityActual), receivedAmount0, amount0));

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

Not using SafeCast for all downcasting operations can cause silent overflows. Normalizing SafeCast if preferred.

[10] Update the solidity version

All the contracts in scope are using 0.7.6.

Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath (Manual downcasting still needs overflow checks even with 0.8).

Use a solidity version of at least 0.8.2 to get compiler automatic inlining.

Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads.

Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.

[11] Missing documentation/NATSPEC

Consider document all functions to improve documentation.

function getNewPrice(

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

[12] Replace magic numbers with constants to improve code readability

res += (xLowestDegree * g) / 5040 + (xLowestDegree * x) / (40320);

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

[G-01] Improvements on for loop

The following changes would optimize gas for the loop highlighted bellow.

1. Avoid initializing the index with the default value of zero
2. Cache the length of the array outside the loop
3. The increment step can be made uncheck
4. Prefix increment costs less than postfix increment

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

[G-02] Use != 0 instead of > 0 to save gas.

Replace > 0 with != 0 for unsigned integers.

There are 30 instances of this issue.

File: src/core/contracts/AlgebraPool.sol
224: require(currentLiquidity > 0, 'NP'); // Do not recalculate the empty ranges
228: if (_liquidityCooldown > 0) {
237: liquidityNext > 0 ? (liquidityDelta > 0 ? _blockTimestamp() : lastLiquidityAddTimestamp) : 0
434: require(liquidityDesired > 0, 'IL');
451: if (amount0 > 0) receivedAmount0 = balanceToken0();
452: if (amount1 > 0) receivedAmount1 = balanceToken1();
454: if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM');
455: if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM');
469: require(liquidityActual > 0, 'IIL2');
505: if (amount0 > 0) TransferHelper.safeTransfer(token0, recipient, amount0);
506: if (amount1 > 0) TransferHelper.safeTransfer(token1, recipient, amount1);
617: if (communityFee > 0) {
641: require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA');
645: require((amountRequired = int256(balanceToken1().sub(balance1Before))) > 0, 'IIA');
667: if (communityFee > 0) {
734: (cache.amountRequiredInitial, cache.exactInput) = (amountRequired, amountRequired > 0);
808: if (cache.communityFee > 0) {
814: if (currentLiquidity > 0) cache.totalFeeGrowth += FullMath.mulDiv(step.feeAmount, Constants.Q128, currentLiquidity);
898: require(_liquidity > 0, 'L');
904: if (amount0 > 0) {
911: if (amount1 > 0) {
924: if (paid0 > 0) {
927: if (_communityFeeToken0 > 0) {
938: if (paid1 > 0) {
941: if (_communityFeeToken1 > 0) {

File: src/core/contracts/DataStorageOperator.sol
138: if (volume >= 2**192) volumeShifted = (type(uint256).max) / (liquidity > 0 ? liquidity : 1);
139: else volumeShifted = (volume << 64) / (liquidity > 0 ? liquidity : 1);

File: src/core/contracts/libraries/DataStorage.sol
80: last.secondsPerLiquidityCumulative += ((uint160(delta) << 128) / (liquidity > 0 ? liquidity : 1)); // just timedelta if liquidity == 0

File: src/core/contracts/libraries/PriceMovementMath.sol
52: require(price > 0);
53: require(liquidity > 0);

[G-03] Use custom errors rather than require/revert strings to save gas

Note that the project would need to update to solidity v0.8.4 to be able to use custom errors.

There are 34 instances of this issue.

File: src/core/contracts/AlgebraFactory.sol
109: require(uint256(alpha1) + uint256(alpha2) + uint256(baseFee) <= type(uint16).max, 'Max fee exceeded');
110: require(gamma1 != 0 && gamma2 != 0 && volumeGamma != 0, 'Gammas must be > 0');

File: src/core/contracts/AlgebraPool.sol
60: require(topTick < TickMath.MAX_TICK + 1, 'TUM');
61: require(topTick > bottomTick, 'TLU');
62: require(bottomTick > TickMath.MIN_TICK - 1, 'TLM');
194: require(globalState.price == 0, 'AI');
224: require(currentLiquidity > 0, 'NP'); // Do not recalculate the empty ranges
434: require(liquidityDesired > 0, 'IL');
454: if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM');
455: if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM');
469: require(liquidityActual > 0, 'IIL2');
474: require((amount0 = uint256(amount0Int)) <= receivedAmount0, 'IIAM2');
475: require((amount1 = uint256(amount1Int)) <= receivedAmount1, 'IIAM2');
608: require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
614: require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
636: require(globalState.unlocked, 'LOK');
641: require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA');
645: require((amountRequired = int256(balanceToken1().sub(balance1Before))) > 0, 'IIA');
731: require(unlocked, 'LOK');
733: require(amountRequired != 0, 'AS');
739: require(limitSqrtPrice < currentPrice && limitSqrtPrice > TickMath.MIN_SQRT_RATIO, 'SPL');
743: require(limitSqrtPrice > currentPrice && limitSqrtPrice < TickMath.MAX_SQRT_RATIO, 'SPL');
898: require(_liquidity > 0, 'L');
921: require(balance0Before.add(fee0) <= paid0, 'F0');
935: require(balance1Before.add(fee1) <= paid1, 'F1');

File: src/core/contracts/DataStorageOperator.sol
27: require(msg.sender == pool, 'only pool can call this');
45: require(uint256(_feeConfig.alpha1) + uint256(_feeConfig.alpha2) + uint256(_feeConfig.baseFee) <= type(uint16).max, 'Max fee exceeded');
46: require(_feeConfig.gamma1 != 0 && _feeConfig.gamma2 != 0 && _feeConfig.volumeGamma != 0, 'Gammas must be > 0');

File: src/core/contracts/base/PoolState.sol
41: require(globalState.unlocked, 'LOK');

File: src/core/contracts/libraries/DataStorage.sol
238: require(lteConsideringOverflow(self[oldestIndex].blockTimestamp, target, time), 'OLD');

File: src/core/contracts/libraries/PriceMovementMath.sol
70: require((product = amount * price) / amount == price); // if the product overflows, we know the denominator underflows
71: require(liquidityShifted > product); // in addition, we must check that the denominator does not underflow

File: src/core/contracts/libraries/TickManager.sol
96: require(liquidityTotalAfter < Constants.MAX_LIQUIDITY_PER_TICK + 1, 'LO');

File: src/core/contracts/libraries/TickTable.sol
15: require(tick % Constants.TICK_SPACING == 0, 'tick is not spaced'); // ensure that the tick is spaced

[G-04] Use right/left shift instead of division/multiplication to save gas

There are 2 instances of this issue.

File: src/core/contracts/libraries/AdaptiveFee.sol
88: res += (xLowestDegree * gHighestDegree) / 2;
96: res += (xLowestDegree * gHighestDegree) / 24;

[G-05] Using private rather than public for constants, saves gas

The values can still be inspected on the source code if necessary

There's 1 instance of this issue.

File: src/core/contracts/libraries/DataStorage.sol
12: uint32 public constant WINDOW = 1 days;

[G-06] x += y costs more gas than x = x + y for state variables

There are 2 instances of this issue.

File: src/core/contracts/AlgebraPool.sol
257: _position.fees0 += fees0;
258: _position.fees1 += fees1;