Open c4-bot-1 opened 5 months ago
raymondfam marked the issue as sufficient quality report
raymondfam marked the issue as primary issue
Replayability would break indeed.
This is correct. A replayable transaction with a paymasterAndData field would need to ensure that this is valid across all chains. This is not specific to Magic Spend and not something we plan to mitigate.
wilsoncusack marked the issue as disagree with severity
wilsoncusack (sponsor) acknowledged
To be fair, I don't have enough information to evaluate the probability of rejection (that is, how many users request MagicSpend to sponsor replayable UserOperations over those who don't). In doubt, I consider this a valid Medium, because the claim is right and well-argued.
3docSec marked the issue as satisfactory
To be fair, I don't have enough information to evaluate the probability of rejection (that is, how many users request MagicSpend to sponsor replayable UserOperations over those who don't). In doubt, I consider this a valid Medium, because the claim is right and well-argued.
I think it is up to wallet clients to know the limitations of using paymasters with replayable transactions. This is not something we were trying to solve for.
3docSec changed the severity to QA (Quality Assurance)
3docSec marked the issue as grade-a
Hi @3docSec We thank you for your time reviewing this issue, however, we request that this issue be upgraded to an issue of high severity for the reasons below.
Firstly this issue has been reviewed and has been deemed of medium severity by the 2 reputable Judges and even the sponsor namely
However, with it not tallying to hypothetical plans, the sponsor currently disagreed with the severity which is fairly understandable but notwithstanding the issue still stands as valid as per code4rena issue categorization and the fix is good, based on the Readme, the documentation and the codebase given to the wardens as at the time of the contest.
It Is worth noting that as specified by c4 for any contest, The contest Readme and the documentation provided by the sponsors are the only source of truth for the wardens during the contest, and nowhere was this issue downplayed by the team, as seen in the report the readme and docs support this issue.
However, the argument we pose here concerns the severity of the issue, as after analyzing the code4rena severity categorization we have seen that this issue falls under the high severity label because: Code4rena Severity Categorization
The Report fulfills the above as:
Here you can see that this report highlights two separate issues concerning cross-chain replayability: 1) Due to hashing the WithdrawRequest with arguments including the chain-ID, such a transaction cannot be played on different chains, thereby breaking the replayability feature (MED RiSK). 2) Due to the nature of how WithdrawRequests are issued, the user takes more funds for gas from the magic spend contract on different chains more than what he/she paid for via their respective coin base account, thereby grieving funds from the entire protocol via the replayability feature(HIGH RISK).
For more context, take a look at the example in the main report:
Also, I would like to add one more thing concerning why this cross-chain replayability is broken
- In the Scenario where this same User-Ops will be executed on different chains perfectly.
An Issue arises as Users are deducted from their coin-base account the withdrawn amount needed for gas fees, however not knowing the number of chains the user will execute this transaction will lead to an issue.
Let's take these steps to understand:
- The user requests $30 for the withdrawal amount to be used as the gas cost of the user ops.
- coin-base deducts this balance from the user and gives them a WithdrawRequest to use $30 worth of ETH.
- Now remember the user intends on using this user ops on multiple chains.
- Supposing all chains require the same gas cost and the user executes this User operation on 5 chains permissionlessly.
- The total amount the user used of the different paymasters on different chains would be about $150 worth of ETH.
- Meanwhile, the user was only deducted $30 from his coin-base account Grieving the paymaster(MagicSpend.sol).
With the following reasons, we believe that this report is qualified for the high severity label.
We request this issue be reevaluated, and considered with sensitivity to be fair to the wardens who submitted the issue, by the honorable Judge. Thank you!
Hi,
next time, please use 2 reports to report two findings (replayable messages can't be replayed when sponsored via MagicSpend / replayed transactions sponsored via MagicSpend double-spend the sponsored amounts).
Putting these 2 root causes together actually explains why the sponsor does not want to allow replaying MagicSpend sponsored transactions. It makes perfect sense: if they allowed it, they would have suffered the high-impact finding.
To summarize, the high impact is not valid because MagicSpend-sponsored transactions are not replay-able, and the med impact of them not being replay-able is therefore not a bug but the way the sponsor took to avoid the high impact.
QA stands still here, I am now more confident of it 😅; speaking of, I had another judge confirm the downgrade to QA and they agreed with the choice.
hello, Good morning @3docSec We have noted the statement you made here, thank you for the advice.
next time, please use 2 reports to report two findings (replayable messages can't be replayed when sponsored via MagicSpend / replayed transactions sponsored via MagicSpend double-spend the sponsored amounts).
But we think there is confusion here,
Putting these 2 root causes together actually explains why the sponsor does not want to allow replaying MagicSpend sponsored transactions. It makes perfect sense: if they allowed it, they would have suffered the high-impact finding.
The cross-chain replayable feature was included in the docs, and also other bugs have been found around this feature in other reports that have already been deemed valid, The documentation and the code highlight the plans to use this feature.
It is stated that the only source of truth to a contest is the code and the documentation/readme, all these include this in them, and not anything said after a contest
To summarize, the high impact is not valid because MagicSpend-sponsored transactions are not replay-able, and the med impact of them not being replay-able is therefore not a bug but the way the sponsor took to avoid the high impact.
you said a high finding is not valid, however, the magic spend is the main core of the protocol on which they intend to launch on all chains, and it is used as paymaster for the users of the coin base protocol, every user ops depends on it as the paymaster however including it in the user ops will DOS their transaction to replay on all chains which as per c4 history is a valid medium at least, then you said the sponsor don't plan to use it on user based transaction but such was seen as to the understanding of the documentation, please reevaluate this finding sir. Thank you for your time.
However, the decision to downgrade by another Judge doesn't seem to align with the ideas behind this finding, as you previous stated it was a valid medium because you had a full context of the protocol at hand and you understood the issue at hand, another other eye might not grasp the entirety of the issue we've submitted. We do not mean to question your judgment, sir, we place this comment with full respect to you, only for the sake of our finding not being overlooked and to be judged with fairness in alignment with c4 rules.
You can rest assured, the finding is definitely not overlooked.
Funds are safe precisely because a MagicSpend approval can't be reused cross-chain, and your report helped prove it.
Isn't that the bug, as per the expected functionality? In the documentation this should be allowed(as it is a functionality of the protocol) but the report proves otherwise, the functionality here is broken.
The functionality is the reason why #114 is valid, but here we have shown that this functionality is broken due to the magic spend which is the core engine to the protocol and every user operation.
The sponsor made it very clear that MagicSpend
sponsoring replay-able transactions is NOT a functionality they planned to provide, nor there is any reasonable reason to believe it should.
You are free to source documentation that explicitly says otherwise, and we'll let the sponsors know they might want to fix the doc.
If a user changes an owner or upgrade their smart wallet, they likely want this change applied to all instances of your smart wallet, across various chains. Our smart wallet allows users to sign a single user operation which can be permissionlessly replayed on other chains.
The magic spend is the one that funds the user ops to be replayed on different chains permissionlessly
Please @3docSec take a look at this https://github.com/code-423n4/2024-03-coinbase/blob/main/src/MagicSpend/README.md#detailed-flows
ERC-4337 EntryPoint tx do not require a paymaster.
hi @McCoady You're right but in this context coin base intends on sponsoring all user ops Tx,
The point here is exempting the use of the magic spend, which in its absence the SCW would be like any other AA, but this protocol intends to make life easier for the user via the use of magic spend, but when used the replayability is DOSed
You're right but in this context coin base intends on sponsoring all user ops Tx,
The sponsor contradicted this statement with their comment:
This is correct. A replayable transaction with a paymasterAndData field would need to ensure that this is valid across all chains. This is not specific to Magic Spend and not something we plan to mitigate.
You made your point @shealtielanz , repeating it multiple times without new evidence won't make it more convincing; considering that you've been already warned you may want to review carefully the backstage guidelines.
Lines of code
https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/MagicSpend/MagicSpend.sol#L284
Vulnerability details
Impact
SCW Allows for Cross-chain replayability where users can replay a single user operation on all chains as per the ReadMe:
The issue is seen in MagicSpend.sol, cause while this is implemented correctly in the SCW Account by excluding the
chainID
from thehash
of the message to be validated:The functionality is still broken with the use of MagicSpend.sol as the paymaster, this is because the
hash
of the message to be signed by owner() contains thechainID
:Let me break this down:
Let's follow the steps to run down the User Ops origin:
Severity Guide
Also, I would like to add one more thing concerning why this cross-chain replayability is broken
An Issue arises as Users are deducted from their coin-base account the withdrawn amount needed for gas fees, however not knowing the number of chains the user will execute this transaction will lead to an issue.
Let's take these steps to understand: The user requests $30 for the withdrawal amount to be used as the gas cost of the user ops.
coin-base deducts this balance from the user and gives them a WithdrawRequest to use $30 worth of ETH.
Now remember the user intends on using this user ops on multiple chains.
Supposing all chains require the same gas cost and the user executes this User operation on 5 chains permissionlessly.
The total amount the user used of the different paymasters on different chains would be about $150 worth of ETH.
Meanwhile, the user was only deducted $30 from his coin-base account Grieving the paymaster(MagicSpend.sol).
Tools Used
Manual analysis.
Recommended Mitigation Steps
Assessed type
Invalid Validation