search

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

8 stars 7 forks source link

Missing `payable` modifier in ExecutorPlugin.executeTransaction(): Restricts Use of Native Assets (ETH) with Transactions #470

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/dd0b41031b199a0aa214e50758943712f9f574a0/contracts/src/core/ExecutorPlugin.sol#L68

Vulnerability details

Impact

A registered executor for a submodule cannot send ETH (native assets) with a transaction because the payable modifier is missing in the executeTransaction() function.

It's essential to address this issue to ensure full compatibility and functionality for transactions involving native assets.

Proof of Concept

If a user(executor) wants to make a transaction to send ETH to the submodule and swap it for USDC, it won't be possible because of the absence of the payable modifier in the executeTransaction function.

ExecutorPlugin.sol executeTransaction()

    function executeTransaction(
        ExecutionRequest calldata execRequest
    ) external nonReentrant returns (bytes memory) {
        ...
    }

Tools Used

Manual review

Recommended Mitigation Steps

Add payable modifier in the executeTransaction() function

Assessed type

Payable

0xad1onchain commented 1 year ago

Invalid, payable not required because smart contact internally does the transfer call

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #363

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 not a duplicate

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

alex-ppg commented 12 months ago

The relevant functions of the ModuleManager in the official Gnosis Safe implementation do not actually receive any ether in these calls.

As their name implies, executors are meant to act on behalf of Gnosis Safe wallets, they are not meant to fund them or their interactions. To this end, it is sensible that these functions do not accept native funds.

c4-judge commented 12 months ago

alex-ppg marked the issue as unsatisfactory: Invalid