Upgrading the contract with changing `minDelay` will make `scheduleOperation` unable to execute

[hats-bug-reporter[bot]]

Github username: @Al-Qa-qa Twitter username: al_qa_qa Submission hash (on-chain): 0xf9e251f7efb47b738f17b5d90747e2f2b97025d00a6ca288b2f93481a97aa244 Severity: low

Description: Description\ EthMultiVault is an Upgradable contract with ProxyAdmin, where this admin can change the implementation contract anytime.

To execute setAdmin and setExitFees, there should be scheduled first to do these tasks.

EthMultiVault.sol#L167

    function scheduleOperation(bytes32 operationId, bytes calldata data) external onlyAdmin {
        uint256 minDelay = generalConfig.minDelay;

        // Generate the operation hash
        bytes32 operationHash = keccak256(abi.encodePacked(operationId, data, minDelay));
        ...
    }

The problem lies is that when scheduling them, they use global minDelay value, and when executing setExitFees it checks that the hash is the same to execute it.

EthMultiVault.sol#L309-L312

        bytes memory data = abi.encodeWithSelector(EthMultiVault.setExitFee.selector, id, exitFee);
@>      bytes32 opHash = keccak256(abi.encodePacked(SET_EXIT_FEE, data, generalConfig.minDelay));

        // Check timelock constraints
@>      _validateTimelock(opHash);

The problem lies is that the protocol deals if minDelay changed when upgrading the contract, the scheduled operation will not be able to get executed. as the hashing will differ.

Recommendations\

When scheduling we should use the GlobalConfig parameter, but in executing we can pass this variable, so that if it is changed. we will still be able to execute the scheduled operation.

EthMultiVault::setExitFee()

-   function setExitFee(uint256 id, uint256 exitFee) external onlyAdmin {
+    function setExitFee(uint256 id, uint256 exitFee, uint256 minDelay) external onlyAdmin {

    ...

    // Generate the operation hash
    bytes memory data = abi.encodeWithSelector(EthMultiVault.setExitFee.selector, id, exitFee);

• bytes32 opHash = keccak256(abi.encodePacked(SET_EXIT_FEE, data, generalConfig.minDelay));

• bytes32 opHash = keccak256(abi.encodePacked(SET_EXIT_FEE, data, minDelay));

  ...

  }

Same in setAdmin()

[mihailo-maksa]

The reported issue concerning the potential impact of changing minDelay on the execution of scheduled operations has been reviewed. Here is our detailed response:

Intended Design: The minDelay parameter is designed to ensure a fixed delay period for executing certain operations. We intentionally do not provide a setMinDelay function or similar mechanism to change this delay dynamically. This design choice is made to guarantee transparency and trust for our users, ensuring that the minDelay remains consistent and predictable.

Operational Integrity: The current implementation ensures that any change to the minDelay parameter is handled through a controlled upgrade process. This approach prevents unexpected changes to the delay period, maintaining the integrity and reliability of scheduled operations.

Upgrade Process Safeguards: The risk associated with changing the minDelay during an upgrade is mitigated by the enforced minDelay of 2 days for contract upgrades. This delay provides sufficient time for users and administrators to adjust and reschedule operations if necessary.

Worst Case Scenario: In the worst-case scenario, if the minDelay is changed during an upgrade, the admin would need to wait for the new minDelay period to expire before rescheduling the operation. This scenario is not a significant concern as it is an admin-restricted operation, ensuring that only authorized personnel handle such adjustments.

User Assurance: By maintaining a fixed minDelay, we provide our users with a reliable and transparent operational environment. Users can trust that the delay period will not change unexpectedly, ensuring a stable and secure system.

Conclusion: The design and implementation of the minDelay parameter are intentional and serve to enhance transparency and trust. The potential issue raised does not constitute a vulnerability but rather highlights the robustness of our design. Therefore, we consider this issue to be invalid.

Status: This issue is invalid.