c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

same transactionStructHash can be build multiple times under PolicyValidator.isPolicySignatureValid, due to unincremented nonce, which cause anyone to create transactionStructHash using the same nonce, which was used and passed the signature verification process.

Proof of Concept

PolicyValidator.isPolicySignatureValid used to check that if the policy signature is valid or not, isPolicySignatureValid is used under TransactionValidator function to Validates a transaction on guard before execution, for Brahma console accounts as well as for for subAccounts


 function isPolicySignatureValid(
        address account,
        address to,
        uint256 value,
        bytes memory data,
        Enum.Operation operation,
        bytes calldata signatures
    ) external view returns (bool) {
        // Get nonce from safe
        uint256 nonce = IGnosisSafe(account).nonce();

        // Build transaction struct hash
        bytes32 transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: to,
                value: value,
                data: data,
                operation: uint8(operation),
                account: account,
                executor: address(0),
                nonce: nonce
            })
        );

        // Validate signature
        return isPolicySignatureValid(account, transactionStructHash, signatures);
    }

The problem in the above signature validation function is that the transactionStructHash do not increment nonce after calling from any external function like TransactionValidator.validatePreTransactionOverridable and TransactionValidator.validatePreTransaction, which might be the critical problem here because a user can use the same nonce (which was verified earlier and passes the Policy Signature verification process) to pass the signature validation process and can easily bypassed this function signature validation check. That can cause any malicious user to access Brahma console accounts of any other legitimate user and can do any malicious activity like changing wallet address to his address and could steal funds of any user.

Tools Used

Manual

Recommended Mitigation Steps

do increment nonce each time, whenever the function isPolicySignatureValid get called for the policy signature verification,

 - bytes32 transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: to,
                value: value,
                data: data,
                operation: uint8(operation),
                account: account,
                executor: address(0),
                nonce: nonce
            })
  + bytes32 transactionStructHash = TypeHashHelper._buildTransactionStructHash(
            TypeHashHelper.Transaction({
                to: to,
                value: value,
                data: data,
                operation: uint8(operation),
                account: account,
                executor: address(0),
                nonce: nonce++
            })

Assessed type

Error

c4-pre-sort commented 10 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #129

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

alex-ppg commented 10 months ago

The nonce is in reality incremented whenever a validation occurs as part of a Gnosis Safe operation. As such, each operation on a Gnosis Safe will utilize an updated nonce in the PolicyValidator and thus be secure. Direct invocation of the function is harmless as it does not mutate any state.