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

8 stars 7 forks source link

SubAccount operator steal funds via the gas refund mechanism. #298

Open c4-submissions opened 10 months ago

c4-submissions commented 10 months ago

Lines of code

https://github.com/safe-global/safe-contracts/blob/main/contracts/Safe.sol#L229

Vulnerability details

Impact

SubAccount operator can steal funds from the SubAccount Gnosis Safe via the Gnosis Safe gas refund mechanism, since the trusted validator signatures do not include the following parameters that are included in the Safe.sol execTransaction function: uint256 safeTxGas uint256 baseGas uint256 gasPrice address gasToken address payable refundReceiver This allows the operator to call execTransaction with these parameters set to send all the Ether or a chosen ERC-20 token held in the SubAccount Gnosis Safe to the operator's address.

Proof of Concept

1) One of the operators, who is an owner on the SubAccount Gnosis Safe, calls execTransaction on the Gnosis Safe with valid to, value, data, operation, and signatures parameters that is allowed by the current policy configured on the SubAccount, and therefore the trusted validator will also sign. However, the operator will also populate execTransaction function parameters related to gas refunds (safeTxGas, baseGas, gasPrice, gasToken, refundReceiver), to values that transfer to themselves Ether or ERC-20 tokens held in the SubAccount Gnosis Safe. 2) The transaction is executed, and in the handlePayment function call within the Gnosis Safe execution: https://github.com/safe-global/safe-contracts/blob/main/contracts/Safe.sol#L229 the Ether or ERC-20 tokens will be transferred to the address the operator specified in the refundReceiver parameter.

Tools Used

Manual review

Recommended Mitigation Steps

Include the safeTxGas, baseGas, gasPrice, gasToken, refundReceiver parameters of the transaction in the trusted validated signature that is verified: https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/PolicyValidator.sol#L54

Assessed type

Invalid Validation

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 primary issue

raymondfam commented 10 months ago

Dust amount entailed. Inadequate proof.

alex-ppg commented 10 months ago

The Warden has demonstrated a potential avenue via which the SafeModerator security checks in relation to the interaction performed can be "bypassed". Effectively, operators of the sub-account can extract any funds held by the sub-account by taking advantage of the gas refund mechanism.

Given that the price-per-gas unit can be arbitrarily specified, a minuscule waste of gas can result in a significant amount of funds being extracted. This particular avenue also bypasses the SafeModerator security checks.

As such, the Warden has showcased a way to actively affect funds within a sub-account that is not intended behaviour by the Sponsor. I will invite the Sponsor to weigh in on this particular issue, however, I am inclined to assign it either a "Medium" or a "High" severity.

c4-judge commented 10 months ago

alex-ppg marked the issue as satisfactory

c4-judge commented 10 months ago

alex-ppg changed the severity to 2 (Med Risk)

c4-judge commented 10 months ago

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

c4-judge commented 10 months ago

alex-ppg marked the issue as selected for report

0xad1onchain commented 9 months ago

Fixed, added missing params to transaction validation struct