User Funds at Risk of Lockup Due to Assertion Failure in `postOp` Function

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L143-L163

Vulnerability details

Impact

If an operation fails legitimately and encounters postOpReverted, the assertion failure will prevent the remaining funds from being transferred back to the user. This could result in a situation where user funds become locked up in the contract, inaccessible to the user and unable to be recovered without manual intervention.

Proof of Concept

The vulnerability in the postOp function of the contract lies in its reliance on an assertion that PostOpMode.postOpReverted should never occur. Let's delve into the specifics:

1. Assumption of Impossibility: The contract includes the following assertion:

   assert(mode != PostOpMode.postOpReverted);

   This assertion assumes that encountering PostOpMode.postOpReverted is impossible and should never happen during contract execution. It implies that any occurrence of postOpReverted would indicate an unexpected and erroneous state in the contract.

2. Limited Error Handling:

• Instead of implementing comprehensive error handling mechanisms, the contract relies solely on this assertion to handle unexpected scenarios.

• The assertion effectively serves as a guard against encountering postOpReverted, with the assumption that such a situation should never arise.

• However, this approach lacks flexibility and robustness as it fails to account for legitimate reasons why an operation might legitimately fail, such as incorrect user input or contract-specific conditions.

  3. Potential for Realistic Scenarios:

• In practice, there are various legitimate reasons why an operation might fail. For instance, if a user provides invalid input data or if a contract condition is not met, the operation could rightfully revert.

• In such cases, encountering postOpReverted is not indicative of a contract bug or an unexpected scenario; rather, it represents a legitimate operation failure.

• However, due to the reliance on the assertion, legitimate failures are treated as unexpected, leading to an assertion failure and potentially adverse consequences.

  Tools Used

  Manual

  Recommended Mitigation Steps

  Implement proper error handling mechanisms to handle scenarios where the operation fails. Instead of relying solely on assertions, the function should include conditional checks to handle different post-operation modes appropriately. If the mode indicates failure due to legitimate reasons, the contract should revert the transaction and provide mechanisms for users to recover their funds.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

This is part of the calls supported through bundler + entrypoint. So it's not going to happen as you described in the user call.

[3docSec]

Users seeing their UserOperation revert because they provided invalid inputs does not seem an impactful scenario, without a clear explanation of what the potentially adverse consequences could be.

[c4-judge]

3docSec marked the issue as unsatisfactory: Insufficient proof