L-01. Rebalance may occur due to wrong requirements check

Submitted by SBSecurity.

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXChecks.sol#L268-L293

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXChecks.sol#L312-L332

Summary

Before a rebalance can occur, checks are implemented to ensure that delta and debtRatio remain within their specified limits. However, it's important to note that the check in GMXChecks::beforeRebalanceChecks ignores the scenario where these values are equal to any of their limits.

Vulnerability Details

In the current implementation of the GMXRebalance::rebalanceAdd function, it first calculates the current values of debtRatio and delta before making any changes. Subsequently, the beforeRebalanceChecks function, checks if these values meet the requirements for a rebalance to occur. These requirements now dictate that both debtRatio and delta must be either ≥ to the UpperLimit, or ≤ to the LowerLimit for a rebalance to take place.

javascript
function beforeRebalanceChecks(
GMXTypes.Store storage self,
GMXTypes.RebalanceType rebalanceType
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Rebalance_Open
) revert Errors.NotAllowedInCurrentVaultStatus();

// Check that rebalance type is Delta or Debt
// And then check that rebalance conditions are met
// Note that Delta rebalancing requires vault's delta strategy to be Neutral as well
if (rebalanceType == GMXTypes.RebalanceType.Delta && self.delta == GMXTypes.Delta.Neutral) {
if (
self.rebalanceCache.healthParams.deltaBefore < self.deltaUpperLimit &&
self.rebalanceCache.healthParams.deltaBefore > self.deltaLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else if (rebalanceType == GMXTypes.RebalanceType.Debt) {
if (
self.rebalanceCache.healthParams.debtRatioBefore < self.debtRatioUpperLimit &&
self.rebalanceCache.healthParams.debtRatioBefore > self.debtRatioLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else {
revert Errors.InvalidRebalanceParameters();
}
}

Suppose a rebalance is successful. In the afterRebalanceChecks section, the code verifies whether both delta and debtRatio are greater than the UpperLimit or less than the LowerLimit. This confirmation implies that these limits are indeed inclusive, meaning that the correct interpretation of these limits should be that LowerLimit ≤ actualValue ≤ UpperLimit. On the other hand, this also indicates that for a rebalancing to occur, the values of deltaBefore and debtRatioBefore need to be outside their limits, i.e., delta should be greater than Upper or less than Lower. However, in the current implementation, if these values are equal to the limit, a rebalance may still occur, which violates the consistency of the afterRebalanceChecks function, thus indicating that these limits are inclusive. Consequently, a value equal to the limit needs to be treated as valid and not be able to trigger a rebalance.

javascript
function afterRebalanceChecks(
GMXTypes.Store storage self
) external view {
// Guards: check that delta is within limits for Neutral strategy
if (self.delta == GMXTypes.Delta.Neutral) {
int256 _delta = GMXReader.delta(self);

if (
_delta > self.deltaUpperLimit ||
_delta < self.deltaLowerLimit
) revert Errors.InvalidDelta();
}

// Guards: check that debt is within limits for Long/Neutral strategy
uint256 _debtRatio = GMXReader.debtRatio(self);

if (
_debtRatio > self.debtRatioUpperLimit ||
_debtRatio < self.debtRatioLowerLimit
) revert Errors.InvalidDebtRatio();
}

Imagine the case when delta or debtRatio is equal to any of its limits; a rebalance will occur. However, on the other hand, these values are valid because they are inclusively within the limits.

Impact

In such a scenario, the system might incorrectly trigger a rebalance of the vault, even when delta or debtRatio is precisely within the established limits, thus potentially causing unintended rebalancing actions.

Tools Used

Manual

Recommendations

Consider a strict check to determine if delta or debtRatio is strictly within its limits, including scenarios where they are equal to any of its limits. In such cases, the code should ensure that a rebalance does not occur when these values are precisely at the limit.

javascript
function beforeRebalanceChecks(
GMXTypes.Store storage self,
GMXTypes.RebalanceType rebalanceType
) external view {
if (
self.status != GMXTypes.Status.Open &&
self.status != GMXTypes.Status.Rebalance_Open
) revert Errors.NotAllowedInCurrentVaultStatus();

// Check that rebalance type is Delta or Debt
// And then check that rebalance conditions are met
// Note that Delta rebalancing requires vault's delta strategy to be Neutral as well
if (rebalanceType == GMXTypes.RebalanceType.Delta && self.delta == GMXTypes.Delta.Neutral) {
if (
-       self.rebalanceCache.healthParams.deltaBefore < self.deltaUpperLimit &&
-       self.rebalanceCache.healthParams.deltaBefore > self.deltaLowerLimit
+       self.rebalanceCache.healthParams.deltaBefore <= self.deltaUpperLimit &&
+       self.rebalanceCache.healthParams.deltaBefore >= self.deltaLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else if (rebalanceType == GMXTypes.RebalanceType.Debt) {
if (
-       self.rebalanceCache.healthParams.debtRatioBefore < self.debtRatioUpperLimit &&
-       self.rebalanceCache.healthParams.debtRatioBefore > self.debtRatioLowerLimit
+       self.rebalanceCache.healthParams.debtRatioBefore <= self.debtRatioUpperLimit &&
+       self.rebalanceCache.healthParams.debtRatioBefore >= self.debtRatioLowerLimit
) revert Errors.InvalidRebalancePreConditions();
} else {
revert Errors.InvalidRebalanceParameters();
}
}