Issues with time period variables stored in uint8/uint16

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x1179dc69a49592d03a7c836e94c93ccd6cef156d9cd3565600d248bc63ef62bc Severity: medium

Description: Description\

The config or StrategyConfig has some variables which are passed as time periods. Below few varibables using uint8/uint16 is highlighted for the explanation of this issue.

        pauseRebalancePeriodInDays 
        swapCostOffsetInitInDays
        swapCostOffsetTightenStepInDays 
        swapCostOffsetRelaxThresholdInDays 
        swapCostOffsetRelaxStepInDays 
        swapCostOffsetMaxInDays 
        swapCostOffsetMinInDays 
        navLookback1InDays 
        navLookback2InDays 
        navLookback3InDays 

If you see above carefully, all such time period variables are stated to inDays with uint16 and uint8 data type.

The issue here is, all such variables which are expected to used in days can not be used because uint8 and uint16 cant store values for 1 days or more than 1 days.

Let's consider a example,

    /// @notice the number of days for the first NAV decay comparison (e.g., 30 days)
    uint8 public immutable navLookback1InDays;

    /// @notice the number of days for the second NAV decay comparison (e.g., 60 days)
    uint8 public immutable navLookback2InDays;

    /// @notice the number of days for the third NAV decay comparison (e.g., 90 days)
    uint8 public immutable navLookback3InDays;

Per Natspec, navLookback1InDays,navLookback2InDays,navLookback3InDays is expected to be 30 days,60 days,90 days and these values are stored in uint8.

The maximum value of uint8 is 255. and if uint8 is used to store 30 days, it will throw an error.

uint8 navLookback1InDays = 30 days     //25,92,000 seconds

When this is checked it remix IDE, the error was,

TypeError: Type int_const 2592000 is not implicitly convertible to expected type uint8. Literal is too large to fit in uint8.

Since, uint8 can only fit 255 so it will throw error.

Similarly, other variable issue can be understood.

    constructor(
        ISystemRegistry _systemRegistry,
        address _lmpVault,
        LMPStrategyConfig.StrategyConfig memory conf
    ) SecurityBase(address(_systemRegistry.accessController())) {

      . . . some code

        navLookback1InDays = conf.navLookback.lookback1InDays;
        navLookback2InDays = conf.navLookback.lookback2InDays;
        navLookback3InDays = conf.navLookback.lookback3InDays;

      . . . some code

        lastRebalanceTimestamp = uint40(block.timestamp);
    }

All the config varibles are set in constructor. The values which are expected to be used in days can not be used as passing 30 in constructor would count it as 30 seconds instead of 30 days. Some of such variables are immutable so once set can not be set again. In worst scenario, it would result in redeployment of contract due to such issue.

This would break overall functioanalities of LMPStrategy contract. All such variables used for validations would perform correct as days are expected to be validated for time periods and seconds would be validated, therefore this would break the intended design of contracts.

Recommendations\

Use uint32 instead of uint8/uint16 for time period variables as used in different places in inscope contracts. uint32 would ensure number of days and months can be stored and contract functionalities would perform as expected.

[codenutt]

The values of these settings are actually "30" for 30 days. The value is not represented in seconds.