Incorrect reserve updates for when `targetPrice` is exactly in the middle of high and low price

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/functions/Stable2.sol#L198-L211 https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/functions/Stable2.sol#L265-L279

Vulnerability details

Impact

The returned reserve will be incorrectly calculated in the calcReserveAtRatioSwap and calcReserveAtRatioLiquidity functions due to pd.currentPrice incorrectly set as high price when targetPrice lies directly in the middle of highPrice and Lowprice.

Proof of Concept

1. Context

We look first at calcReserveAtRatioLiquidity function. The pd.CurrentPrice parameter is updated depending on if the pd.targetPrice is closer to pd.lutData.highPrice or pd.lutData.lowPrice . This is enforced by comparing the differences between them and the target price before settling pd.currentPrice to low/high price. This is also corroborated by the comments.

        // update scaledReserve[j] such that calcRate(scaledReserves, i, j) = low/high Price,
        // depending on which is closer to targetPrice.
        if (pd.lutData.highPrice - pd.targetPrice > pd.targetPrice - pd.lutData.lowPrice) {
            // targetPrice is closer to lowPrice.
            scaledReserves[j] = scaledReserves[i] * pd.lutData.lowPriceJ / pd.lutData.precision;

            // set current price to lowPrice.
            pd.currentPrice = pd.lutData.lowPrice;
        } else {
            // targetPrice is closer to highPrice.
            scaledReserves[j] = scaledReserves[i] * pd.lutData.highPriceJ / pd.lutData.precision;

            // set current price to highPrice.
            pd.currentPrice = pd.lutData.highPrice;
        }

This can also be observed in the calcReserveAtRatioSwap function.

        // update `scaledReserves` based on whether targetPrice is closer to low or high price:
        if (pd.lutData.highPrice - pd.targetPrice > pd.targetPrice - pd.lutData.lowPrice) {
            // targetPrice is closer to lowPrice.
            scaledReserves[i] = parityReserve * pd.lutData.lowPriceI / pd.lutData.precision;
            scaledReserves[j] = parityReserve * pd.lutData.lowPriceJ / pd.lutData.precision;
            // initialize currentPrice:
            pd.currentPrice = pd.lutData.lowPrice;
        } else {
            // targetPrice is closer to highPrice.
            scaledReserves[i] = parityReserve * pd.lutData.highPriceI / pd.lutData.precision;
            scaledReserves[j] = parityReserve * pd.lutData.highPriceJ / pd.lutData.precision;
            // initialize currentPrice:
            pd.currentPrice = pd.lutData.highPrice;
        }

2. Bug Location

The functions however ignore an edgecase in which the value of pd.lutData.highPrice - pd.targetPrice is exactly equal to pd.targetPrice - pd.lutData.lowPrice, meaning there's no price deviations, and as such current price should remain the same as its target. This is because the function directly uses the else block, which handles the <= aspects of the operation.

3. Final Effect

This means that when pd.lutData.highPrice - pd.targetPrice == pd.targetPrice - pd.lutData.lowPrice, the price difference on both sides, hence, not closer to any, the currentPrice which should be set to the targetPrice, i.e the desired price is wrongly shifted higher. This incorrect price is then used to calculate the scaledReserves potentially returning incorrect values for the reserves in both function.

Tools Used

Manual code review

Recommended Mitigation Steps

Recommend handling this edgecase and setting pd.currentPrice to targetPrice instead, when there are no deviations in its proximity to the high or low price.

Assessed type

Context

[Brean0]

The Basin development community believes that in the case where pd.lutData.highPrice - pd.targetPrice == pd.targetPrice - pd.lutData.lowPrice, the function works as intended. The function is designed to work regardless of whether the initial guess is at the low or high point (i.e irrespective of distance), but ideally chooses the price closer to the target price. Thus, we do not think anything critical is introduced here. We would appreciate a POC if this isn't correct.

[alex-ppg]

The Warden outlines that a targetPrice being equidistant from the highPrice and lowPrice will not behave as expected in the reserve calculations at a particular ratio. As the Sponsor outlines, the if-else code block is meant to simply choose the closest price point to the targetPrice, and defaulting the equality case to be closer to the higher price is a preference that may be suboptimal but should not result in a security vulnerability.

I invite the Warden to supply a PoC demonstrating a material flaw that would arise from this defaulting behavior, and until then consider the submission to be invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid