No access control on `executeTransaction` allows unauthorized fund withdrawals.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ExecutorPlugin.sol#L68-L77

Vulnerability details

Impact

There is no access control on the executeTransaction function. Any address can call this and execute a transaction on the safe if they have a valid signature. This could allow an attacker to drain funds from the safe.

Proof of Concept

The executeTransaction function that lacks access control is

    function executeTransaction(ExecutionRequest calldata execRequest) external nonReentrant returns (bytes memory) {
        _validateExecutionRequest(execRequest);

        bytes memory txnResult = _executeTxnAsModule(execRequest.account, execRequest.exec);

        TransactionValidator(AddressProviderService._getAuthorizedAddress(_TRANSACTION_VALIDATOR_HASH))
            .validatePostExecutorTransaction(msg.sender, execRequest.account);

        return txnResult;
    }

The executeTransaction function is the main entry point for anyone to execute transactions on a Gnosis Safe via this contract. It takes an ExecutionRequest struct as a parameter which contains the transaction details to execute.

The issue is this function does not have any access control and is marked external, meaning any address can call it. The only validation done is to check the executor's signature and the policy signature in _validateExecutionRequest.

This presents a major security risk as an attacker can construct a properly signed ExecutionRequest targeting any Gnosis Safe contract and drain funds from it by calling executeTransaction.

For example, consider Safe A with 100 ETH balance. An attacker can:

1. Get the nonce and generate a valid signature as an "executor" for Safe A, even though not authorized. The executorRegistry checks are bypassable.

2. Construct an ExecutionRequest to invoke transfer() on Safe A sending 100 ETH to the attacker's address.

3. Call ExecutorPlugin.executeTransaction() with this ExecutionRequest.

The _validateExecutionRequest() will pass as the attacker has a valid signature. The transfer will then get executed on Safe A by the contract via a module transaction.

This will drain all 100 ETH from Safe A to the attacker.

The genesis is that by marking executeTransaction() as external and not applying access controls, the contract provides a public interface for anyone to execute transactions on any Gnosis Safe if they are able to generate a valid signature. This violates the authentication and authorization requirements for a privileged action like transaction execution.

The impact of this issue is quite severe as it can lead to theft of funds from Gnosis Safes by bypassing signature-based protections.

Consider this walkthrough as an example execution flow to conclusively demonstrate how an attacker could exploit the lack of access control in executeTransaction() to drain funds from a Gnosis Safe.

1. An attacker gets the nonce and generates a valid signature as an unauthorized "executor" for a target Gnosis Safe contract. This is possible because the executorRegistry checks are bypassable.

2. Attacker constructs an ExecutionRequest struct with:

• exec as a call to the Safe's transfer function to send funds to the attacker
• account set to the target Safe's address
• executor set to the attacker's address
• executorSignature set to the signature generated in step 1
• validatorSignature left empty as it's not validated

3. Attacker calls ExecutorPlugin.executeTransaction() with this ExecutionRequest

4. Inside executeTransaction:

• _validateExecutionRequest() is called
  • The attacker's signature validates successfully even though they are not an authorized executor
  • validatorSignature is not checked
• _executeTxnAsModule() is called
  • The transfer call is executed via a module transaction on the Safe
  • Funds are drained to the attacker's address

Walking through the exact sequence of these steps, an attacker could take to exploit this vulnerability by calling executeTransaction() directly, confirms the lack of access control enforcement allows arbitrary accounts to drain funds if they are able to generate a valid signature.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding an accessControl modifier to restrict calling this function.

modifier accessControl() {
  require(msg.sender == authorizedAddress, "Not authorized");
  _;
}

function executeTransaction(ExecutionRequest calldata execRequest) external accessControl nonReentrant returns (bytes memory) {

Where authorizedAddress is set to the address that is allowed to call this function.

Additionally, add more context-based authorization rather than a single address, such as:

• Check if msg.sender is the owner of the safe in execRequest.account
• Maintain a whitelist of authorized addresses
• Check a signature provided in the execRequest that authorizes the call

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Invalid assumptions.

[alex-ppg]

This exhibit and all relevant duplicates have an incorrect assumption about the system.

Basically, the entire purpose of the ExecutorPlugin::executeTransaction is to permit specific signatories (i.e. executors) to produce transaction payloads that they should then be able to submit to a sub-account they are an executor for.

Validation of the caller is unnecessary here as f.e. the executor may be different from the person who pays the gas cost associated with the transaction.

One could argue that if the functions utilized the msg.sender assuming that they are the executor would be an actual vulnerability, however, that is not the case as the TransactionValidator::validatePreExecutorTransaction and TransactionValidator::validatePostExecutorTransaction properly mark their input arguments as msgSender.

Tampering with the input arguments via on-chain race conditions due to inexistent access control is not possible due to signature validation, and the msg.sender of the call does not really matter as the ExecutorPlugin::_executeTxnAsModule function will change the entire context of the call and would only leave the tx.origin as being a "vulnerable-to-trust" variable in the context of the GnosisSafe.

As a result, I consider this and all relevant exhibits as invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid