Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The userOps using webauthn must get lesser verificationGasLimit during gas estimation #108

Open
c4-bot-5 opened this issue Mar 21, 2024 · 8 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates Q-12 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@c4-bot-5
Copy link
Contributor

c4-bot-5 commented Mar 21, 2024

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/main/src/SmartWallet/CoinbaseSmartWallet.sol#L321
https://github.com/code-423n4/2024-03-coinbase/blob/main/src/WebAuthnSol/WebAuthn.sol#L122-L128

Vulnerability details

Impact

The userOps using webauthn must get lesser verificationGasLimit when calling eth_estimateUserOperationGas api to estimate the gas values for a UserOperation.

With lesser verificationGasLimit, those userOps will be rejected by bundlers.

Proof of Concept

  1. UserOp op has unset gas fields.
  2. Signature of op will be set to a dummy webauthn signature where current userOpHash hash1 is encoded as challenge of webAuthn.
bytes32 hash1 = 0x15fa6f8c855db1dccbb8a42eef3a7b83f11d29758e84aed37312527165d5eec5;
bytes32 challenge = account.replaySafeHash(hash);
WebAuthnInfo memory webAuthn = Utils.getWebAuthnStruct(challenge);
  1. Call eth_estimateUserOperationGas api of bundler with current op.
  2. Gas fields will be set to different gas values by the bundler to find the best gas settings.
op.verificationGasLimit = some_value_bundler_want_to_try
  1. However, different gas values results in a different userOpHash hash2.
  1. For hash1 is not equal to hash2, the gas estimation will exit early here and result in a estimated verificationGasLimit which does not cover the following checks.
  • UserOp validation goes into webauthn validation from here and exits here.
  • Those logics have not been walked through while gas estimation. However, those logics will be walked through while userOp execution.
  • It should exist here if we want to get the correct verificationGasLimit.
  1. verificationGasLimit of userOp will be set to a lesser verificationGasLimit from gas estimation.
  2. op with a lesser verificationGasLimit will be rejected by bundlers.

Relate info about this findings

Recommended Mitigation Steps

  • Do not encode userOpHash to userOp.signature.

Assessed type

Other

@c4-bot-5 c4-bot-5 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Mar 21, 2024
c4-bot-10 added a commit that referenced this issue Mar 21, 2024
@c4-bot-8 c4-bot-8 changed the title Mismatch webauthn challenge causes usersOp gas estimation to underpredict the gas limit which will cause the userOp to fail The userOps using webauthn must get lesser verificationGasLimit during gas estimation Mar 21, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 22, 2024
@c4-pre-sort
Copy link

raymondfam marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Mar 22, 2024
@raymondfam
Copy link

Will let the sponsor review it but the mitigation step seems infeasible.

@wilsoncusack
Copy link

This is a general problem with eth_estimateUserOperationGas. You want to construct a dummy signature that will match as closely as possible to the real signature, so that validation proceeds and you can get an accurate gas estimation. We have been using this for months with no issue.

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Mar 26, 2024
@c4-sponsor
Copy link

wilsoncusack (sponsor) disputed

@3docSec
Copy link

3docSec commented Mar 27, 2024

It is common practice to submit regular transactions with a gas limit that includes a margin on top of what's simulated, or at least re-simulating the final transaction before sending it. Worth keeping this as QA though.

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Mar 27, 2024
@c4-judge
Copy link
Contributor

3docSec changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Mar 27, 2024
@c4-judge
Copy link
Contributor

3docSec marked the issue as grade-a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-a primary issue Highest quality submission among a set of duplicates Q-12 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

9 participants