QuickSwap and StellaSwap contest - berndartmueller's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/libraries/TransferHelper.sol#L21

Vulnerability details

The helper function TransferHelper.safeTransfer performs an ERC-20 token transfer with a low-level call without confirming the contract’s existence prior.

The Solidity documentation includes the following warning:

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Impact

A pool may assume that failed transactions involving destructed tokens are successful when in fact the token transfer silently failed. No liquidity can be added if the token contract has not yet been deployed yet. However, if the token has been destroyed (after deploying the pool), the pool will act as if the assets were sent even though they were not.

Proof of Concept

libraries/TransferHelper.sol#L21

library TransferHelper {
  /// @notice Transfers tokens from msg.sender to a recipient
  /// @dev Calls transfer on token contract, errors with TF if transfer fails
  /// @param token The contract address of the token which will be transferred
  /// @param to The recipient of the transfer
  /// @param value The value of the transfer
  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');
  }
}

Exploit Scenario:

The pool contains tokens A and B. Token A has a bug, and the contract is destroyed. Bob is unaware of the issue and swaps 1,000 B tokens for A tokens. Bob successfully transfers 1,000 B tokens to the pool but receives no A tokens in return. As a result, Bob loses 1,000 B tokens.

Tools Used

Manual review

Recommended mitigation steps

Consider checking the contract’s existence before the low-level call in TransferHelper.safeTransfer, similar to what is done in OpenZeppelin's Address.functionCall. This will ensure that a swap reverts if the token to be bought no longer exists, preventing the pool from accepting the token to be sold without returning any tokens in exchange.

Disclaimer: This very same issue has been found in the Trail of Bits audit of Uniswap V3 (TOB-UNI-009)

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraFactory.sol#L91-L95 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L918 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L546-L549

Vulnerability details

Collected community fees from each swap and flash loan are immediately sent to the defined AlgebraFactor.vaultAddress address. Contrary to the used pull pattern in Uniswap V3.

Having fees (ERC-20 tokens) immediately sent to the vault within each swap and flash loan, opens up potential issues if the vault address is set to the zero address.

The following AlgebraPool functions are affected:

• swap,
• swapSupportingFeeOnInputTokens, and
• flash

Impact

If the AlgebraFactor.vaultAddress address is set to address(0), all Algebra pools deployed by this factory contract will have their swap and flash loan functionality affected in a either completely broken way (transactions will revert due to ERC-20 token transfers to the zero address) or fees are sent (effectively burned) to the zero address. In the end, it will depend on the ERC-20 token implementation if it reverts or simply burns the fees.

Proof of Concept

AlgebraFactory.setVaultAddress

AlgebraFactory.setVaultAddress is used by the owner of the AlgebraFactory to set the vault address.

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

AlgebraPool.sol#L918

function flash(
  address recipient,
  uint256 amount0,
  uint256 amount1,
  bytes calldata data
) external override lock {
  [...]

  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); // @audit-info `vault` is used as the recipient of community fees
    }
    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); // @audit-info `vault` is used as the recipient of community fees
    }
    totalFeeGrowth1Token += FullMath.mulDiv(paid1 - fees1, Constants.Q128, _liquidity);
  }

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

AlgebraPool._payCommunityFee

This function is called from within the AlgebraPool.swap and AlgebraPool.swapSupportingFeeOnInputTokens functions.

function _payCommunityFee(address token, uint256 amount) private {
  address vault = IAlgebraFactory(factory).vaultAddress();
  TransferHelper.safeTransfer(token, vault, amount);
}

Tools Used

Manual review

Recommended mitigation steps

Either

• consider adding a check if vault == address(0) in AlgebraPool._payCommunityFee and in AlgebraPool.flash,
• prevent setting vault to address(0) in AlgebraFactory.setVaultAddress, or
• use a pull pattern to collect community (protocol) fees

Lines of code

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

Vulnerability details

The AlgebraPool.initialize function used to initialize a newly deployed Algebra pool can be front-run and called by anyone.

The attacker can initialize the pool before the legitimate pool deployer and set an arbitrary initial price (initialPrice). This allows an attacker to generate profits from the initial liquidity provider’s deposits.

Impact

Anyone can front-run pool initialization and set an arbitrary initial price, allowing them to generate profits from the initial liquidity provider’s deposits.

Proof of Concept

Exploit Scenario:

1. Bob deploys a pool for assets A and B. The current market price is 1 A == 1 B
2. Eve front-runs Bob’s transaction to the initialize function and sets a price such that 1 A ~= 10 B
3. Bob calls mint and deposits assets A and B worth $100,000, sending ~10 times more of asset B than asset A
4. Eve swaps A tokens for B tokens at an unfair price, profiting off of Bob’s deployment

AlgebraPool.initialize

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

Tools Used

Manual review

Recommended mitigation steps

Either

• consider adding access control to the AlgebraPool.initialize function,
• moving the price operations from initialize to the constructor and having the pool deployer provide the initial price via the factory, or
• ensuring that the documentation clearly warns users about incorrect initialization

Disclaimer: This very same issue has been found in the Trail of Bits audit of Uniswap V3 (TOB-UNI-007)