M-01 Unmitigated

[c4-bot-5]

Lines of code

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

Vulnerability details

Issue Report

M-01: Balance check during MagicSpend validation cannot ensure that MagicSpend has enough balance to cover the requested fund.

Details

Issue#110

An issue was identified in MagicSpend contract where during the validation phase of multiple UserOperations, the contract's balance check does not account for the cumulative effect of multiple withdrawals being validated and executed in sequence. This can lead to a situation where the contract believes it has enough balance to cover all requests during validation but runs out of funds during the execution phase, due to not accounting for the total aggregate withdrawal amount requested by all operations.

Mitigation

1. PR#16
2. PR#17

In the first PR the core idea was to use a variable to keep track of the total amount being withdrawn during the validation phase of each UserOperation within a transaction bundle. This would help ensure that the sum of all withdrawals did not exceed the contract's available balance. By validating each withdrawal request against the tracked total withdrawal amount and the contract's balance, the PR aimed to preemptively reject withdrawal requests that would lead to an insufficient balance, thus preventing the entire transaction bundle from failing. This however turns out to be inefficient in the case of paymaster as reputional damage will have already been done by the multiple reverts introduced by this mitigation.

The second PR introduced a new strategy to address the issue by setting a maxWithdrawDenominator, which represents a more probabilistic approach to managing the risk of transaction reverts due to insufficient contract balance when multiple withdrawals occur within the same transaction bundle. By probabilistically limiting withdrawal amounts, the strategy reduces the likelihood of transaction reverts in bundles due to insufficient balance, which could negatively impact the Paymaster's reputation.

The issue#110 is not entirely resolved but the introduction of maxWithdrawDenominator allows for limiting the size of each withdrawal relative to the contract's balance. This approach is designed to probabilistically reduce the likelihood of reverts due to balance insufficiency when multiple withdrawals are processed in the same transaction.

Loc

Suggestion

Implement mechanisms for dynamically adjusting the maxWithdrawDenominator based on recent transaction history or other relevant metrics. This could help adapt to changing usage patterns without manual intervention.

Conclusion

Unmitigated/Partially Mitigated: The solution presented decreases the likelihood of transaction failures due to insufficient balance when multiple withdrawals are executed in a single transaction batch, but it does not eliminate the possibility entirely.

Assessed type

Invalid Validation

[McCoady]

The team commented themselves prior to the mitigation review that "It is of course known that this doesn't entirely solve the issue, and the efficacy depends the value chosen and usage." This finding just points out that a transaction batch could still fail if maxWithdrawDenominator is too low, but this is up to the project team to set to a suitable number.

Also, the suggestion to adjust maxWithdrawDenominator is already possible in the code here:

    function setMaxWithdrawDenominator(uint256 newDenominator) external onlyOwner {
        _setMaxWithdrawDenominator(newDenominator);
    }

The team are trusted to use this correctly and adjust the value should that be necessary.

[stevieraykatz]

Agree with @McCoady that we think we have addressed the original issue.

We have a hook to adjust the maxWithdrawDenominator. Additionally, since Coinbase will have to be involved in generating MagicSpend-sponsored transactions, we will have the information to both:

1. limit the number of concurrent withdraws
2. track the required balance on the contract to ensure just-in-time funding

[anthonyshiks]

I see your point but the suggestion I made was to eliminate manual intervention by any owner to set the maxWithdrawDenominator. As this value can still be set by compromised owner to malicious values that still enables the same issue. I have also demonstrated how owner can get compromised in this report #18. @McCoady mentions that the owner is trusted but this is news to me as it has not been explicitly mentioned in contest readme or mitigation readme..The reasoning still stands here as unmitigated since the path to attack vector is actually very feasible and likely.. if we stick with setting it manually then safeguards still need to be in place in case of compromised owner to not set it to values that enables this.

[McCoady]

The MagicSpend contract owner will be Coinbase itself, the owner discussed in #18 is the SmartWallet owner (a regular user).

[anthonyshiks]

Oh okay noted.

[anthonyshiks]

The MagicSpend contract owner will be Coinbase itself, the owner discussed in #18 is the SmartWallet owner (a regular user)

@McCoady Still not mentioned as trusted for MagicSpend in readme.

[3docSec]

The team deliberately made a tradeoff choice here. I agree that the solution is somewhat probabilistic, but the solution did effectively "mitigate" the problem.

[c4-judge]

3docSec marked the issue as nullified

[anthonyshiks]

The team clearly came to the same conclusion that this is partially mitigated. Is the glass half full or half empty?..still stand by unmitigated as issue is not clearly and effectively mitigated like the other reports. If it was a simple yes or no ..then it's no

Partially remediates code-423n4/2024-03-coinbase-findings#110

[3docSec]

The team transformed a medium-severity finding into a centralization finding ("these values set by admins may not cover particular situations"), so the glass is more than half full in my opinion.

If you would like to open an issue in the C4 org repo to have the community reflect on if this is enough to consider a finding mitigated, I'll be happy to have this point clarified.