the function _validateExecutionRequest checks the valid excutor account by the address of account given in call data instead of msg.sender which is realy easily exploitble

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

the function _validateExecutionRequest checks the valid excutor account by the address of account given in call data instead of msg.sender which is realy easily exploitable

if you look at the function

  function _validateExecutionRequest(ExecutionRequest calldata execRequest) internal {
        // Fetch executor registry
        ExecutorRegistry executorRegistry =
            ExecutorRegistry(AddressProviderService._getRegistry(_EXECUTOR_REGISTRY_HASH));

        // Check if executor is valid for given account
        if (!executorRegistry.isExecutor(execRequest.account, execRequest.executor)) {
            revert InvalidExecutor();
        }

        // Empty Signature check for EOA executor
        if (execRequest.executor.code.length == 0 && execRequest.executorSignature.length == 0) {
            // Executor is an EOA and no executor signature is provided
            revert InvalidSignature();
        }

        // Build transaction struct hash
        bytes32 _transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: execRequest.exec.target,
                value: execRequest.exec.value,
                data: execRequest.exec.data,
                operation: uint8(SafeHelper._parseOperationEnum(execRequest.exec.callType)),
                account: execRequest.account,
                executor: execRequest.executor,
                nonce: executorNonce[execRequest.account][execRequest.executor]++
            })
        );

        // Build EIP712 digest for transaction struct hash
        bytes32 _transactionDigest = _hashTypedData(_transactionStructHash);

        // Validate executor signature
        if (
            !SignatureCheckerLib.isValidSignatureNow(
                execRequest.executor, _transactionDigest, execRequest.executorSignature
            )
        ) {
            revert InvalidExecutor();
        }

        // Validate policy signature
        TransactionValidator(AddressProviderService._getAuthorizedAddress(_TRANSACTION_VALIDATOR_HASH))
            .validatePreExecutorTransaction(
            msg.sender, execRequest.account, _transactionStructHash, execRequest.validatorSignature
        );
    }

it checks the address of executor with given account address which must be msg.sender otherwise has 0 impact

Proof of Concept

  function _validateExecutionRequest(ExecutionRequest calldata execRequest) internal {
        // Fetch executor registry
        ExecutorRegistry executorRegistry =
            ExecutorRegistry(AddressProviderService._getRegistry(_EXECUTOR_REGISTRY_HASH));

        // Check if executor is valid for given account
        if (!executorRegistry.isExecutor(execRequest.account, execRequest.executor)) {
            revert InvalidExecutor();
        }

        // Empty Signature check for EOA executor
        if (execRequest.executor.code.length == 0 && execRequest.executorSignature.length == 0) {
            // Executor is an EOA and no executor signature is provided
            revert InvalidSignature();
        }

        // Build transaction struct hash
        bytes32 _transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: execRequest.exec.target,
                value: execRequest.exec.value,
                data: execRequest.exec.data,
                operation: uint8(SafeHelper._parseOperationEnum(execRequest.exec.callType)),
                account: execRequest.account,
                executor: execRequest.executor,
                nonce: executorNonce[execRequest.account][execRequest.executor]++
            })
        );

        // Build EIP712 digest for transaction struct hash
        bytes32 _transactionDigest = _hashTypedData(_transactionStructHash);

        // Validate executor signature
        if (
            !SignatureCheckerLib.isValidSignatureNow(
                execRequest.executor, _transactionDigest, execRequest.executorSignature
            )
        ) {
            revert InvalidExecutor();
        }

        // Validate policy signature
        TransactionValidator(AddressProviderService._getAuthorizedAddress(_TRANSACTION_VALIDATOR_HASH))
            .validatePreExecutorTransaction(
            msg.sender, execRequest.account, _transactionStructHash, execRequest.validatorSignature
        );
    }

Tools Used

vscode

Recommended Mitigation Steps

• consider checking excutor ACC with msg.sender instead of address variable in parameters

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]

This is a deliberate Brahma decision; even if the executor does not match the msg.sender, the signed payload provided in the transaction must be signed by the executor.

As the executor as well as the policy validator have authorized the transaction, there is no readily apparent vulnerability from permitting anyone to submit these payloads for execution.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid