Treasury rebalance will fail due to interest accrual

Summary

If Compound has updated their interest rate model, then Notional will calculate the before total underlying token balance without accruing interest. If this exceeds Constants.REBALANCING_UNDERLYING_DELTA, then rebalance execution will revert.

Vulnerability Detail

The TreasuryAction._executeRebalance() function will revert on a specific edge case where oracle.getTotalUnderlyingValueStateful() does not accrue interest before calculating the value of the treasury's cToken holdings.

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/external/actions/TreasuryAction.sol#L284-L302

File: TreasuryAction.sol
284:     function _executeRebalance(uint16 currencyId) private {
285:         IPrimeCashHoldingsOracle oracle = PrimeCashExchangeRate.getPrimeCashHoldingsOracle(currencyId);
286:         uint8[] memory rebalancingTargets = _getRebalancingTargets(currencyId, oracle.holdings());
287:         (RebalancingData memory data) = REBALANCING_STRATEGY.calculateRebalance(oracle, rebalancingTargets);
288: 
289:         (/* */, uint256 totalUnderlyingValueBefore) = oracle.getTotalUnderlyingValueStateful();
290: 
291:         // Process redemptions first
292:         Token memory underlyingToken = TokenHandler.getUnderlyingToken(currencyId);
293:         TokenHandler.executeMoneyMarketRedemptions(underlyingToken, data.redeemData);
294: 
295:         // Process deposits
296:         _executeDeposits(underlyingToken, data.depositData);
297: 
298:         (/* */, uint256 totalUnderlyingValueAfter) = oracle.getTotalUnderlyingValueStateful();
299: 
300:         int256 underlyingDelta = totalUnderlyingValueBefore.toInt().sub(totalUnderlyingValueAfter.toInt());
301:         require(underlyingDelta.abs() < Constants.REBALANCING_UNDERLYING_DELTA);
302:     }

cTokenAggregator.getExchangeRateView() returns the exchange rate which is used to calculate the underlying value of cToken holdings in two ways:

If the interest rate model is unchanged, then we correctly accrue interest by calculating it without mutating state.
If the interest rate model HAS changed, then we query cToken.exchangeRateStored() which DOES NOT accrue interest.

File: cTokenAggregator.sol
092:     function getExchangeRateView() external view override returns (int256) {
093:         // Return stored exchange rate if interest rate model is updated.
094:         // This prevents the function from returning incorrect exchange rates
095:         uint256 exchangeRate = cToken.interestRateModel() == INTEREST_RATE_MODEL
096:             ? _viewExchangeRate()
097:             : cToken.exchangeRateStored();
098:         _checkExchangeRate(exchangeRate);
099: 
100:         return int256(exchangeRate);
101:     }

Therefore, if the interest rate model has changed, totalUnderlyingValueBefore will not include any accrued interest and totalUnderlyingValueAfter will include all accrued interest. As a result, it is likely that the delta between these two amounts will exceed Constants.REBALANCING_UNDERLYING_DELTA, causing the rebalance to ultimately revert.

It does not really make sense to not accrue interest if the interest rate model has changed unless we want to avoid any drastic changes to Notional's underlying protocol. Then we may want to explicitly revert here instead of allowing the rebalance function to still execute.

Impact

The treasury manager is unable to rebalance currencies across protocols and therefore it is likely that most funds become under-utilised as a result.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/external/actions/TreasuryAction.sol#L284-L302

Tool used

Manual Review

Recommendation

Ensure this is well-understand and consider accruing interest under any circumstance. Alternatively, if we do not wish to accrue interest when the interest rate model has changed, then we need to make sure that underlyingDelta does not include this amount as TreasuryAction._executeDeposits() will ultimately update the vault's position in Compound.

jeffywu commented 1 year ago

Valid issue, although I would disagree with the severity since interest rate models are unlikely to change and we have already deprecated Compound V2 support.

0xleastwood commented 1 year ago

Escalate for 10 USDC

While Compound V2 is intended to be deprecated, a substantial portion of Notional's codebase relied on extensively on this at contest time. Looking at severity guidelines, I think this one should be included as medium severity because the scenario is considered to be viable, although unlikely.

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

While Compound V2 is intended to be deprecated, a substantial portion of Notional's codebase relied on extensively on this at contest time. Looking at severity guidelines, I think this one should be included as medium severity because the scenario is considered to be viable, although unlikely.

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Result: Medium Unique Agree with the points raised in the escalation. Considering this a valid medium

sherlock-admin commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

0xleastwood: accepted

sherlock-audit / 2023-03-notional-judging

xiaoming90 - Treasury rebalance will fail due to interest accrual #189