search

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

8 stars 7 forks source link

A module can bypass the policy check in a console account even if the guard is set #323

Closed c4-submissions closed 10 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModeratorOverridable.sol#L86-L92 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/TransactionValidator.sol#L156-L171

Vulnerability details

Impact

Users can import any kind of Gnosis Safe as a console account, including 1.5+ safes.

If the user uses a module to execute their transaction, instead of calling execTransaction, some safety features will be disabled, even if they are correctly setting the SafeModeratorOverridable guard and the ConsoleFallbackHandler as fallback.

Proof of Concept

  1. Bob is using a Gnosis v1.5, which will be registered as a console account through registerWallet in WalletRegistry
  2. Bob enables SafeModeratorOverridable as a guard, and ConsoleFallbackHandler as a fallback to leverage their policy and security features
  3. Bob executes a transaction through a module installed in their original safe by using execTransactionFromModule. So, checkModuleTransaction will be executed instead of checkTransaction:
if (guard != address(0)) {
    guardHash = Guard(guard).checkModuleTransaction(to, value, data, operation, msg.sender);
}

https://github.com/safe-global/safe-contracts/blob/810fad9a074837e1247ca24ac9e7f77a5dffce19/contracts/base/ModuleManager.sol#L95

However, the former does not have any checks in place:

/**
 * @notice Inherited from IGuard, function is called after executing a Safe transaction during execTransactionViaModule
 * @dev No op-check. Provides compatibility with Safe 1.5 guard over module
 */
/* solhint-disable no-empty-blocks */
function checkModuleTransaction(
    address, /* to */
    uint256, /* value */
    bytes memory, /* data */
    Enum.Operation, /* operation */
    address /* module */
) external override {}

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModeratorOverridable.sol#L86-L92

  1. Policy checks are not performed, and the normal safety checks for removing the guard/fallback handler are also not performed:
/**
 * Following conditions validate if the transaction aims to remove guard or fallback handler on Safe
 *         from == to (safe sending txn to itself)
 *         value == 0
 *         data == abi.encodeCall(IGnosisSafe.setGuard, (address(0))) || abi.encodeCall(IGnosisSafe.setFallbackHandler, (address(0)))
 *         operation == Enum.Operation.Call
 *
 * In case these conditions are met, the guard is being removed, return true
 */
if (_from == _to && _value == 0 && _operation == Enum.Operation.Call) {
    if (SafeHelper._GUARD_REMOVAL_CALLDATA_HASH == keccak256(_data)) {
        return true;
    } else if (SafeHelper._FALLBACK_REMOVAL_CALLDATA_HASH == keccak256(_data)) {
        return true;
    }
}

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/TransactionValidator.sol#L156-L171

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a check similar to checkTransaction to checkModuleTransaction to avoid this scenario.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #16

c4-pre-sort commented 10 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #368

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The Warden specifies usage of an unreleased version of Gnosis Safe (v1.5+) which is still under active development.

The Gnosis Safe system in use by the Brahma system is v1.3 and while they intend to support v1.5+, they will introduce proper support for it when it launches per the Sponsor's statements.

Finally, the exhibit relies on a manual import of a Gnosis Safe with malicious modules installed. This is generally considered out-of-scope for the contest.

c4-judge commented 10 months ago

alex-ppg marked the issue as unsatisfactory: Out of scope

DadeKuma commented 10 months ago

Finally, the exhibit relies on a manual import of a Gnosis Safe with malicious modules installed. This is generally considered out-of-scope for the contest.

According to the README it seems in scope:

Attack ideas (Where to look for bugs)

  • Users can choose to import their own safe wallet in console which can be malicious. Exploring potential attack vectors there.

SafeModeratorOverridable is designed to protect the user by checking the transaction against the related policy, unless the user willingly wants to disable the guard or the fallback.

A user can unknowingly use an unsafe module in their own safe, which can be imported into Brahma, and the guard will fail to protect this user if they use execTransactionFromModule, as the module is capable of bypassing the policy check with any transaction.

The Warden specifies usage of an unreleased version of Gnosis Safe (v1.5+) which is still under active development. The Gnosis Safe system in use by the Brahma system is v1.3 and while they intend to support v1.5+, they will introduce proper support for it when it launches per the Sponsor's statements.

I see a discrepancy between this statement and the judgment in issue #16. If we followed the same rationale, the sponsor should have fixed that issue when they introduced proper support for it, as tests will certainly fail when they upgrade to the new version. Moreover, it's already possible to import external 1.5 safes as console accounts.

According to your comment:

I think the confusion arises from the fact that the various modules of Brahma (i.e. SafeModerator) are meant to be compatible with more versions than the one actually in use by the SubAccount systems etc. given that they represent Gnosis Safe modules that can be attached to any instance, further evidenced by the ability to register any wallet in the WalletRegistry.

Finally, the code for SafeModeratorOverridable does not state that 1.5 is not compatible. In fact, it explicitly states that it provides compatibility:

@dev No op-check. Provides compatibility with Safe 1.5 guard over module

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

alex-ppg commented 10 months ago

Hey @DadeKuma, thanks a lot for providing thorough feedback! Let me get back to each of your points.

According to the README it seems in scope:

The statement of the README file is the following:

Attack ideas (Where to look for bugs)

  • Users can choose to import their own safe wallet in console which can be malicious. Exploring potential attack vectors there.

It implies that a user can import a malicious safe willingly, it does not imply that a user can import a safe that may contain malicious modules they are unaware of.

Additionally, the attack vectors the Sponsor would be interested in would be attack vectors that would affect safes other than the one imported. The Sponsor is more than happy to chime in here if they disagree, however, this is the interpretation they have depicted thus far.

SafeModeratorOverridable is designed to protect the user by checking the transaction against the related policy, unless the user willingly wants to disable the guard or the fallback.

A user can unknowingly use an unsafe module in their own safe, which can be imported into Brahma, and the guard will fail to protect this user if they use execTransactionFromModule, as the module is capable of bypassing the policy check with any transaction.

If we consider this rationale valid, a Gnosis Safe could have a malicious fallback module installed when it is introduced to the Brahma system. The TransactionValidator and all relevant contracts of the Brahma system would be unable to protect the Gnosis Safe in such a case, as the fallback functions can be directly invoked.

As such, the solution proposed would not really cover the scenario of a Gnosis Safe being imported with malicious configuration. This is what is considered out-of-scope as a user could import a non-compliant Gnosis Safe, their own custom contract, etc. A scenario of a Gnosis Safe being imported with a malicious module enabled is identical to having a malicious fallback method enabled.

I see a discrepancy between this statement and the judgment in issue #16. If we followed the same rationale, the sponsor should have fixed that issue when they introduced proper support for it, as tests will certainly fail when they upgrade to the new version. Moreover, it's already possible to import external 1.5 safes as console accounts.

Please do let me know what the discrepancy you identify is as I cannot identify it. The rationale of the exhibit states the following, in summary:

The Sponsor evaluated this and performed the relevant change, specifying that the Brahma system is not compatible with v1.5. They did not actually correct the code to be compatible, as that would be a futile effort given that Gnosis Safe v1.5 is under active development.

In relation to the last sentence of the quoted block, it is not possible to import external 1.5 safes as Console Accounts given that the version has not released yet, at least officially. Sure, it is possible to take the code off the repository and deploy it, but it is impossible to currently deploy official 1.5 Gnosis Safes.

The same statements as above apply to the last statement made in relation to the SafeModeratorOverridable. To re-iterate, compatibility with a version that is under active development is by definition impossible.

More than happy to discuss this further if you have any concerns, however, I will sum up my response in the TL;DR below:

DadeKuma commented 10 months ago

@alex-ppg thanks for your reply.

Suppose this scenario:

  1. A user has a 1.5 safe with no malicious module installed, initially. They register this safe as a console account.
  2. After registering, they set the SafeModeratorOverridable as a guard, and ConsoleFallbackHandler as a fallback, as they want to leverage their security features.
  3. These security features work until the user enables a malicious module in the console account, and they execute a tx through it, thinking that the existing Brahma security features and the v1.5 checks are enough as protection.
  4. The guard is designed to check for the appropriate policy, but in this instance, it will fail to do so, as there are no checks, which should be the same as a non-module tx.

