code-423n4 / 2023-10-brahma-findings

8 stars 7 forks source link

Low-level call in `_executeTxnAsModule` bypasses Safe's built-in security checks. #118

Closed c4-submissions closed 12 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ExecutorPlugin.sol#L86-L98 https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/ExecutorPlugin.sol#L90-L93

Vulnerability details

Impact

Module execution is done via a low-level call. This bypasses the Safe's built-in validations.

Proof of Concept

The low-level call happens in the _executeTxnAsModule function

function _executeTxnAsModule(address _account, Types.Executable memory _executable)
  internal 
  returns (bytes memory)
{

  (bool success, bytes memory txnResult) = IGnosisSafe(_account).execTransactionFromModuleReturnData(
    _executable.target,
    _executable.value,
    _executable.data,
    SafeHelper._parseOperationEnum(_executable.callType)  
  );

  ...

}

Specifically on.

IGnosisSafe(_account).execTransactionFromModuleReturnData(
  _executable.target,
  _executable.value,
  _executable.data,
  ...
)

This bypasses Gnosis Safe's standard transaction workflow which includes:

Because _executeTxnAsModule directly calls execTransactionFromModule, these protections are skipped.

This exposes several risks.

Ultimately this could lead to loss of funds due to the circumvention of key Gnosis Safe checks designed to prevent unauthorized or invalid transactions.

Safer approach would be to use the official transaction workflow:

This would enforce all the Gnosis Safe's validity and authorization checks as designed.

The low-level call by _executeTxnAsModule bypasses critical Safe protections that could lead to transaction abuse. But Using the standard workflow would be aligned with Safe's security model.

Tools Used

Manual Review VsCode

Recommended Mitigation Steps

Should execute through the official Safe transaction flow instead.

Assessed type

call/delegatecall

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

raymondfam commented 1 year ago

These are not low-level calls.

alex-ppg commented 12 months ago

The referenced interaction is actually intended and conforms to the Gnosis Safe specification; the ExecutorPlugin simply behaves as a module of the Gnosis Safe.

c4-judge commented 12 months ago

alex-ppg marked the issue as unsatisfactory: Invalid