eLSeR17 - Users are likely to get confused, as units in docs/comments do not match units in the code

[sherlock-admin2]

eLSeR17

Medium

Users are likely to get confused, as units in docs/comments do not match units in the code

Summary

Users are likely to get confused, as units in docs/comments do not match units in the code.

Root Cause

In 'MorphoLeverageStrategyExtension.sol', MethodologySettings and ExecutionSettings structs define some units format for variables that do not correspond to the real units shown in the code (even if the code will not revert as units actually ARE consistent inside the code).

https://github.com/sherlock-audit/2024-10-morpho-x-index/blob/main/index-coop-smart-contracts/contracts/adapters/MorphoLeverageStrategyExtension.sol#L79-L106

According to code, PRECISE_UNIT = 10**18, but the following variables from the mentioned struct are not consisten with this:

 uint256 targetLeverageRatio;                     // Long term target ratio in precise units (10e18)
        uint256 minLeverageRatio;                        // In precise units (10e18). If current leverage is below, rebalance target is this ratio
        uint256 maxLeverageRatio;                        // In precise units (10e18). If current leverage is above, rebalance target is this ratio
        uint256 recenteringSpeed;                        // % at which to rebalance back to target leverage in precise units (10e18)
.
.
.
uint256 unutilizedLeveragePercentage;            // Percent of max borrow left unutilized in precise units (1% = 10e16)

This happens because 10e18 = 10 * 10 ** 18 = 10 ** 19, also if 1% = 10e16, then 100% = 10e18 = 10 ** 19, which would exceed PRECISE_UNIT (1e18). As a result, the percentage that these variable show will be 10 times higher in reality than what a common user would think after reading the comments. Confusion results even more likely after the discrepancy that slippageTolerance DOES HAVE the correct units format in its comment, which would discourage users to believe that the whole contract has wrong units:

uint256 slippageTolerance;                      // Allowable percent trade slippage in preciseUnits (1% = 10^16)

Take into account that these same structs and their comments are shown in the official docs of the protocol: https://docs.indexcoop.com/index-coop-community-handbook/products/leverage/flexible-leverage-indices/fli-technical-documentation/flexibleleveragestrategyadapter

Attack Path

1. Deployer deploys 'MorphoLeverageStrategyExtension' contract, as part of the developer team, it will probably use the correct format units for the mentioned state variables.
2. After some time of normal use of the system, userA sees that unutilizedLeveragePercentage = 7 * 10e16, according to docs this is 7%; but the real value is 70%. Take into account that 7 * 10e16 = 7 * 1017 , and PRECISE_UNIT = 1018.
3. UserA calls rebalance() or ripcord(), which uses _calculateMaxBorrowCollateral(), which involves unutilizedLeveragePercentage.
4. The outcome is totally different to what the user expected, as the value of unutilizedLeveragePercentage was 10 times higher than what he thought after reading docs.

Impact

Users will be confused/mistaken as the units in which variables are expressed in the official docs do not correspond to the reality of the code. This is a true issue if users rebalance() or ripcord() after checking the value of any of the mentioned variables, as they will get an unexpected outcome because of the format discrepancy. Same will happen if any contract or system operates after checking variables in this contract.

Mitigation

Change comments in the code and official docs to properly express the format units for this variables.

Example:

uint256 unutilizedLeveragePercentage;            // Percent of max borrow left unutilized in precise units (1% = 1e16)

or

uint256 unutilizedLeveragePercentage;            // Percent of max borrow left unutilized in precise units (1% = 10**16)