code-423n4 / 2022-09-quickswap-findings

0 stars 0 forks source link

Frontrunning the initialize function can drain the LP initial deposit #311

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

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.

sameepsi commented 1 year ago

duplicate of #84