There is no checking whether the ExecutorPlugin module has been activated or not on the sub-account, this can cause malfunctions if the user wants to execute tx via ExecutorPlugin

[c4-submissions]

Lines of code

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

Vulnerability details

There is no checking whether the ExecutorPlugin module has been activated or not on the sub-account, this can cause malfunctions if the user wants to execute tx via ExecutorPlugin

Impact

Can cause malfunctions if the user wants to execute tx via ExecutorPlugin on the sub-account

Proof of Concept

If the user wants to execute tx on a sub-account via the ExecutorPlugin then, of course, the ExecutorPlugin as a module on the sub-account must be activated. The basis of the sub-account is Gnosif-Safe too, in which there is an enableModule function whose function is to activate the module on the sub-account. But in this case that never happened.

In this document it can be seen clearly how tx can be executed on sub-accounts via the ExecutorPlugin. For Executor to make a transaction on SubAccount via ExecutorPlugin. Execution via ExecutorPlugin executes in 3 steps;

1. The Executor calls executeTransaction() function on ExecutorPlugin enabled as module on SubAccount. The ExecutorPlugin validates that signer Executor is registered on ExecutorRegistry, then transfers the control to TransactionValidator by calling validatePreExecutorTransaction() which in turn transfers the control to PolicyValidator by calling isPolicySignatureValid() to check the Trusted Validator Signature is valid and then returns the control back to ExecutorPlugin via TransactionValidator.
2. ExecutorPlugin executes transaction on SubAccount as module.
3. After execution of module transaction, the ExecutorPlugin calls validatePostExecutorTransaction() function on TransactionValidator. to check that SafeModerator hasn't been removed as guard on SubAccount and ConsoleAccount hasn't been removed as module on SubAccount.

Here is a very good document from the Brahma team.

Checking whether the module is active or not is in the validatePostTransaction function in the TransactionValidator.sol contract, where this function calls the internal function _checkSubAccountSecurityConfig

File : contracts/src/core/TransactionValidator.sol

181 function _checkSubAccountSecurityConfig(address _subAccount) internal view {
182        address guard = SafeHelper._getGuard(_subAccount);
183        address fallbackHandler = SafeHelper._getFallbackHandler(_subAccount);
184
185        // Ensure guard has not been disabled
186        if (guard != AddressProviderService._getAuthorizedAddress(_SAFE_MODERATOR_HASH)) revert InvalidGuard();
187
188        // Ensure fallback handler has not been altered
189        if (fallbackHandler != AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH)) {
190            revert InvalidFallbackHandler();
191        }
192
193        address ownerConsole =
194            WalletRegistry(AddressProviderService._getRegistry(_WALLET_REGISTRY_HASH)).subAccountToWallet(_subAccount);
195
196        // Ensure owner console as a module has not been disabled
197        if (!IGnosisSafe(_subAccount).isModuleEnabled(ownerConsole)) revert InvalidModule();
198    }

In this function (line 197), what is checked is only whether the console account is active as a module or not, but it does not check whether the ExecutorPlugin is active as a module or not. If the transaction continues then unusual behavior may occur on the transaction.

Tools Used

Manual review

Recommended Mitigation Steps

Check if the ExecutorPlugin module is enabled, revert if not.

if (!IGnosisSafe(_subAccount).isModuleEnabled(executorPlugin)) revert InvalidModule();

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #39

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

In the case that the ExecutorPlugin is not properly enabled as a module, the transaction will simply fail when attempting to perform the IGnosisSafe::execTransactionFromModule function.

As such, no security concern can arise from this behaviour. Sub-accounts are meant to enable the ExecutorPlugin on a need-to basis as it may not be required by all sub-accounts.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid