`latestSample` can be manipulation to make `stabilize()` calls always fail

[code423n4]

Lines of code

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

Vulnerability details

Impact

In function stabilize(), in case exchangeRate > priceTarget, which means price of Malt is larger than $1 so it has to sell Malt to return to $1. However, team don't want to sell Malt when the market is too volatile (price changing too fast), so they added some validations to make sure livePrice is within the bounds before selling.

if (exchangeRate > priceTarget) {
  if (
    !hasRole(ADMIN_ROLE, sender) &&
    !hasRole(INTERNAL_WHITELIST_ROLE, sender)
  ) {
    uint256 upperBand = exchangeRate +
      ((exchangeRate * upperBandLimitBps) / 10000);
    uint256 latestSample = maltDataLab.maltPriceAverage(0);
    uint256 minThreshold = latestSample -
      (((latestSample - priceTarget) * sampleSlippageBps) / 10000); 
    // @audit latestSample can smaller than priceTarget

    require(livePrice < upperBand, "Stabilize: Beyond upper bound");
    require(livePrice > minThreshold, "Stabilize: Slippage threshold");
  }

  _distributeSupply(livePrice, priceTarget, stabilizeToPeg);

However, when calculating minThreshold, it used latestSample - priceTarget. Here, only exchangeRate is validated to be larger than priceTarget while latestSample is not which can be overflow and make the function reverted.

Proof of Concept

In the code, latestSample = maltDataLab.maltPriceAverage(0), which is actually really easy to be manipulated since it is 0 seconds TWAP - current price.

Also, since attacker only has to manipulate price just enough to be slightly smaller than priceTarget, so he did not need too much funds to DOS function stabilize().

Because when function stabilize() is DOS, it means price of Malt is wrong but cannot be fixed so the impact is serious for a stable coin protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing the formula to calculate minThreshold in the ways that did not have to do the subtraction latestSample - priceTarget.

[Picodes]

maltPriceAverage(0) is not that easy to manipulate as it is not automatically updated, and still takes an average on sampleLength

[Picodes]

So the tricky case would be when exchangeRate > priceTarget but latestSample < priceTarget. It indeed could overflow in this case, but:

• it cannot be that easily manipulated

• exchangeRate and latestSample would be of opposite signs which is a sign of price volatility

  In the end, I think this is of Medium severity as it points out what seems to be an unintended but non-critical DOS

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[0xScotch]

This is duplicated by #36

[c4-judge]

Picodes marked the issue as duplicate of #36

[c4-judge]

Picodes marked the issue as satisfactory