c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeEnabler.sol#L16-L84

Vulnerability details

Impact

Modules are third party accounts and they have some level of access to the GnosisSafe depending on configuration by the account owner. Therefore, they are created and assigned by account owners and they can execute transactions independently but they cannot always be trusted and in such case whereby a module is being controlled by a malicious third party, there should be a logic implemented to disable them, which is present in the original Safe bytecode but it isn't implemented in SafeEnabler.sol.

Proof of Concept

Contract is acclaimed to be in very similar comparisons with the original safe 1.3.0 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeEnabler.sol#L10-L16 but it doesn't ensure similar security guidelines to halt loss of user funds or operations of malicious transactions. However this was implemented in the original safe 1.3.O, by disabling a module peradventure it's been controlled by a malicious actor. https://github.com/safe-global/safe-contracts/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/base/ModuleManager.sol#L32C5-L53

Tools Used

Manual review, Vscode

Recommended Mitigation Steps

Logic to remove a module from the whitelist should be implemented in the SafeEnabler.sol. To halt fraudulent activities by malicious third party


function disableModule(address prevModule, address module) public authorized {
    // Validate module address and check that it corresponds to module index.
    require(module != address(0) && module != SENTINEL_MODULES, "GS101");
    require(modules[prevModule] == module, "GS103");
    modules[prevModule] = modules[module];
    modules[module] = address(0);
    emit DisabledModule(module);
}

## Assessed type

Access Control