code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/StabilizerNode.sol#L161-L176

Vulnerability details

Impact

In StabilizerNode.stabilize, globalIC.collateralRatio() is used to calculate SwingTraderEntryPrice and ActualPriceTarget, with collateralRatio indicating the ratio of the current global collateral to the malt supply.

  function collateralRatio() public view returns (uint256) {
    uint256 decimals = malt.decimals();
    uint256 totalSupply = malt.totalSupply();
    if (totalSupply == 0) {
      return 0;
    }
    return (collateral.total * (10**decimals)) / totalSupply;
  }

Global collateral includes the balance of collateral tokens in the overflowPool

  function getCollateralizedMalt() public view returns (PoolCollateral memory) {
    uint256 target = maltDataLab.priceTarget(); // 是否选用  getActualPriceTarget()

    uint256 unity = 10**collateralToken.decimals();

    // Convert all balances to be denominated in units of Malt target price
    uint256 overflowBalance = maltDataLab.rewardToMaltDecimals((collateralToken.balanceOf(
      address(overflowPool)
    ) * unity) / target);
    uint256 liquidityExtensionBalance = (collateralToken.balanceOf(
      address(liquidityExtension)
    ) * unity) / target;
    (
      uint256 swingTraderMaltBalance,
      uint256 swingTraderBalance
    ) = swingTraderManager.getTokenBalances();
    swingTraderBalance = (swingTraderBalance * unity) / target;

    return
      PoolCollateral({
        lpPool: address(stakeToken),
        // Note that swingTraderBalance also includes the overflowBalance
        // Therefore the total doesn't need to include overflowBalance explicitly
        total: maltDataLab.rewardToMaltDecimals(
            liquidityExtensionBalance + swingTraderBalance
        ),

In StabilizerNode.stabilize, since the undistributed rewards in the overflowPool are not distributed, this can cause the actual collateral radio to be large and thus affect the stabilize process. A simple example is:

impliedCollateralService.syncGlobalCollateral() is called to synchronize the latest data.
there are some gap epochs in RewardThrottle and their rewards are not distributed from the overflowPool.
When StabilizerNode.stabilize is called, it treats the undistributed rewards in the overflowPool as collateral, thus making globalIC.collateralRatio() large, and the results of maltDataLab. getActualPriceTarget/getSwingTraderEntryPrice will be incorrect, thus making stabilize incorrect.

Since stabilize is a core function of the protocol, stabilizing with the wrong data is likely to cause malt to be depegged, so the vulnerability should be high-risk.

Proof of Concept

https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/StabilizerNode.sol#L161-L176

Tools Used

None

Recommended Mitigation Steps

Call RewardThrottle.checkRewardUnderflow at the beginning of StabilizerNode.stabilize to distribute the rewards in the overflowPool, then call impliedCollateralService. syncGlobalCollateral() to synchronize the latest data

  function stabilize() external nonReentrant onlyEOA onlyActive whenNotPaused {
    // Ensure data consistency
    maltDataLab.trackPool();

    // Finalize auction if possible before potentially starting a new one
    auction.checkAuctionFinalization();

+  RewardThrottle.checkRewardUnderflow();
+  impliedCollateralService.syncGlobalCollateral();

    require(
      block.timestamp >= stabilizeWindowEnd || _stabilityWindowOverride(),
      "Can't call stabilize"
    );

0xScotch commented 1 year ago

By a strict implementation of the protocol this is a bug as it would result in global collateral being slightly misreported and therefore downstream decisions being made on incorrect data. However, in practice the chances of a big gap in epochs is very low due to the incentivization to upkeep that as well as the degree to which the global IC would be incorrect would be very small. It seems very unlikely this bug would ever lead to a depeg as stated.

Let's say 50% of the Malt float is in staked LP and the current APR is 10%. We go for 48 epochs (24 hours) without any call to checkRewardUnderflow. This means the global IC will be misreported by 24 hours of APR (10%).

The current APR is 10% and 50% of float is staked, therefore the yearly rewards represent 5% of the total float. One day worth of that is 5% / 365 = 0.013%.

Therefore we can say that under the above stated circumstances the global IC would be misquoted by 0.02%. Seems very unlikely that discrepancy would be the cause of a depeg.