howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/libraries/UniHelper.sol#L35-L66

Vulnerability details

Impact

Negative ticks when fetching the price from the Uniswap pool are not rounded towards 0, which will give a higher price for pools using TWAP. This will allow opening positions with less margin leading to bad debt accruing over time.

Proof of Concept

UniHelper.sol

function callUniswapObserve(IUniswapV3Pool uniswapPool, uint256 ago) internal view returns (uint160, uint256) {
    uint32[] memory secondsAgos = new uint32[](2);

    secondsAgos[0] = uint32(ago);
    secondsAgos[1] = 0;

    (bool success, bytes memory data) =
        address(uniswapPool).staticcall(abi.encodeWithSelector(IUniswapV3PoolOracle.observe.selector, secondsAgos));

    if (!success) {
        if (keccak256(data) != keccak256(abi.encodeWithSignature("Error(string)", "OLD"))) {
            revertBytes(data);
        }

        (,, uint16 index, uint16 cardinality,,,) = uniswapPool.slot0();

        (uint32 oldestAvailableAge,,, bool initialized) = uniswapPool.observations((index + 1) % cardinality);

        if (!initialized) {
            (oldestAvailableAge,,,) = uniswapPool.observations(0);
        }

        ago = block.timestamp - oldestAvailableAge;
        secondsAgos[0] = uint32(ago);

        (success, data) = address(uniswapPool).staticcall(
            abi.encodeWithSelector(IUniswapV3PoolOracle.observe.selector, secondsAgos)
        );
        if (!success) {
            revertBytes(data);
        }
    }

    int56[] memory tickCumulatives = abi.decode(data, (int56[]));

    int24 tick = int24((tickCumulatives[1] - tickCumulatives[0]) / int56(int256(ago))); //@audit this should round down on negative tick

    uint160 sqrtPriceX96 = TickMath.getSqrtRatioAtTick(tick);

    return (sqrtPriceX96, ago);
}

Here we can see that in case tick is negative, it is not being round down towards 0, that will usually give higher prices for the pools that are using TWAP. For reference here is the Uniswap implementation which uses the correct approach.

The impact of this misconfiguration will be observed in the code that uses this functionality:

PositionCalculator::calculateMinMargin
- This function is used to determine whether position is eligible for liquidation when opening new trade or in the liquidation itself.
  
  When false higher price is being returned, there will be unsafe positions that will appear safe, until they become insolvent.
PerpMarketV1::predyTradeAfterCallback
- _calculateInitialMargin which internally ends up calling the PositionCalculator::getSqrtIndexPrice which will use the flawed TWAP implementation in case no Chainlink price feed is set. When there is a negative tick that is not rounded properly _calculateInitialMargin will return smaller number because it will overvalue the position here:
PerpMarketV1.sol

      function _calculateInitialMargin(uint256 vaultId, uint256 pairId, uint256 leverage)
          internal
          view
          returns (int256)
      {
          DataType.Vault memory vault = _predyPool.getVault(vaultId);

          uint256 sqrtPrice = _predyPool.getSqrtIndexPrice(pairId);//@audit wrong price used

          uint256 price = Math.calSqrtPriceToPrice(sqrtPrice);

          uint256 netValue = _calculateNetValue(vault, price);

          return (netValue / leverage).toInt256() - _calculatePositionValue(vault, sqrtPrice);
      }

Then depending on whether marginAmountUpdate is positive or negative, 2 inconsistencies will occur:

[PerpMarketV1.sol](https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketV1.sol#L102-L142)

      function predyTradeAfterCallback(
          IPredyPool.TradeParams memory tradeParams,
          IPredyPool.TradeResult memory tradeResult
      ) external override(BaseHookCallbackUpgradable) onlyPredyPool {
      ...MORE CODE
              int256 marginAmountUpdate =
                  _calculateInitialMargin(tradeParams.vaultId, tradeParams.pairId, callbackData.leverage);

              uint256 cost = 0;

              if (marginAmountUpdate > 0) {
                  cost = uint256(marginAmountUpdate);
              }

              _verifyOrderV3(callbackData.resolvedOrder, cost);

              if (marginAmountUpdate > 0) {
                  quoteToken.safeTransfer(address(_predyPool), uint256(marginAmountUpdate));
              } else if (marginAmountUpdate < 0) {
                  _predyPool.take(true, callbackData.trader, uint256(-marginAmountUpdate));
              }
      ...MORE CODE
      }

- Positive - `PredyPool` will receive less than tokens, because they have false higher price, calculated in the `_calculateInitialMargin`
- Negative - trader will be transferred more tokens in the `GlobalData::take`:

[GlobalData.sol](https://github.com/code-423n4/2024-05-predy/blob/main/src/types/GlobalData.sol#L72-L86)

      function take(GlobalDataLibrary.GlobalData storage globalData, bool isQuoteAsset, address to, uint256 amount)
          internal
      {
          DataType.PairStatus memory pairStatus = globalData.pairs[globalData.lockData.pairId];

          address currency;

          if (isQuoteAsset) {
              currency = pairStatus.quotePool.token;
          } else {
              currency = pairStatus.basePool.token;
          }

          ERC20(currency).safeTransfer(to, amount);
      }

Here are similar issue for reference:

In overall this misconfiguration + the relatively small TWAP period will open various arbitrage and price manipulations.

Tools Used

Manual Review

Recommended Mitigation Steps

Add this line.

if (tickCumulatives[1] - tickCumulatives[0] < 0 && (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0) tick--;

Assessed type

Oracle

c4-judge commented 3 months ago

alex-ppg changed the severity to 2 (Med Risk)