TransactionValidator does not validate gas token address and gas price parameter when validating the transaction

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/TransactionValidator.sol#L64

Vulnerability details

Impact

TransactionValidator does not validate gas token address and gas price parameter when validating the transaction

Proof of Concept

the safe transaction struct is listed below:

    struct SafeTransactionParams {
        Enum.Operation operation;
        address from;
        address to;
        address payable refundReceiver;
        address gasToken;
        address msgSender;
        uint256 value;
        uint256 safeTxGas;
        uint256 baseGas;
        uint256 gasPrice;
        bytes data;
        bytes signatures;
    }

However, when validating the transaction, only the parameter from, to, value and data and operation type and signature is used

    function validatePreTransactionOverridable(SafeTransactionParams memory txParams) external view {
        // Check if guard or fallback handler is being removed, if yes, skip policy validation
        if (_isConsoleBeingOverriden(txParams.from, txParams.to, txParams.value, txParams.data, txParams.operation)) {
            return;
        }

        // Validate policy otherwise
        _validatePolicySignature(
            txParams.from, txParams.to, txParams.value, txParams.data, txParams.operation, txParams.signatures
        );
    }

the parameter tx gas and base gas and gas price and refundReceiver and gas token is not part of the data used to validate the policy signature

this is concerning

a more QA impact is when executing the transaction, the refundReceiver is set to wrong address so a wrong address receive the gas refund

a more severe impact is when executing the transaction, arbitrary amount of gas price and gas amount can be passed in

for certain transaction, the transaction may perform low level call and pass the gas parameter around and if there are insufficient gas passed in, to not block the transaction flow,

the low level call sliently revert, and such slient revert may not. be expected

for example, when executing transaction such as

(bool result, bytes memory data) = address(to).call{gas: gasleft()}(msg.data)
if (!result) {
 // does not revert
}

one example transaction is when finalizing the withdraw in L1 optimism bridge, sufficient gas must be provided to finalize the transaction, other wise the fund can be locked in the bridge because transaction sliently revert without reverting the whole transaction

ensuring sufficient gas amount passed in really important

and if the executor pass the gas price too low, the transaction may be pending forever and not executed

Tools Used

Manual Review

Recommended Mitigation Steps

validating gas price, gas amount in transaction validator

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

0xad1onchain (sponsor) confirmed

[0xad1onchain]

Thanks for the issue

[alex-ppg]

While the description of the issue is slightly hard to read and could use some polishing, the warden has articulated why they consider the vulnerability at hand as medium in severity.

The "Recommended Mitigation Steps" chapter while straightforward and correct should have been expanded to contain clearer instructions re-iterating what the concern of the issue is.

In order to consider the issue as "medium" (and potentially higher), we need to evaluate the following impact:

• A combination of gasToken, refundReceiver, and gasPrice can result in funds of the Safe being impacted and there is an incentive to do so

Based on the architecture of Brahma, the potential attack paths where there would be an incentive to do so would be affecting the SubAccount via either an Executor or an Operator. All other exploitation paths would require social engineering as the actual signed payload of a transaction for the Gnosis Safe does include the gasToken, gasPrice, and refundReceiver variables meaning that the signers have properly "sanitized" them.

The exploitation path via an Executor is impossible as an Executor will ultimately result in an IGnosisSafe::execTransactionFromModuleReturnData function invocation which does not permit these parameters to be set.

An exploitation path via an Operator (a delegated owner per the Brahma documentation) would require some form of social engineering as well since operators are only capable of submitting transactions to be signed by other parties; they cannot submit transactions directly for execution by the Gnosis Safe.

I am leaning towards QA for this one and will try to get some feedback from the Warden directly.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as duplicate of #298

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked issue #298 as primary and marked this issue as a duplicate of 298