c4-bot-5 commented 5 months ago

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

UserOp op has unset gas fields.

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

Call eth_estimateUserOperationGas api of bundler with current op.
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
```
However, different gas values results in a different userOpHash hash2.
- https://github.com/eth-infinitism/account-abstraction/blob/abff2aca61a8f0934e533d0d352978055fddbd96/contracts/interfaces/UserOperation.sol#L78
- https://github.com/eth-infinitism/account-abstraction/blob/abff2aca61a8f0934e533d0d352978055fddbd96/contracts/interfaces/UserOperation.sol#L84-L86
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.
verificationGasLimit of userOp will be set to a lesser verificationGasLimit from gas estimation.
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-pre-sort commented 5 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as primary issue

raymondfam commented 5 months ago

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

wilsoncusack commented 5 months ago

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 commented 5 months ago

wilsoncusack (sponsor) disputed

3docSec commented 5 months ago

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 commented 5 months ago

3docSec changed the severity to QA (Quality Assurance)