Use of incorrect `minDelay` which is deviating from documentation

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x876246e971474cb7ed99a725abaac463ca9568ba6329ae0d96a09f388347aebd Severity: medium

Description: Description\

To set the critical parameters i.e Admin and Exit fee, EthMultiVault.sol has used timelock mechanism with minimum delay so once this delay is passed then both of these critical parameters can be set in EthMultiVault.sol contract.

minDelay is the part of General configuration struct which is a part of EthMultiVault.sol contract.

The issue is that, the deployment script considers minDelay as 1 days or 24 hours but the documentation clearly states 12 hours.

As per deployment script:

        IEthMultiVault.GeneralConfig memory generalConfig = IEthMultiVault.GeneralConfig({
            admin: admin, // Admin address for the EthMultiVault contract
            protocolVault: protocolVault, // Protocol vault address (should be a multisig in production)
            feeDenominator: 10000, // Common denominator for fee calculations
            minDeposit: 0.0003 ether, // Minimum deposit amount in wei
            minShare: 1e5, // Minimum share amount (e.g., for vault initialization)
            atomUriMaxLength: 250, // Maximum length of the atom URI data that can be passed when creating atom vaults
            decimalPrecision: 1e18, // decimal precision used for calculating share prices
@>          minDelay: 1 days // minimum delay for timelocked transactions
        });

and As per Intuition documentation:

Two timelocks exist to prevent admins from arbitrarily updating the contract and critical parameters. The contract upgradeability timelock has a minimum duration of 2 days, and the critical configurable parameters have a minimum timelock duration of 12 hours.

This difference of 12 hours would affect greatly and considering the documentation upto date, this would break the intended design of Intuition protocol for critical setters like Admin and Exit fee.

The following functions are greatly affected and makes use of minDelay to set or validatations, etc. check (@>) to understand the impact due to this issue

1) scheduleOperation()

    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));

        // Check timelock constraints and schedule the operation
        if (timelocks[operationHash].readyTime != 0) {
            revert Errors.MultiVault_OperationAlreadyScheduled();
        }

        // calculate the time when the operation can be executed
@>        uint256 readyTime = block.timestamp + minDelay;

        timelocks[operationHash] = Timelock({data: data, readyTime: readyTime, executed: false});
    }

2) cancelOperation()

    function cancelOperation(bytes32 operationId, bytes calldata data) external onlyAdmin {
        // Generate the operation hash
@>        bytes32 operationHash = keccak256(abi.encodePacked(operationId, data, generalConfig.minDelay));

3) setAdmin()

    function setAdmin(address admin) external onlyAdmin {
        // Generate the operation hash
        bytes memory data = abi.encodeWithSelector(EthMultiVault.setAdmin.selector, admin);
@>        bytes32 opHash = keccak256(abi.encodePacked(SET_ADMIN, data, generalConfig.minDelay));

4) setExitFee()

    function setExitFee(uint256 id, uint256 exitFee) external onlyAdmin {
        uint256 maxExitFeePercentage = generalConfig.feeDenominator / 10;

        if (exitFee > maxExitFeePercentage) {
            revert Errors.MultiVault_InvalidExitFee();
        }

        // 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));

It should be noted that, minDelay can not be changed once set so this would be treated as Immutable and with this value in production, it surely deviates from documentation so Medium severity is more appropriate here.

Recommendations\ Consider using 12 hours instead of 1 days for minimum delay of critical setters i.e Admin and Exit fee.

On issue severity\

Low severity if documentation is incorrect and contracts minimum delay is correct and should be corrected as per contest rules such issues are low severity.

[mihailo-maksa]

The reported issue concerning the discrepancy between the minDelay value in the deployment script and the documentation has been reviewed. Here is our detailed perspective:

Documentation Correction: The documentation states that the minimum delay for timelocked operations should be 12 hours, whereas the deployment script sets it to 24 hours. This discrepancy indicates that the documentation needs to be updated to accurately reflect the deployed contract's settings.

Impact Assessment: Since the deployed minDelay of 24 hours does not introduce any security risks or functional problems, this issue is primarily about aligning the documentation with the actual deployment. Ensuring accurate documentation is essential for maintaining user trust and clarity.

Severity Assessment: Given that this issue does not introduce any vulnerabilities or risks to users, it is classified as low severity. The primary concern is the inconsistency between the documentation and the actual deployment.

Conclusion: The discrepancy between the deployed minDelay value and the documentation needs to be addressed. We recommend updating the documentation to reflect the deployed minDelay of 24 hours. This issue is more of a documentation enhancement rather than a security vulnerability.

Status: This issue is a documentation enhancement.

Comment for the Reporter: Thank you for highlighting this discrepancy. The documentation states that the minimum delay for timelocked operations should be 12 hours, but the contract was deployed with a minDelay of 24 hours. Since this does not harm users and is more of a documentation issue, we classify it as a low severity enhancement. We can still consider a lower payout for this valid suggestion.

[0xRizwan]

@mihailo-maksa This should be labelled as minor issue since documentation were tagged earlier.

[mihailo-maksa]

Here is our detailed perspective:

• Criteria Assessment: This issue doesn’t meet any of the criteria for acceptance as a security vulnerability. The discrepancy between the documentation and the deployment script does not compromise security or functionality.
• Technical Accuracy: We are technically correct. A minDelay of 24 hours exceeds the 12-hour requirement, providing an even longer safeguard period.
• Final Judgement: The issue is invalid. The initial tagging as a documentation issue does not impact the validity of our assessment.

Detailed Criteria

Scope of the Bug Bounty Program

The scope includes the core contracts of the Intuition protocol:

• EthMultiVault.sol
• AtomWallet.sol

Examples of What's in Scope

• Stealing or freezing user assets (ETH)
• Unauthorized pausing or unpausing of the protocol
• Internal accounting errors leading to incorrect fee charges or asset distribution
• Unauthorized changes to contract configuration

Out of Scope

• Non-security-related suggestions (we believe your issue lies here)
• Efficiency improvements
• Known weaknesses documented in audits

Severity Levels (examples)

• High/Critical: Unauthorized manipulation, theft, freezing of funds
• Medium: Exploits compromising user experience
• Low: Non-critical functionality issues without user fund loss

Impact and Recommendations

• Impact Assessment: The minDelay of 24 hours does not introduce any security risks or functional problems.
• Recommendation: Update the documentation to reflect the deployed minDelay of 24 hours.

Please refer to the readme file for more details on intended behavior and the developer docs.

Thank you for highlighting this discrepancy. We appreciate your input, but our final judgement is that this issue is invalid.