search

Gearbox-protocol / core-v3

Other
28 stars 4 forks source link

feat: add checks on debt limits #261

Closed lekhovitsky closed 1 month ago

lekhovitsky commented 1 month ago

Fixes https://github.com/spearbit-audits/review-gearbox/issues/56

Additionally, introduces a crucial check that maxDebt / minDebt doesn't exceed a safety threshold.

StErMi commented 1 month ago

@lekhovitsky would you mind explaining this new check?

        if (maxDebt * CreditManagerV3(creditManager).maxEnabledTokens() > minDebt * 100) {
            revert IncorrectLimitsException(); // I:[CC-15]
        }

The above check is out of the scope of the fix review and I don't see a direct correlation between the minDebt or maxDebt with the number of max enabled token that are related to the collateral enabled (some quota has been purchased to later on borrow)

StErMi commented 1 month ago

Excluding the comment above, the implementation seems correct.

Note that the above checks are valid only for the current (when the checks are performed) configuration of the PriceOracleV3 instance.

If PriceOracleV3.setPriceFeed(underlying, ...) is called or if the value returned by PriceOracleV3.convertToUSD(minDebt, underlying) is later on equal to zero, those checks are not valid anymore. For this reason, they cannot be considered an invariant that is always held after it has been proven at the moment of execution of CreditConfiguratorV3.setPriceOracle or CreditConfiguratorV3.setDebtLimits

Note also that these checks are implemented directly on the CreditConfiguratorV3 instance but the CreditManagerV3 is deployed with the creditConfigurator == msg.sender and there's no assurance that the deployer is indeed a valid credit configurator instead of an EOA or another contract that won't perform those checks.

As already mentioned in other findings, Gearbox should consider moving to the base contract (in this case the CreditManager) all these checks.

StErMi commented 1 month ago

Note also that _setLimits (where some of the checks on the minDebt value are made) is also indirectly called when the CreditConfiguratorV3.setCreditFacade(address newCreditFacade, bool migrateParams) is executed with migrateParams == true.

Such transaction could revert if the current USD conversion of the current minDebt value is equal to zero. This is a minor issue because the configurator can work around the situation by updating the minDebt value and re-execute the setCreditFacade function.

lekhovitsky commented 1 month ago

Such transaction could revert if the current USD conversion of the current minDebt value is equal to zero.

This scenario is a severe misconfig and is probably not really worth considering.