hats-finance / Inverter-Network-0xe47e52c4fea05e555920f1dcdcc6fb8eca103eeb

Fork of the Inverter Smart Contracts Repository
GNU Lesser General Public License v3.0
0 stars 3 forks source link

Duplicate Module Titles Cause Incorrect Module Address Retrieval #58

Open hats-bug-reporter[bot] opened 3 months ago

hats-bug-reporter[bot] commented 3 months ago

Github username: @0xmahdirostami Twitter username: 0xmahdirostami Submission hash (on-chain): 0xa3617c930f1addf5a6c906825e76cb692aa16953e2455ee921f65c2624e1a237 Severity: low

Description: Description: When adding a new module to the orchestrator, the system does not check whether a module with the same title already exists. This can lead to duplicate module titles, which can result in incorrect values being returned by the _isModuleUsedInOrchestrator function.

Scenario: If two modules with the same title are added, the findModuleAddressInOrchestrator function will be unable to differentiate between them. This can cause the function to return the wrong module address.

Impact: The incorrect module address returned by findModuleAddressInOrchestrator can lead to various issues, including the execution of unintended modules, potential security vulnerabilities, and unexpected behavior of the system.

Mitigation: Implement a check for duplicate module titles when adding new modules to ensure that each module has a unique title. This will prevent the addition of modules with duplicate titles and ensure the correct functioning of findModuleAddressInOrchestrator.

0xmahdirostami commented 3 months ago

Another scenario:

Other Mitigation:

Instead of title, use identifier.

Titles could be the same, but identifiers not.

    function identifier(IModule_v1.Metadata memory metadata)
        internal
        pure
        returns (bytes32)
    {
        return keccak256(
            abi.encode(metadata.majorVersion, metadata.url, metadata.title)
        );
    }
FHieser commented 3 months ago

Yes youre correct here. It seems we forgot to add the disclaimer in the natspec, as we knew of that interactions. I would consider this low though as the function is basically not used anywhere.