-
Notifications
You must be signed in to change notification settings - Fork 124
Fix early revert in verify for accurate gas estimation
#70
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
base: main
Are you sure you want to change the base?
Changes from all commits
4a711a6
9d2776a
f2cb9e8
13cc1a7
78d74ed
c97c9ed
4f17a2a
5a06e86
03e0711
89eddf6
151a415
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ETHEREUM_RPC= | ||
| BASE_RPC= | ||
| ARBITRUM_RPC= |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,14 @@ | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenSAboveP256_N_DIV_2() (gas: 429635068) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenTheUpFlagIsNotSet() (gas: 435310864) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenUserVerifictionIsRequiredButTestWasNotPerformed() (gas: 432573571) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnTrue_WhenSBelowP256_N_DIV_2() (gas: 456301488) | ||
| WebAuthnTest:test_chrome() (gas: 225641) | ||
| WebAuthnTest:test_safari() (gas: 221888) | ||
| WebAuthnForkTest:test_verify_fullPath_arb() (gas: 254948) | ||
| WebAuthnForkTest:test_verify_fullPath_base() (gas: 254970) | ||
| WebAuthnForkTest:test_verify_fullPath_eth() (gas: 254971) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_arb() (gas: 244890) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_base() (gas: 244913) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_eth() (gas: 244912) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenSAboveP256_N_DIV_2() (gas: 456140535) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenTheUpFlagIsNotSet() (gas: 456268829) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenUserVerifictionIsRequiredButTestWasNotPerformed() (gas: 444300014) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnTrue_WhenSBelowP256_N_DIV_2() (gas: 456260565) | ||
| WebAuthnGasBenchmarks:test_verify_invalid_challenge_path() (gas: 239948) | ||
| WebAuthnGasBenchmarks:test_verify_valid_signature_path() (gas: 249982) | ||
| WebAuthnTest:test_chrome() (gas: 225617) | ||
| WebAuthnTest:test_safari() (gas: 221864) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| WebAuthnForkTest:test_verify_fullPath_arb() (gas: 253734) | ||
| WebAuthnForkTest:test_verify_fullPath_base() (gas: 253756) | ||
| WebAuthnForkTest:test_verify_fullPath_eth() (gas: 253757) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_arb() (gas: 43717) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_base() (gas: 43740) | ||
| WebAuthnForkTest:test_verify_invalidChallenge_eth() (gas: 43739) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenSAboveP256_N_DIV_2() (gas: 24590028) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenTheUpFlagIsNotSet() (gas: 30126008) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnFalse_WhenUserVerifictionIsRequiredButTestWasNotPerformed() (gas: 27464255) | ||
| WebAuthnFuzzTest:test_Verify_ShoulReturnTrue_WhenSBelowP256_N_DIV_2() (gas: 50886607) | ||
| WebAuthnGasBenchmarks:test_verify_invalid_challenge_path() (gas: 38778) | ||
| WebAuthnGasBenchmarks:test_verify_valid_signature_path() (gas: 248768) | ||
| WebAuthnTest:test_chrome() (gas: 224261) | ||
| WebAuthnTest:test_safari() (gas: 220583) | ||
amiecorso marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,7 +47,7 @@ library WebAuthn { | |
| /// @dev Secp256r1 curve order / 2 used as guard to prevent signature malleability issue. | ||
| uint256 private constant _P256_N_DIV_2 = FCL_Elliptic_ZZ.n / 2; | ||
|
|
||
| /// @dev The precompiled contract address to use for signature verification in the “secp256r1” elliptic curve. | ||
| /// @dev The precompiled contract address to use for signature verification in the "secp256r1" elliptic curve. | ||
| /// See https://github.com/ethereum/RIPs/blob/master/RIPS/rip-7212.md. | ||
| address private constant _VERIFIER = address(0x100); | ||
|
|
||
|
|
@@ -95,49 +95,50 @@ library WebAuthn { | |
| /// - Does NOT verify the attestation object: this assumes that response.attestationObject is NOT present in the response, | ||
| /// i.e. the RP does not intend to verify an attestation. | ||
| /// | ||
| /// @param challenge The challenge that was provided by the relying party. | ||
| /// @param requireUV A boolean indicating whether user verification is required. | ||
| /// @param challenge The challenge that was provided by the relying party. | ||
| /// @param requireUV A boolean indicating whether user verification is required. | ||
| /// @param webAuthnAuth The `WebAuthnAuth` struct. | ||
| /// @param x The x coordinate of the public key. | ||
| /// @param y The y coordinate of the public key. | ||
| /// @param x The x coordinate of the public key. | ||
| /// @param y The y coordinate of the public key. | ||
| /// | ||
| /// @return `true` if the authentication assertion passed validation, else `false`. | ||
| function verify(bytes memory challenge, bool requireUV, WebAuthnAuth memory webAuthnAuth, uint256 x, uint256 y) | ||
| internal | ||
| view | ||
| returns (bool) | ||
| { | ||
| bool hasFailedChecks = false; | ||
|
Comment on lines
108
to
+110
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Could use
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the actual return is an expression that incorporates multiple booleans |
||
| if (webAuthnAuth.s > _P256_N_DIV_2) { | ||
| // guard against signature malleability | ||
| return false; | ||
| hasFailedChecks = true; | ||
| } | ||
|
|
||
| // 11. Verify that the value of C.type is the string webauthn.get. | ||
| // bytes("type":"webauthn.get").length = 21 | ||
| // bytes("type":"webauthn.get").length = 21 | ||
| string memory _type = webAuthnAuth.clientDataJSON.slice(webAuthnAuth.typeIndex, webAuthnAuth.typeIndex + 21); | ||
| if (keccak256(bytes(_type)) != _EXPECTED_TYPE_HASH) { | ||
| return false; | ||
| hasFailedChecks = true; | ||
| } | ||
|
|
||
| // 12. Verify that the value of C.challenge equals the base64url encoding of options.challenge. | ||
| bytes memory expectedChallenge = bytes(string.concat('"challenge":"', Base64.encodeURL(challenge), '"')); | ||
| string memory actualChallenge = | ||
| webAuthnAuth.clientDataJSON.slice(webAuthnAuth.challengeIndex, webAuthnAuth.challengeIndex + expectedChallenge.length); | ||
| if (keccak256(bytes(actualChallenge)) != keccak256(expectedChallenge)) { | ||
| return false; | ||
| hasFailedChecks = true; | ||
| } | ||
|
|
||
| // Skip 13., 14., 15. | ||
|
|
||
| // 16. Verify that the UP bit of the flags in authData is set. | ||
| if (webAuthnAuth.authenticatorData[32] & _AUTH_DATA_FLAGS_UP != _AUTH_DATA_FLAGS_UP) { | ||
| return false; | ||
| hasFailedChecks = true; | ||
| } | ||
|
|
||
| // 17. If user verification is required for this assertion, verify that the User Verified bit of the flags in | ||
| // authData is set. | ||
| // authData is set. | ||
| if (requireUV && (webAuthnAuth.authenticatorData[32] & _AUTH_DATA_FLAGS_UV) != _AUTH_DATA_FLAGS_UV) { | ||
| return false; | ||
| hasFailedChecks = true; | ||
| } | ||
|
|
||
| // skip 18. | ||
|
|
@@ -146,19 +147,29 @@ library WebAuthn { | |
| bytes32 clientDataJSONHash = sha256(bytes(webAuthnAuth.clientDataJSON)); | ||
|
|
||
| // 20. Using credentialPublicKey, verify that sig is a valid signature over the binary concatenation of authData | ||
| // and hash. | ||
| // and hash. | ||
| bytes32 messageHash = sha256(abi.encodePacked(webAuthnAuth.authenticatorData, clientDataJSONHash)); | ||
| bytes memory args = abi.encode(messageHash, webAuthnAuth.r, webAuthnAuth.s, x, y); | ||
| // try the RIP-7212 precompile address | ||
| (bool success, bytes memory ret) = _VERIFIER.staticcall(args); | ||
| bool sigValid = _verifySigP256(messageHash, webAuthnAuth.r, webAuthnAuth.s, x, y); | ||
| return !hasFailedChecks && sigValid; | ||
| } | ||
|
|
||
| /// @dev Verifies a P256 signature using the precompiled contract or FCL. | ||
| /// @param messageHash The hash of the message to verify. | ||
| /// @param r The r value of the signature. | ||
| /// @param s The s value of the signature. | ||
| /// @param x The x coordinate of the public key. | ||
| /// @param y The y coordinate of the public key. | ||
| /// @return True if the signature is valid, false otherwise. | ||
|
Comment on lines
+156
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: separate tag types with empty line |
||
| function _verifySigP256(bytes32 messageHash, uint256 r, uint256 s, uint256 x, uint256 y) private view returns (bool) { | ||
amiecorso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // staticcall will not revert if address has no code | ||
| // check return length | ||
| // note that even if precompile exists, ret.length is 0 when verification returns false | ||
| // so an invalid signature will be checked twice: once by the precompile and once by FCL. | ||
| // Ideally this signature failure is simulated offchain and no one actually pay this gas. | ||
| bool valid = ret.length > 0; | ||
| if (success && valid) return abi.decode(ret, (uint256)) == 1; | ||
|
|
||
| return FCL_ecdsa.ecdsa_verify(messageHash, webAuthnAuth.r, webAuthnAuth.s, x, y); | ||
| // Ideally this signature failure is simulated offchain and no one actually pays this gas. | ||
| (bool success, bytes memory ret) = _VERIFIER.staticcall(abi.encode(messageHash, r, s, x, y)); | ||
| if (success && ret.length > 0) { | ||
| return abi.decode(ret, (uint256)) == 1; | ||
| } | ||
| return FCL_ecdsa.ecdsa_verify(messageHash, r, s, x, y); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import {WebAuthn} from "../src/WebAuthn.sol"; | ||
| import {Base64Url} from "FreshCryptoLib/utils/Base64Url.sol"; | ||
| import {Test} from "forge-std/Test.sol"; | ||
|
|
||
| abstract contract BaseWebAuthnTest is Test { | ||
| // Fixed public key for tests | ||
| uint256 internal constant PUBKEY_X = 28573233055232466711029625910063034642429572463461595413086259353299906450061; | ||
| uint256 internal constant PUBKEY_Y = 39367742072897599771788408398752356480431855827262528811857788332151452825281; | ||
|
|
||
| // Fixed indices and auth data | ||
| uint256 internal constant CHALLENGE_INDEX = 23; | ||
| uint256 internal constant TYPE_INDEX = 1; | ||
| bytes internal constant AUTH_DATA = hex"49960de5880e8c687434170f6476605b8fe4aeb9a28632c7995cf3ba831d9763050000010a"; | ||
|
|
||
| // Signature scalars | ||
| uint256 internal constant SIG_R = 29739767516584490820047863506833955097567272713519339793744591468032609909569; | ||
| uint256 internal constant SIG_S = 45947455641742997809691064512762075989493430661170736817032030660832793108102; | ||
|
|
||
| // Default challenge | ||
| bytes internal defaultChallenge; | ||
|
|
||
| // Prebuilt auth fixtures shared by inheritors | ||
| WebAuthn.WebAuthnAuth internal validAuth; | ||
| WebAuthn.WebAuthnAuth internal wrongChallengeAuth; | ||
|
|
||
| function setUp() public virtual { | ||
| defaultChallenge = abi.encode(0xf631058a3ba1116acce12396fad0a125b5041c43f8e15723709f81aa8d5f4ccf); | ||
| validAuth = buildWebAuthnAuth(defaultChallenge); | ||
| wrongChallengeAuth = buildWebAuthnAuth(abi.encode(0xdeadbeef)); | ||
| } | ||
|
|
||
| function buildWebAuthnAuth(bytes memory challenge) internal pure returns (WebAuthn.WebAuthnAuth memory auth) { | ||
| auth = WebAuthn.WebAuthnAuth({ | ||
| authenticatorData: AUTH_DATA, | ||
| clientDataJSON: string.concat( | ||
| '{"type":"webauthn.get","challenge":"', | ||
| Base64Url.encode(challenge), | ||
| '","origin":"http://localhost:3005","crossOrigin":false}' | ||
| ), | ||
| challengeIndex: CHALLENGE_INDEX, | ||
| typeIndex: TYPE_INDEX, | ||
| r: SIG_R, | ||
| s: SIG_S | ||
| }); | ||
| } | ||
| } |
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the motivation for fork testing on arb, base and eth specifically? How were these networks chosen?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. originally this was to get a sense of the gas difference between the early-return v.s. the heavy path on chains that do/don't implement RIP-7212 (precompile for secp256r1 ec i.e. passkey signatures). Ethereum mainnet does not implement this, but Base and Arbitrum should (among other rollups). we're not actually able to see a difference in fork testing, so I'm not sure if foundry's fork testing environment incorporates precompiles. Would be open to removing arbitrum to make it somewhat less arbitrary (i.e. one 7212-compatible chain and one non-7212 chain) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Think this would be nice for clarity |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import {WebAuthn} from "../src/WebAuthn.sol"; | ||
| import {BaseWebAuthnTest} from "./BaseWebAuthnTest.t.sol"; | ||
| import {Test} from "forge-std/Test.sol"; | ||
|
|
||
| contract WebAuthnForkTest is BaseWebAuthnTest { | ||
| uint256 private baseFork; | ||
| uint256 private ethFork; | ||
| uint256 private arbFork; | ||
|
|
||
| function setUp() public override { | ||
| string memory baseRpc = vm.envString("BASE_RPC"); | ||
| string memory ethRpc = vm.envString("ETHEREUM_RPC"); | ||
| string memory arbRpc = vm.envString("ARBITRUM_RPC"); | ||
| baseFork = vm.createFork(baseRpc); | ||
| ethFork = vm.createFork(ethRpc); | ||
| arbFork = vm.createFork(arbRpc); | ||
|
|
||
| super.setUp(); | ||
| } | ||
|
|
||
| function test_verify_fullPath_base() public { | ||
| vm.selectFork(baseFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, validAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertTrue(result, "Valid auth should pass verification on Base"); | ||
| } | ||
|
|
||
| function test_verify_fullPath_eth() public { | ||
| vm.selectFork(ethFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, validAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertTrue(result, "Valid auth should pass verification on Ethereum"); | ||
| } | ||
|
|
||
| function test_verify_fullPath_arb() public { | ||
| vm.selectFork(arbFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, validAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertTrue(result, "Valid auth should pass verification on Arbitrum"); | ||
| } | ||
|
|
||
| function test_verify_invalidChallenge_base() public { | ||
| vm.selectFork(baseFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, wrongChallengeAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertFalse(result, "Wrong challenge should fail on Base"); | ||
| } | ||
|
|
||
| function test_verify_invalidChallenge_eth() public { | ||
| vm.selectFork(ethFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, wrongChallengeAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertFalse(result, "Wrong challenge should fail on Ethereum"); | ||
| } | ||
|
|
||
| function test_verify_invalidChallenge_arb() public { | ||
| vm.selectFork(arbFork); | ||
| bool result = WebAuthn.verify(defaultChallenge, false, wrongChallengeAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertFalse(result, "Wrong challenge should fail on Arbitrum"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.0; | ||
|
|
||
| import {WebAuthn} from "../src/WebAuthn.sol"; | ||
| import {BaseWebAuthnTest} from "./BaseWebAuthnTest.t.sol"; | ||
| import {Test} from "forge-std/Test.sol"; | ||
|
|
||
| contract WebAuthnGasBenchmarks is BaseWebAuthnTest { | ||
| function test_verify_valid_signature_path() public { | ||
| bool result = WebAuthn.verify(defaultChallenge, false, validAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertTrue(result, "Valid auth should pass verification"); | ||
| } | ||
|
|
||
| function test_verify_invalid_challenge_path() public { | ||
| bool result = WebAuthn.verify(defaultChallenge, false, wrongChallengeAuth, PUBKEY_X, PUBKEY_Y); | ||
| assertFalse(result, "Auth with wrong challenge should fail verification"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow crazy difference 😮