Cross Chain Replayability will be broken for Smart Accounts using MagicSpend.sol as their PayMaster.

[c4-bot-1]

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:

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 issue is seen in MagicSpend.sol, cause while this is implemented correctly in the SCW Account by excluding the chainID from the hash of the message to be validated:

    function getUserOpHashWithoutChainId(UserOperation calldata userOp)
        public
        view
        virtual
        returns (bytes32 userOpHash)
    {
        return keccak256(abi.encode(UserOperationLib.hash(userOp), entryPoint()));
    }

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 the chainID:

   /// @return The hash to be signed for the given `account` and `withdrawRequest`.
    function getHash(address account, WithdrawRequest memory withdrawRequest) public view returns (bytes32) {
        return SignatureCheckerLib.toEthSignedMessageHash(
            abi.encode(
                address(this), //@audit-issue uses address this to avoid cross contract replayabilty.
                account,
                block.chainid, //@audit-issue cross chain replayability is broken cause of this.
                withdrawRequest.asset,
                withdrawRequest.amount,
                withdrawRequest.nonce,
                withdrawRequest.expiry 
            )
        );
    }

Let me break this down:

• User Ops contains the paymasterData.
• paymasterData contains the withdrawal request.
• withdrawal request contains the signature
• Signature is a mixture of the owner's private key & the hash of the message.
• the hash of the message to be signed to get the signature contains the chainID.

Let's follow the steps to run down the User Ops origin:

• The user gets a withdrawal request with a signature signed by the owner of MagicSpend.sol(Withdrawal request hash signed by the owner).
• The WithdrawRequest.signature is signed with the owner's private key on the hash of the WithdrawRequest struct parameters including the current block-chainID.
• This withdrawal request is appended to the User Ops Struct.
• User specifies the wish to execute the same User Ops on all chains that have their SCW(by including the cross-chain selector).
• On the call to validateUserOps() by The EntryPoint the validation will pass.
• The EntryPoint sees they've also included MagicSpend.sol as their paymaster so it calls validatePaymasterUserOps() on MagicSpend.sol.
• MagicSpend.sol verifies the signature and it passes.
• The EntryPoint acknowledges this and continues to execute the User Operation.
• Now The User submits the same User ops to another chain to make use of the cross-chain replayability, after validating the User ops on the SCW, on the call to validate the paymaster Ops will always revert on this different chain because the block-chainID that was included in the signature of the withdrawal request is different from the current block-chainID.

Severity Guide

• The cross-chain replayability functionality is broken as the same User Ops cannot be executed on the different chains using MagicSpender.sol.

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).

Tools Used

Manual analysis.

Recommended Mitigation Steps

• In MagicSpend.sol, check if the user intends to replay their transaction in different chains and exclude the block-ChainID from the hash to be signed.
• Concerning the scenario where things worked perfectly concerning creating a mechanism to avoid that.
  OR
• Do a mitigation review after implementing the fixes of this current audit.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Replayability would break indeed.

[wilsoncusack]

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.

[c4-sponsor]

wilsoncusack marked the issue as disagree with severity

[c4-sponsor]

wilsoncusack (sponsor) acknowledged

[3docSec]

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.

[c4-judge]

3docSec marked the issue as satisfactory

[wilsoncusack]

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.

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-a

[shealtielanz]

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

• @raymondfam
• @3docSec
• @wilsoncusack

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

• 3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).
• 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

The Report fulfills the above as:

• It shows that a critical functionality of the protocol is broken.
• It shows that there is actual loss of funds to the protocol.

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!

[3docSec]

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.

[shealtielanz]

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.

[shealtielanz]

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.

[3docSec]

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.

[shealtielanz]

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.

[shealtielanz]

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.

[3docSec]

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.

[shealtielanz]

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/README.md#cross-chain-replayability

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

[shealtielanz]

Please @3docSec
take a look at this
https://github.com/code-423n4/2024-03-coinbase/blob/main/src/MagicSpend/README.md#detailed-flows

[McCoady]

ERC-4337 EntryPoint tx do not require a paymaster.

[shealtielanz]

hi @McCoady
You're right but in this context coin base intends on sponsoring all user ops Tx,

[shealtielanz]

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

[3docSec]

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.