Coinbase Smart Wallet - shealtielanz's results

Smart Wallet from Coinbase Wallet

General Information

Platform: Code4rena

Start Date: 14/03/2024

Pot Size: $49,000 USDC

Total HM: 3

Participants: 51

Period: 7 days

Judge: 3docSec

Id: 350

League: ETH

Coinbase

Findings Distribution

Researcher Performance

Rank: 22/51

Findings: 1

Award: $36.34

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Coinbase

Findings Information

🌟 Selected for report: 0xmystery

Also found by: 0xbrett8571, 0xhacksmithh, 7ashraf, Bigsam, Circolors, IceBear, Jorgect, Koala, Limbooo, SBSecurity, Tigerfrake, ZanyBonzy, aycozynfada, cheatc0d3, cryptphi, d3e4, doublespending, foxb868, gpersoon, imare, jesjupyter, lsaudit, robriks, shealtielanz, y4y

Awards

36.3397 USDC - $36.34

Labels

bug

disagree with severity

downgraded by judge

grade-a

primary issue

QA (Quality Assurance)

satisfactory

sponsor acknowledged

sufficient quality report

edited-by-warden

:robot:_09_group

Q-13

External Links

Link to GitHub issue

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

#0 - c4-pre-sort
2024-03-21T23:59:35Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort
2024-03-21T23:59:39Z

raymondfam marked the issue as primary issue

#2 - raymondfam
2024-03-22T00:00:30Z

Replayability would break indeed.

#3 - wilsoncusack
2024-03-26T13:57:46Z

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.

#4 - c4-sponsor
2024-03-26T13:57:53Z

wilsoncusack marked the issue as disagree with severity

#5 - c4-sponsor
2024-03-26T13:57:56Z

wilsoncusack (sponsor) acknowledged

#6 - 3docSec
2024-03-27T07:42:56Z

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.

#7 - c4-judge
2024-03-27T07:43:05Z

3docSec marked the issue as satisfactory

#8 - wilsoncusack
2024-03-27T13:15:07Z

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.

#9 - c4-judge
2024-03-27T13:51:46Z

3docSec changed the severity to QA (Quality Assurance)

#10 - c4-judge
2024-03-27T13:51:54Z

3docSec marked the issue as grade-a

#11 - shealtielanz
2024-03-27T22:10:19Z

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!

#12 - 3docSec
2024-03-28T05:39:24Z

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.

#13 - shealtielanz
2024-03-28T07:17:29Z

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.

#14 - shealtielanz
2024-03-28T07:22:12Z

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.

#15 - 3docSec
2024-03-28T07:40:35Z

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.

#16 - shealtielanz
2024-03-28T07:44:40Z

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.

#17 - shealtielanz
2024-03-28T07:49:43Z

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.

#18 - 3docSec
2024-03-28T08:06:13Z

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.

#19 - shealtielanz
2024-03-28T08:13:03Z

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

#20 - shealtielanz
2024-03-28T08:15:29Z

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

#21 - McCoady
2024-03-28T08:17:15Z

ERC-4337 EntryPoint tx do not require a paymaster.

#22 - shealtielanz
2024-03-28T08:18:59Z

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

#23 - shealtielanz
2024-03-28T08:27:00Z

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

#24 - 3docSec
2024-03-28T08:37:24Z

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.

Coinbase Smart Wallet - shealtielanz's results

Smart Wallet from Coinbase Wallet

General Information

Findings Distribution

Researcher Performance

External Links

[Q-13] QA Report - $36.34

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2024-03-21T23:59:35Z

#1 - c4-pre-sort2024-03-21T23:59:39Z

#2 - raymondfam2024-03-22T00:00:30Z

#3 - wilsoncusack2024-03-26T13:57:46Z

#4 - c4-sponsor2024-03-26T13:57:53Z

#5 - c4-sponsor2024-03-26T13:57:56Z

#6 - 3docSec2024-03-27T07:42:56Z

#7 - c4-judge2024-03-27T07:43:05Z

#8 - wilsoncusack2024-03-27T13:15:07Z

#9 - c4-judge2024-03-27T13:51:46Z

#10 - c4-judge2024-03-27T13:51:54Z

#11 - shealtielanz2024-03-27T22:10:19Z

#12 - 3docSec2024-03-28T05:39:24Z

#13 - shealtielanz2024-03-28T07:17:29Z

#14 - shealtielanz2024-03-28T07:22:12Z

#15 - 3docSec2024-03-28T07:40:35Z

#16 - shealtielanz2024-03-28T07:44:40Z

#17 - shealtielanz2024-03-28T07:49:43Z

#18 - 3docSec2024-03-28T08:06:13Z

#19 - shealtielanz2024-03-28T08:13:03Z

#20 - shealtielanz2024-03-28T08:15:29Z

#21 - McCoady2024-03-28T08:17:15Z

#22 - shealtielanz2024-03-28T08:18:59Z

#23 - shealtielanz2024-03-28T08:27:00Z

#24 - 3docSec2024-03-28T08:37:24Z

#0 - c4-pre-sort
2024-03-21T23:59:35Z

#1 - c4-pre-sort
2024-03-21T23:59:39Z

#2 - raymondfam
2024-03-22T00:00:30Z

#3 - wilsoncusack
2024-03-26T13:57:46Z

#4 - c4-sponsor
2024-03-26T13:57:53Z

#5 - c4-sponsor
2024-03-26T13:57:56Z

#6 - 3docSec
2024-03-27T07:42:56Z

#7 - c4-judge
2024-03-27T07:43:05Z

#8 - wilsoncusack
2024-03-27T13:15:07Z

#9 - c4-judge
2024-03-27T13:51:46Z

#10 - c4-judge
2024-03-27T13:51:54Z

#11 - shealtielanz
2024-03-27T22:10:19Z

#12 - 3docSec
2024-03-28T05:39:24Z

#13 - shealtielanz
2024-03-28T07:17:29Z

#14 - shealtielanz
2024-03-28T07:22:12Z

#15 - 3docSec
2024-03-28T07:40:35Z

#16 - shealtielanz
2024-03-28T07:44:40Z

#17 - shealtielanz
2024-03-28T07:49:43Z

#18 - 3docSec
2024-03-28T08:06:13Z

#19 - shealtielanz
2024-03-28T08:13:03Z

#20 - shealtielanz
2024-03-28T08:15:29Z

#21 - McCoady
2024-03-28T08:17:15Z

#22 - shealtielanz
2024-03-28T08:18:59Z

#23 - shealtielanz
2024-03-28T08:27:00Z

#24 - 3docSec
2024-03-28T08:37:24Z