I don't think that this is the same case as a malicious fallback, as the fallback's role is similar to the guard in terms of security, as it provides security checks for valid signatures.

However, modules are meant to provide additional logic to safes, and not protection; there is a big disclaimer in Safe's docs:

Modules add additional functionalities to the Safe contracts. They are smart contracts that implement the Safe’s functionality while separating module logic from the Safe’s core contract. A basic Safe does not require any modules. Adding and removing a module requires confirmation from all owners. Events are emitted whenever a module is added or removed and also whenever a module transaction was successful or failed. ⚠️ WARNING: Modules are a security risk since they can execute arbitrary transactions, so only trusted and audited modules should be added to a Safe. A malicious module can completely takeover a Safe

https://github.com/safe-global/safe-contracts/blob/52ce39c89cc3e3529963100dd774a6a03c098792/docs/overview.md?plain=1#L36

Safe's v1.5 introduces guards' checks for modules for this same exact reason, because they are a security risk, and they weren't available before this version.

Regarding the discrepancy:

The same statements as above apply to the last statement made in relation to the SafeModeratorOverridable. To re-iterate, compatibility with a version that is under active development is by definition impossible.

This is why I don't understand why #16 is valid then. If that's the case, a user cannot import a version that it's in active development, there won't be a signature mismatch, so the function will never revert?

But if users wait until the v1.5 version is released, and they use the same guard code, they will also be vulnerable to this issue. It can happen, supposing that the sponsor fixes #16, but ignores this issue.

It implies that a user can import a malicious safe willingly, it does not imply that a user can import a safe that may contain malicious modules they are unaware of. Additionally, the attack vectors the Sponsor would be interested in would be attack vectors that would affect safes other than the one imported.

I agree with this statement, and it would have been way more interesting than this finding. However, I think it's open to interpretation, as it's highly improbable that this is the case, as console accounts do not interact with other console accounts.

bronzepickaxe commented 10 months ago

fyi @DadeKuma , any safe that has been deployed =< 1.40 can not set SafeModeratorOverridable as their guard because SafeModeratorOverridable will fail the new guard check implemented =< 1.4.0, per changelog

DadeKuma commented 10 months ago

@bronzepickaxe Thanks, I've read that issue, but this issue and issue 16 assume a code that works properly/is fixed.

Otherwise, issue 16 is also impossible for the same reason, as SafeModeratorOverridable cannot be set with the current code, but that issue is considered valid.

alex-ppg commented 10 months ago

Hey @DadeKuma, thanks for pinpointing what the discrepancy you see is and thank you @bronzepickaxe for chiming in.

I think you are mistaken in what #16 is about and what it’s distinction is in relation to your issue. Issue #16 highlights that the code states it’s compatible when it is not, which is a valid observation and one that the Sponsor actively consumed.

Your exhibit states a “vulnerability” that would arise from importing Gnosis Safe implementations of the v1.5 version and up. Apart from being unreleased, your proposed solution would only protect v1.5 Gnosis Safe implementations and up.

All released versions of Gnosis Safe that the project wishes to support would still be “vulnerable” to the inclusion of a module by an Executor. To protect against this, the Sponsor specifies that the Transaction Validator will filter transactions and prevent module inclusions.

We consider that pre-v1.5 transactions are filtered by the Transaction Validator as a fact given that no other solution exists for those versions. As such, the Transaction Validators are already prepared against these types of attacks.

What your exhibit details is that for v1.5 Gnosis Safe implementations imported to the codebase, inclusion of a module is not protected. This is an incorrect assumption, as inclusion of a module is already protected against in all pre-v1.5 Gnosis Safe deployments.

It will not cease functioning for v1.5 Gnosis Safe implementations, regardless of whether this can be secured against on-chain. In order to consider your exhibit valid, we would have to basically state that all pre-v1.5 versions are vulnerable with no available fix. This is not a correct statement as it would invalidate the whole premise the Brahma system is built on.

I hope the above makes sense, please do not hesitate to share any further feedback or clarification you require.

EDIT: One minor point to add, the SafeModeratorOverridable can still be added regardless of #16. The issue states that transactions via the Executor will only fail, all other workflows will work correctly. Keep in mind that the issue pertains to a return variable missing, not a missing function signature.