lastSetMintExchangeRate is not updated

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/CashManager.sol#L305

Vulnerability details

Impact

The lastSetMintExchangeRate variable is not updated if exchange rate deviates the max amount. This becomes a problem since once Admin unpauses the system and new Exchange rate are being set, lastSetMintExchangeRate still points to exchange rate from last to last epoch instead of last epoch. This would highly cause system to again go in pause state

Proof of Concept

1. Admin sets the new exchange rate for epoch 1 using the setMintExchangeRate function

2. Lets say below is variable conditions

epochToSet=1
currentEpoch=2
lastSetMintExchangeRate=100
exchangeRate=500

3. This makes below calculations

...
uint256 rateDifference;
    if (exchangeRate > lastSetMintExchangeRate) {
      rateDifference = exchangeRate - lastSetMintExchangeRate;
    } else if (exchangeRate < lastSetMintExchangeRate) {
      rateDifference = lastSetMintExchangeRate - exchangeRate;
    }

    uint256 maxDifferenceThisEpoch = (lastSetMintExchangeRate *
      exchangeRateDeltaLimit) / BPS_DENOMINATOR;

    if (rateDifference > maxDifferenceThisEpoch) {
      epochToExchangeRate[epochToSet] = exchangeRate;
      _pause();
      emit MintExchangeRateCheckFailed(
        epochToSet,
        lastSetMintExchangeRate,
        exchangeRate
      );
    }
...

4. The rateDifference becomes 500-100=400. Lets say maxDifferenceThisEpoch was 300

5. Since rateDifference > maxDifferenceThisEpoch is true, below if condition executes and system is kept in pause state

if (rateDifference > maxDifferenceThisEpoch) {
      epochToExchangeRate[epochToSet] = exchangeRate;
      _pause();
      emit MintExchangeRateCheckFailed(
        epochToSet,
        lastSetMintExchangeRate,
        exchangeRate
      );
    }

6. As we can see epochToExchangeRate[1] gets updated to 500 but lastSetMintExchangeRate is still 100

7. Lets say Admin unpause system.

8. This becomes a problem with next epoch.

9. Once Admin sets the exchange rate say 700 for epoch 3, the rate difference exchangeRate - lastSetMintExchangeRate; will use lastSetMintExchangeRate as 100 instead of 500, causing the rateDifference to become 600 instead of 200.

10. Ideally this should not have failed mint check but due to lack of upgrade this causes mint check to fail

Recommended Mitigation Steps

Update the if condition to update lastSetMintExchangeRate

if (rateDifference > maxDifferenceThisEpoch) {
      epochToExchangeRate[epochToSet] = exchangeRate;
lastSetMintExchangeRate = exchangeRate;
      _pause();
      emit MintExchangeRateCheckFailed(
        epochToSet,
        lastSetMintExchangeRate,
        exchangeRate
      );
    }

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as satisfactory

[tom2o17]

We have overrideExchangeRate to handle cases such as this. Under no circumstances will there be a 500% move in the underlying price of the asset that is being tracked w/n an epoch. If the pause is triggered the admin is assumed to correct the error from the SETTER_ADMIN through a call to overrideExchangeRate

Link to referenced code here

[c4-sponsor]

ali2251 marked the issue as sponsor disputed

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid