Using `msg.sender` may lead to caller confusion vulnerabilities in proxy calls.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/SafeModerator.sol#L71-L73 https://github.com/code-423n4/2023-10-brahma/blob/c217699448ffd7ec0253472bf0d156e52d45ca71/contracts/src/core/SafeModerator.sol#L71-L73 https://github.com/code-423n4/2023-10-brahma/blob/a6424230052fc47c4215200c19a8eef9b07dfccc/contracts/src/core/TransactionValidator.sol#L105-L107

Vulnerability details

Impact

The use of msg.sender for key operations like validatePostTransaction introduces risk of caller confusion vulnerabilities if the contract is called via a proxy.

Proof of Concept

The use of msg.sender in the validatePostTransaction call does introduce a potential risk of caller confusion and the problematic code is this line: #L71-72

TransactionValidator(...).validatePostTransaction(txHash, success, msg.sender);

Using msg.sender here makes the assumption that msg.sender will always be the address initiating the original transaction.

However, if the SafeModerator contract is called via a proxy contract, msg.sender will be the proxy address rather than the original caller.

This could allow the proxy contract to manipulate the msg.sender value passed to validatePostTransaction and bypass validity checks.

Let me give an in-depth analysis of the risks of using msg.sender instead of tx.origin:

The validatePostTransaction call uses msg.sender to pass the address being validated by the TransactionValidator:

TransactionValidator(...).validatePostTransaction(..., msg.sender)

This assumes msg.sender will always be the original caller who initiated the Safe transaction. However, if the SafeModerator is called via a proxy contract, msg.sender will be the proxy address, not the original caller.

This means the proxy contract can manipulate what gets passed to the TransactionValidator. A malicious proxy could pass any address it wants to validatePostTransaction by changing msg.sender.

This bypasses the entire purpose of the post-transaction validation. The TransactionValidator will validate the proxy address rather than the original caller address that actually submitted the Safe transaction.

An attacker could exploit this by deploying a malicious proxy that calls SafeModerator, and then passes its own address or some other address to the TransactionValidator.

The TransactionValidator would then approve that address, even though it wasn't the original Safe transaction sender. This completely defies the intent of the validity checks.

Here are the specific lines that demonstrate the issue:

In SafeModerator.sol:#L71-72

function checkAfterExecution(bytes32 txHash, bool success) external view override {

  TransactionValidator(...).validatePostTransaction(txHash, success, msg.sender);

}

Here we can see msg.sender being directly passed as the third argument to validatePostTransaction.

Then in TransactionValidator.sol#L105-L106

function validatePostTransaction(

  bytes32 txHash, 

  bool success,

  address supposedSender

) public {

  // Validate supposedSender...

}

This shows that supposedSender is expected to be the original Safe transaction sender.

However, by passing msg.sender, if called via a proxy contract, supposedSender would be the proxy address instead of the real initiator.

These two code snippets demonstrate concretely how msg.sender usage here can lead to the caller confusion issue I described.

Tools Used

Manual Review

Recommended Mitigation Steps

Using tx.origin instead would prevent this issue, as it always refers to the original external account that initiated the full call stack, regardless of any proxy contracts.

Replacing msg.sender with tx.origin would ensure the proper originating account is validated in TransactionValidator, preventing any caller confusion risk.

The root cause is the use of msg.sender without considering a proxy calling context. Switching to tx.origin is the right fix here.

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

[0xad1onchain]

Invalid, please tell/describe a real exploitby using msg.sender instead of txn.origin. Not to mention that using txn.origin is totally impossible in case of GnosisSafe because its a smart contract and not an EOA, so it cannot originate transactions on chain

[c4-sponsor]

0xad1onchain (sponsor) disputed

[alex-ppg]

As the Sponsor states, the Warden does not justify why the usage of a msg.sender for post-transaction validation is insecure. If the Warden can articulate a proper exploitation path that does not require elevated privileges or something of that sort, I would re-consider this particular exhibit.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid