Modules are not correctly set up

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/SafeEnabler.sol#L43-L54

Vulnerability details

Impact

The Safe's ModuleManager implements additional function setupModules, which firstly sets SENTINEL_MODULES to the modules. There's no such a function in SafeEnabler.sol. This implies, that instead of adding _SENTINEL_MODULES to the modules, we will be adding address(0) there. This may lead to some unexpected behavior, since enabled module will be marked as address(0), which implies it is not enabled.

Proof of Concept

While looking at the documentation and the NatSpec, we can see, that enableModule() is based on the ModuleManager.sol from Safe. This is even explicitly mentioned in the NatSpace:

     *  Refer https://github.com/safe-global/safe-contracts/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/base/ModuleManager.sol#L32C5-L32C5

When we look into ModuleManager.sol, we can spot, that it implements function setupModules which is responsible for setting up the modules. According to that code-base, modules[SENTINEL_MODULES] should point out to SENTINEL_MODULES, which is address(0x1): modules[SENTINEL_MODULES] = SENTINEL_MODULES;.

However, there's no such a code in SafeEnabler.sol. This implies, that when we will run enableModule() for the first time, then modules[module] = modules[_SENTINEL_MODULES] will be assigned to address(0x0), instead of _SENTINEL_MODULES.

File: src/core/SafeEnabler.sol

43:    function enableModule(address module) public {
44:        _onlyDelegateCall();
45:
46:        // Module address cannot be null or sentinel.
47:        // solhint-disable-next-line custom-errors
48:        require(module != address(0) && module != _SENTINEL_MODULES, "GS101");
49:
50:        // Module cannot be added twice.
51:        // solhint-disable-next-line custom-errors
52:        require(modules[module] == address(0), "GS102");
53:        modules[module] = modules[_SENTINEL_MODULES];
54:        modules[_SENTINEL_MODULES] = module;

Since we miss function setupModules() which sets modules[SENTINEL_MODULES] = SENTINEL_MODULES, then, we know that modules[SENTINEL_MODULES] points to address(0). Thus, at line 53, modules[module] is assigned to address(0), instead of _SENTINEL_MODULES.

That way, there won't be _SENTINEL_MODULES in the modules, there will be address(0) instead.

More descriptive PoC:

The proper implementation is in Safe's ModuleManager.sol: https://github.com/safe-global/safe-contracts/blob/186a21a74b327f17fc41217a927dea7064f74604/contracts/base/ModuleManager.sol This is how the code works there:

1. SENTINEL_MODULES is set to address(0x1).
2. There's a function setupModules() which sets modules[SENTINEL_MODULES] = SENTINEL_MODULES;
3. modules[SENTINEL_MODULES] is now address(0x1).
4. Now, we call enableModule(address X).
5. modules[module] = modules[SENTINEL_MODULES];, which implies that modules[X] = modules[SENTINEL_MODULES], so modules[X] = address(1)
6. modules[SENTINEL_MODULES] = module;, which implies that modules[SENTINEL_MODULES] = X

As you see modules[X] = address(1).

The current implementation in SafeEnabler.sol works like this:

1. _SENTINEL_MODULES is set to address(0x1).
2. There's no function setupModules() , which implies that modules[_SENTINEL_MODULES] == address(0)
3. modules[_SENTINEL_MODULES] is still address(0x0).
4. Now, we call enableModule(address X).
5. modules[module] = modules[_SENTINEL_MODULES];, which implies that modules[X] = modules[_SENTINEL_MODULES], so modules[X] = address(0)
6. modules[_SENTINEL_MODULES] = module;, which implies that modules[_SENTINEL_MODULES] = X

As you see, modules[X] = address(0), which means that this module is not enabled at all.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement additional function setupModules - the same as it's done in Safe's ModuleManager.sol

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[alex-ppg]

The ModulesManager::setupModules function is automatically invoked as part of a Gnosis Safe IGnosisSafe::setup execution flow. The SafeEnabler is meant to be the target of delegatecall instructions that permit a bypass of certain security checks that the default ModulesManager enforces.

As such, Gnosis Safe deployments in the Brahma system are correctly instantiated and the SafeEnabler does not need to expose this particular functionality.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid