code-423n4 / 2024-05-predy-findings

10 stars 9 forks source link

`PerpMarketLib::validateStopPrice` calculates the ratio of `oraclePrice` and `stopPrice` incorrectly #273

Closed howlbot-integration[bot] closed 4 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketV1.sol#L213-L215 https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketLib.sol#L97-L115 https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketLib.sol#L147-L153 https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketLib.sol#L178-L186

Vulnerability details

Impact

After trades are executed in the Perp Market, the trade order is validated to ensure that the trade conforms to the parameters set by the user, such as market price, limit price, and stop price, etc.

When the stop price is validated, the ratio of the oracle price and stop price is calculated.

The problem is that the ratio incorrectly returns 0 when oracle price == stop price, when it should actually return 1.

This leads to a problem where a valid stop price may incorrectly be invalidated, causing DoS of trades.

Proof of Concept

After trades are completed, they are validated

PerpMarketV1.sol#L213-L215

    PerpMarketLib.validateTrade(
        tradeResult, tradeAmount, perpOrder.limitPrice, perpOrder.stopPrice, perpOrder.auctionData
    );

The stop price is validated under certain conditions

PerpMarketLib.sol#L97-L115

    } else if (limitPrice > 0 && stopPrice > 0) {
        // limit & stop order
@>      if (
            !validateLimitPrice(tradePrice, tradeAmount, limitPrice)
                && !validateStopPrice(oraclePrice, tradePrice, tradeAmount, stopPrice, auctionData)
        ) {
            revert LimitStopOrderDoesNotMatch();
        }
    } else if (limitPrice > 0) {
        // limit order
        if (!validateLimitPrice(tradePrice, tradeAmount, limitPrice)) {
            revert LimitPriceDoesNotMatch();
        }
    } else if (stopPrice > 0) {
        // stop order
@>      if (!validateStopPrice(oraclePrice, tradePrice, tradeAmount, stopPrice, auctionData)) {
            revert StopPriceDoesNotMatch();
        }
    }

validateStopPrice calls decay2 with ratio(oraclePrice, stopPrice) as the value parameter

PerpMarketLib.sol#L147-L153

    uint256 decayedSlippageTorelance = DecayLib.decay2(
        auctionParams.startPrice,
        auctionParams.endPrice,
        auctionParams.startTime,
        auctionParams.endTime,
@>      ratio(oraclePrice, stopPrice)
    );

PerpMarketLib.sol#L178-L186

    function ratio(uint256 price1, uint256 price2) internal pure returns (uint256) {
        if (price1 == price2) {
@>          return 0;
        } else if (price1 > price2) {
            return (price1 - price2) * Bps.ONE / price2;
        } else {
            return (price2 - price1) * Bps.ONE / price2;
        }
    }

Here, if price1 == price2 the ratio returned is 0, which is incorrect. Any values that are equal have a ratio of 1 (100%) (Bps.ONE in this case).

This means that the decayedSlippageTolerance value will not be correct and validate stop prices may be deemed invalid, causing DoS of trades.

Tools Used

Manual Review

Recommended Mitigation Steps

When prices are equal, the ratio should be 1 (100%)

    function ratio(uint256 price1, uint256 price2) internal pure returns (uint256) {
        if (price1 == price2) {
-           return 0;
+           return Bps.ONE;
        } else if (price1 > price2) {
            return (price1 - price2) * Bps.ONE / price2;
        } else {
            return (price2 - price1) * Bps.ONE / price2;
        }
    }

Assessed type

Math

c4-judge commented 4 months ago

alex-ppg marked the issue as unsatisfactory: Invalid