code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/main/contracts/RewardSystem/RewardThrottle.sol#L225-L227 https://github.com/code-423n4/2023-02-malt/blob/main/contracts/RewardSystem/RewardThrottle.sol#L252-L254

Vulnerability details

Impact

averageCashflowAPR returns meaningless value when startEpoch = endEpoch.

Proof of Concept

There are two averageCashflowAPR functions in RewardThrottle.sol and they returns epochCashflowAPR(endEpoch) when startEpoch == endEpoch.

    if (startEpoch == endEpoch || averagePeriod == 0) {
      return epochCashflowAPR(endEpoch);
    }

In the both of implementations, apr = (endEpochState.cumulativeCashflowApr - startEpochState.cumulativeCashflowApr) / averagePeriod.

    uint256 delta = endEpochState.cumulativeCashflowApr -
      startEpochState.cumulativeCashflowApr;

    apr = delta / averagePeriod;

And state[i + 1].cumulativeCashflowApr = state[i].cumulativeCashflowApr + epochCashflowAPR(i) for any epoch i.

apr is the average of epochCashflowAPRs for (startEpoch, startEpoch + 1, ... , endEpoch - 1). When endEpoch > startEpoch, all things are good, but when endEpoch == startEpoch, there's nothing to average, so it's natural to return 0 instead of epochCashflowAPR(endEpoch).

Inside of RewardThrottle.sol, endEpoch will always be greater than startEpoch because smoothingPeriod > 0. L139

    uint256 cashflowAverageApr = averageCashflowAPR(smoothingPeriod);

L40

    uint256 public smoothingPeriod = 24; // 24 epochs = 12 hours

L698-L704

  function setSmoothingPeriod(uint256 _smoothingPeriod)
    external
    onlyRoleMalt(ADMIN_ROLE, "Must have admin privs")
  {
    require(_smoothingPeriod > 0, "No zero smoothing period");
    smoothingPeriod = _smoothingPeriod;
  }

So this only affects when it is called from outside. So the impact reduced, but it's still a problem.

Tools Used

Manual Review

Recommended Mitigation Steps

Return 0 instead of epochCashflowAPR(endEpoch) when startEpoch = endEpoch.

c4-sponsor commented 1 year ago

0xScotch marked the issue as sponsor disputed

0xScotch commented 1 year ago

I don't agree 0 is the natural thing to return in this case. You are requesting the average APR for a period of time. When startEpoch == endEpoch returning the APR for that epoch feels natural to me.

Regardless then seems like a stylistic choice more than a vulnerability. So unless there is a specific security concern for this behaviour I would say this isn't a valid finding.

Picodes commented 1 year ago

Downgrading to low in the absence of impact description.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)