code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The initialize function in AlgebraPool.sol#L193-L206 is a very important function and sets the liquidity price at the beginning of the pool.

Performs some checks (For example, if the price is not 0)

However it is unprotected against running from the front, and a bot listening to Mempool will run from the front and cause its pool to start at too high or too low a price.

It is very important that this function is not enabled for FrontRunning operation.

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

1- Alice starts a new pool in Algebra and triggers the price determination transaction with initialize 2 - Bob listens to the mempool with the following code, which is a minimal example, and sees at what price the initialize function is triggered with the initialPrice argument, and starts the pool at the price he wants by pre-executing it and makes Alice deposit the wrong amount at the wrong price.

mempool.js
 var customWsProvider = new ethers.providers.WebSocketProvider(url);
 customWsProvider.on("pending", (tx) => { 
 let pendingTx = await connect.provider.getTransaction(txHash);
        if (pendingTx) {
            // filter pendingTx.data
            if (pendingTx.data.indexOf("0xf637731d") !== -1) {      //  func. signature : f637731d  =>  initialize(uint160) 
               console.log(pendingTx);
            }
        }

Tools Used

Manual code review

Recommended Mitigation Steps

Add a modifier that ensures that only the authorized person, that is, the first liquidator to the pool, initiates the initialize function

Or, divide the process of determining the price of the pool into parts, first print the price amounts to the state variable, and then make the initialize run before this price can be changed.

0xean commented 2 years ago

Downgrading to Medium. Alice shouldn't be relying on this initial price to determine the "fair" market price. When there is very limited liquidity the price is extremely easy to move along the x*y=k curve in any case so even after the initialized call is made someone could manipulate the pools pricing very easily when liquidity is low.

sameepsi commented 2 years ago

I don't think it's a bug. Even if someone sets the wrong price initially then it will be arbitraged. That's how AMMs work by design.