the operators of the sub account can execute any transaction(not restricted by policy ) to a 3rd party without going through the policy validation process by the trustedValidator

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L39-L55 https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L42-L44

Vulnerability details

Impact

this vulnerability will cause the tokens of the sub accounts to be stolen or perform any activity on the subAcoounts without the validation against the policy and will allow the operators to execute transactions that are not restricted by the policy of the console account when interacting with a 3rd party which is considered a huge loss of funds for the owners of the console accounts .

Proof of Concept

this vulnerability arises from the absent of the policy validation if the operators used the pre-validated messages scheme which will check that all the requested operators have approved the hash of the message in the safe contract ,as shown here https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/lib/safe-contracts/contracts/GnosisSafe.sol#L333-L337

    function approveHash(bytes32 hashToApprove) external {
        require(owners[msg.sender] != address(0), "GS030");
        approvedHashes[msg.sender][hashToApprove] = 1;
        emit ApproveHash(hashToApprove, msg.sender);
    }

so when the operator provide the data of the transaction to a 3rd party without signature , the 3rd party will check the ERC-1271 signature by calling isValidSignature() on the safe contract which will send the call to the consoleFallbackHandler as shown here , https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L39-L55

    function isValidSignature(bytes memory _data, bytes memory _signature) public view override returns (bytes4) {
        // Caller should be a Safe
        GnosisSafe safe = GnosisSafe(payable(msg.sender));
        bytes32 messageHash = getMessageHashForSafe(safe, _data);
        if (_signature.length == 0) {
            require(safe.signedMessages(messageHash) != 0, "Hash not approved");
        } else {
            /// @dev For validating signatures, `PolicyValidator` is invoked to ensure that
            /// the intent of `messageHash` signed by the owners of the safe, complies with its committed
            /// policy
            PolicyValidator policyValidator =
                PolicyValidator(AddressProviderService._getAuthorizedAddress(_POLICY_VALIDATOR_HASH));
            require(policyValidator.isPolicySignatureValid(msg.sender, messageHash, _signature), "Policy not approved");
            safe.checkSignatures(messageHash, _data, _signature);
        }
        return EIP1271_MAGIC_VALUE;
    }

so in the function isValidSignature() there is no policy validation process in case the operators provided signedMessage which will allow the operators to execute any transaction on the subAccount without being restricted by the policy that is applied by the console account

        bytes32 messageHash = getMessageHashForSafe(safe, _data);
        if (_signature.length == 0) {
            require(safe.signedMessages(messageHash) != 0, "Hash not approved");

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L42-L44 although there is a policy validation step if there is signature provided , as shown here https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ConsoleFallbackHandler.sol#L45-L53

        } else {
            /// @dev For validating signatures, `PolicyValidator` is invoked to ensure that
            /// the intent of `messageHash` signed by the owners of the safe, complies with its committed
            /// policy
            PolicyValidator policyValidator =
                PolicyValidator(AddressProviderService._getAuthorizedAddress(_POLICY_VALIDATOR_HASH));
            require(policyValidator.isPolicySignatureValid(msg.sender, messageHash, _signature), "Policy not approved");
            safe.checkSignatures(messageHash, _data, _signature);
        }

POC

the scenario of this vulnerability to be exploited will be : 1) the operators approved the hash of a Malicious transaction that is prevented by the policy 2) the operators provided this data without signatureto the 3rd party such as uniswap . 3) the 3rd party will want to check whether this data is pre-validated by the owners of sub account or not by calling the method isValidSignature() on the consoleFallBackHandler contract 4) the code in the consoleFallBackHandler contract will not perform policy validation since there is no signature and will only check that the message is approved by the operators .

       if (_signature.length == 0) {
            require(safe.signedMessages(messageHash) != 0, "Hash not approved");

5) the handler will return the magic value to the 3rd party which means that the data is valid

        return EIP1271_MAGIC_VALUE;

6) then the transaction will be executed on the 3rd party .

Tools Used

manual review

Recommended Mitigation Steps

perform the policy validation process on the data of the transaction by the trusted validator or can prevent executing a transaction by the pre-validated messages scheme .

Assessed type

Context

[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 pre-validated message scheme relies on a Gnosis Safe transaction being executed. As such, the relevant policies will have already been validated albeit at a time in the past.

This does not present an active concern, as the policy validator willingly authorized a transaction to be considered as "validated" in the future via the signedMessages entry.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid

[c4-judge]

alex-ppg marked the issue as primary issue

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid