search

code-423n4 / 2023-10-brahma-findings

8 stars 7 forks source link

`SafeEnabler.sol` does not provide any functionality to remove/disable previously added Module #249

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

While SafeEnabler.sol implements a function enableModule(), which enables the module for the Safe, there's no way to disable/remove it later.

Whenever some module is/becomes malicious and there would be need to remove them - protocol does not provide such a mechanism.

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 from Safe, we can spot, that it implements not only enableModule(), but also disableModule() which gives a chance to disable previously enabled module. However, SafeEnabler.sol does not implement such a function. Only enableModule() is implemented, which implies, it's not possible to disable previously enabled module.

Tools Used

Manual code review

Recommended Mitigation Steps

Implement additional function disableModule() (it's code is even in Safe's ModuleManager.sol) to allow disabling modules.

Assessed type

Other

raymondfam commented 1 year ago

Similar to N-06 from the bot.

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

alex-ppg commented 12 months ago

This particular SafeEnabler is utilized as a temporary delegatecall replacement to the original by Gnosis Safe to ensure certain security checks are bypassed when initializing a fresh Console Account / sub-account deployment.

This function is never directly invoked nor is it part of a Gnosis Safe, it is only meant to be the target of delegatecall instructions and if a Console Account / sub-account wishes to remove a module, they can invoke the relevant function of the Gnosis Safe counterpart with a delegatecall invocation.

c4-judge commented 12 months ago

alex-ppg marked the issue as unsatisfactory: Invalid