code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/main/contracts/RewardSystem/RewardThrottle.sol#L154-L170

Vulnerability details

Impact

Cap is not applied when runwayDeficit = 0, so targetAPR can be an invalid value.

Proof of Concept

In RewardThrottle.updateDesiredAPR, targetAPR will be decreased when cashflowAverageApr < targetCashflowApr

    } else if (cashflowAverageApr < targetCashflowApr) {
      uint256 deficit = runwayDeficit();

      if (deficit == 0) {
        aprLastUpdated = block.timestamp;
        return;
      }

      uint256 delta = targetCashflowApr - cashflowAverageApr;
      uint256 adjustment = (delta * proportionalGainBps) / 10000;

      if (adjustment > adjustmentCap) {
        adjustment = adjustmentCap;
      }

      newAPR -= adjustment;
    }

When overflow balance is enough to cover current targetAPR, there is no deficit and runwayDeficit returns 0 for deficit. When deficit == 0, targetAPR will not be updated so updateDesiredAPR only updates apr updated time and ends with return. But aprCap and aprFloor can be updated by the admin after the last update of targetAPR. So targetAPR should be capped by aprCap and aprFloor though it is not changed at all when deficit == 0. There is another early return logic in updateDesiredAPR before apr update period, but it's correct to skip capping because it does nothing in this case.

Tools Used

Manual Review

Recommended Mitigation Steps

Apply aprCap and aprFloor to targetAPR even though it is not changed at all when deficit == 0.

c4-sponsor commented 1 year ago

0xScotch marked the issue as sponsor disputed

0xScotch commented 1 year ago

This is the desired behaviour. If we have enough capital to sustain pay the current APR for the desired runway then there is no need to apply any cap. The cap is there to avoid runaway situations that drain capital due to inflated APR. That is not a problem if we already have a full runway

Picodes commented 1 year ago

It is correct though that if aprFloor is increased above targetAPR, it won't be applied if deficit == 0. Downgrading to low as it seems that this only impacts when the new admin settings will be applied.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)