The `updatePolicy` function does not check if the policy is registered in the `PolicyRegistry` before updating it

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleOpBuilder.sol#L30 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleOpBuilder.sol#L68 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleOpBuilder.sol#L95 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/ConsoleOpBuilder.sol#L149 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/PolicyRegistry.sol#L66 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/registries/PolicyRegistry.sol#L35

Vulnerability details

Impact

Unverified Policy Activation in ConsoleOpBuilder

• Severity: High Impact
• Affected Code: updatePolicy and _updatePolicy function in PolicyRegistry.sol around line 66 and 35.
• Details:
  • The enablePolicyOnConsole function is responsible for generating multicall bytecode to enable a policy on a Brahma console account.
  • Currently, the function does not verify if the provided policy is already registered in the PolicyRegistry before generating the bytecode.
  • This omission creates an opportunity for an attacker to generate bytecode for any arbitrary policy, even if it's not registered or recognized by the system.
  • If this bytecode gets executed, it can associate the Brahma console account with an invalid or potentially malicious policy.
  • Such a flaw can compromise the entire system's operations if other parts of the system rely on these policies for decision-making, validation, or any other operation.
  • Users who unknowingly execute such malicious bytecode could have their accounts associated with policies that could lead to loss of funds, unauthorized actions, or other unintended consequences.

Proof of Concept

Exploiting the Unverified Policy Activation in ConsoleOpBuilder

1. Target Function: enablePolicyOnConsole in ConsoleOpBuilder.sol (line 23).

2. Attack Scenario:

   • Assume an attacker identifies the lack of policy validation in the enablePolicyOnConsole function.
   • The attacker crafts a malicious policy that, when associated with a Brahma console account, can exploit the system in some manner (e.g., bypassing certain checks, granting unauthorized access, etc.).
   • The attacker then calls the enablePolicyOnConsole function with a valid Brahma console account and their malicious policy's commit hash.
   • The function, without verifying the policy's validity, generates the multicall bytecode to associate the account with the malicious policy.
   • The attacker or an unknowing user then executes this bytecode, leading to the Brahma console account being associated with the malicious policy.

3. Consequences:

   • The system might now treat the Brahma console account as being associated with a valid policy, even though it's malicious.
   • Depending on the system's reliance on these policies, this can lead to a range of vulnerabilities, from unauthorized actions to potential loss of funds.
   • Other users or systems interacting with this compromised Brahma console account might face unintended consequences due to the malicious policy.

4. Verification:

   • To verify this vulnerability, one can attempt to call enablePolicyOnConsole with a valid Brahma console account and an arbitrary, unregistered policy commit hash.
   • If the function successfully generates the bytecode without any error, it confirms the lack of policy validation.

    function enablePolicyOnConsole(address account, bytes32 policyCommit) external view returns (bytes memory) {
        Types.Executable[] memory txns = new Types.Executable[](3);

        txns[0] = Types.Executable({
            callType: Types.CallType.CALL,
            target: AddressProviderService._getRegistry(_POLICY_REGISTRY_HASH),
            value: 0,
            data: abi.encodeCall(PolicyRegistry.updatePolicy, (account, policyCommit))
        });

        txns[1] = Types.Executable({
            callType: Types.CallType.CALL,
            target: account,
            value: 0,
            data: abi.encodeCall(
                IGnosisSafe.setGuard, (AddressProviderService._getAuthorizedAddress(_SAFE_MODERATOR_OVERRIDABLE_HASH))
                )
        });

        txns[2] = Types.Executable({
            callType: Types.CallType.CALL,
            target: account,
            value: 0,
            data: abi.encodeCall(
                IGnosisSafe.setFallbackHandler,
                (AddressProviderService._getAuthorizedAddress(_CONSOLE_FALLBACK_HANDLER_HASH))
                )
        });

        return abi.encodeCall(IGnosisMultiSend.multiSend, (SafeHelper._packMultisendTxns(txns)));
    }

Tools Used

VS Code

Recommended Mitigation Steps

For the vulnerability identified in the enablePolicyOnConsole function of the ConsoleOpBuilder.sol contract:

1. Validate Policy Before Activation:

   • Implement a check in the enablePolicyOnConsole function to validate the policy's registration status in the PolicyRegistry before generating the multicall bytecode.
   • This could be done by adding a function in the PolicyRegistry contract that verifies if a given policy is registered.

2. Access Control:

   • Restrict the set of addresses that can call the enablePolicyOnConsole function to only trusted entities.
   • This can be achieved using a modifier that checks the msg.sender against a list of authorized addresses.

3. Rate Limiting:

   • Introduce rate limiting to prevent rapid or bulk policy changes, which can reduce the impact of potential attacks.
   • This can be implemented using a cooldown period between policy changes for each account.

Recommended to fix it:

1. In PolicyRegistry.sol:

   
   // Add a mapping to track registered policies
   mapping(bytes32 => bool) public isPolicyRegistered;

// Modify the existing function that registers a policy (assuming such a function exists) function registerPolicy(bytes32 policyCommit) external { // Your existing logic for policy registration...

// Mark the policy as registered
isPolicyRegistered[policyCommit] = true;

}

// Add a function to check if a policy is registered function isRegisteredPolicy(bytes32 policyCommit) external view returns (bool) { return isPolicyRegistered[policyCommit]; }

2. In ConsoleOpBuilder.sol:
Before the bytecode generation in the `enablePolicyOnConsole` function:
```solidity
// Ensure the policy is registered before enabling
require(
    PolicyRegistry(AddressProviderService._getRegistry(_POLICY_REGISTRY_HASH)).isRegisteredPolicy(policyCommit),
    "Policy not registered"
);

We're tracking registered policies in the PolicyRegistry. We're checking in the ConsoleOpBuilder if the policy being set is registered.

---

Issue Type: Improper Input Validation

Explanation:

Improper input validation refers to the absence of checks or validations for the inputs that a function or system receives. When a system doesn't validate or sanitize its inputs adequately, it can be susceptible to various types of attacks. In this specific context, the enablePolicyOnConsole function in the ConsoleOpBuilder.sol contract doesn't validate if the provided policy (input) is registered in the PolicyRegistry before processing it.

Consequences:

1. Potential Association with Malicious Policies: Attackers can associate user accounts with malicious or unrecognized policies.
2. Compromised System Operations: If the system relies on these policies for various operations or decisions, having an unrecognized policy can lead to unintended behaviors, unauthorized actions, or security breaches.
3. Loss of Trust: Users might lose trust in the platform if they unknowingly get associated with malicious policies.

In the Broader Context:

Improper input validation is a common vulnerability in software development. It's especially critical in blockchain and smart contract development due to the immutable nature of deployed contracts. Once a vulnerability is exploited, it can't be easily rectified without updating the contract or, in some cases, performing a network fork.

Assessed type

Invalid Validation

[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]

The Warden specifies functions in an out-of-scope contract.

Additionally, the ConsoleOpBuilder contract is meant to be used as a utility that enables the generation of ABI-encoded payloads for submission to f.e. a Gnosis Safe multi-call.

It cannot directly influence how an arbitrary Gnosis Safe is configured as the payload would need to be approved by its signers and the relevant validation processes (i.e. PolicyValidator).

[c4-judge]

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