Merge branch 'main' into duplicat-match-sig

src/AAFactory.sol

@@ -19,6 +19,7 @@ contract AAFactory {
 
   /// @dev The bytecode hash of the beacon proxy, used for deploying proxy accounts.
   bytes32 public immutable beaconProxyBytecodeHash;
+  /// @dev The address of the SsoBeacon contract used for the SSO accounts' beacon proxies.
   address public immutable beacon;
 
   /// @notice A mapping from unique account IDs to their corresponding deployed account addresses.
@@ -65,7 +66,7 @@ contract AAFactory {
 
     accountMappings[_uniqueAccountId] = accountAddress;
 
-    // Initialize the newly deployed account with validators, hooks and K1 owners.
+    // Initialize the newly deployed account with validators and K1 owners.
     ISsoAccount(accountAddress).initialize(_initialValidators, _initialK1Owners);
 
     emit AccountCreated(accountAddress, _uniqueAccountId);

src/SsoAccount.sol

@@ -200,9 +200,9 @@ contract SsoAccount is
     }
 
     if (_transaction.signature.length == 65) {
-      (address signer, ECDSA.RecoverError error) = ECDSA.tryRecover(_signedHash, _transaction.signature);
+      (address signer, ECDSA.RecoverError err) = ECDSA.tryRecover(_signedHash, _transaction.signature);
       return
-        signer == address(0) || error != ECDSA.RecoverError.NoError || !_k1IsOwner(signer)
+        signer == address(0) || err != ECDSA.RecoverError.NoError || !_k1IsOwner(signer)
           ? bytes4(0)
           : ACCOUNT_VALIDATION_SUCCESS_MAGIC;
     }

src/handlers/ERC1271Handler.sol

@@ -31,9 +31,9 @@ abstract contract ERC1271Handler is IERC1271Upgradeable, EIP712("Sso1271", "1.0.
    */
   function isValidSignature(bytes32 hash, bytes memory signature) external view override returns (bytes4 magicValue) {
     if (signature.length == 65) {
-      (address signer, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature);
+      (address signer, ECDSA.RecoverError err) = ECDSA.tryRecover(hash, signature);
       return
-        signer == address(0) || error != ECDSA.RecoverError.NoError || !_k1IsOwner(signer) ? bytes4(0) : _ERC1271_MAGIC;
+        signer == address(0) || err != ECDSA.RecoverError.NoError || !_k1IsOwner(signer) ? bytes4(0) : _ERC1271_MAGIC;
     }
 
     (bytes memory decodedSignature, address validator) = SignatureDecoder.decodeSignatureNoHookData(signature);

src/interfaces/IHook.sol

@@ -6,7 +6,7 @@ import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol
 import { IModule } from "./IModule.sol";
 
 /// @title Validation hook interface for native AA
-/// @author getclave.io
+/// @author Initially getclave.io, updated by Matter Labs
 /// @notice Validation hooks trigger before each transaction,
 /// can be used to enforce additional restrictions on the account and/or transaction during the validation phase.
 interface IValidationHook is IModule, IERC165 {
@@ -18,7 +18,7 @@ interface IValidationHook is IModule, IERC165 {
 }
 
 /// @title Execution hook interface for native AA
-/// @author getclave.io
+/// @author Initially getclave.io, updated by Matter Labs
 /// @notice Execution hooks trigger before and after each transaction, during the execution phase.
 interface IExecutionHook is IModule, IERC165 {
   /// @notice Hook that triggers before each transaction during the execution phase.

src/interfaces/IOwnerManager.sol

@@ -20,15 +20,15 @@ interface IOwnerManager {
 
   /**
    * @notice Adds a k1 owner to the list of k1 owners
-   * @dev Can only be called by self or a whitelisted module
+   * @dev Can only be called by self
    * @dev Address can not be the zero address
    * @param addr address - Address to add to the list of k1 owners
    */
   function k1AddOwner(address addr) external;
 
   /**
    * @notice Removes a k1 owner from the list of k1 owners
-   * @dev Can only be called by self or a whitelisted module
+   * @dev Can only be called by self
    * @param addr address - Address to remove from the list of k1 owners
    */
   function k1RemoveOwner(address addr) external;

src/managers/HookManager.sol

@@ -16,8 +16,8 @@ import { IModule } from "../interfaces/IModule.sol";
 /**
  * @title Manager contract for hooks
  * @notice Abstract contract for managing the enabled hooks of the account
- * @dev Hook addresses are stored in a linked list
- * @author https://getclave.io
+ * @dev Hook addresses are stored in an enumerable set
+ * @author Initially https://getclave.io, then updated by Matter Labs
  */
 abstract contract HookManager is IHookManager, SelfAuth {
   using EnumerableSet for EnumerableSet.AddressSet;

src/managers/OwnerManager.sol

@@ -11,7 +11,7 @@ import { IOwnerManager } from "../interfaces/IOwnerManager.sol";
  * @title Manager contract for owners
  * @notice Abstract contract for managing the owners of the account
  * @dev K1 Owners are secp256k1 addresses
- * @dev Owners are stored in a linked list
+ * @dev Owners are stored in an enumerable set
  * @author https://getclave.io
  */
 abstract contract OwnerManager is IOwnerManager, SelfAuth {

src/test/TestWebAuthValidator.sol

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import { WebAuthValidator } from "../validators/WebAuthValidator.sol";
+
+contract WebAuthValidatorTest is WebAuthValidator {
+  /// @notice Verifies a message using the P256 curve.
+  /// @dev Useful for testing the P256 precompile
+  /// @param message The sha256 hash of the authenticator hash and hashed client data
+  /// @param rs The signature to validate (r, s) from the signed message
+  /// @param pubKey The public key to validate the signature against (x, y)
+  /// @return valid true if the signature is valid
+  function p256Verify(
+    bytes32 message,
+    bytes32[2] calldata rs,
+    bytes32[2] calldata pubKey
+  ) external view returns (bool valid) {
+    valid = rawVerify(message, rs, pubKey);
+  }
+}

src/validators/SessionKeyValidator.sol

@@ -68,7 +68,8 @@ contract SessionKeyValidator is IModuleValidator {
     require(sessionCounter[msg.sender] == 0, "Revoke all keys first");
   }
 
-  /// @notice This module should not be used to validate signatures
+  /// @notice This module should not be used to validate signatures (including EIP-1271),
+  /// as a signature by itself does not have enough information to validate it against a session.
   /// @return false
   function validateSignature(bytes32, bytes memory) external pure returns (bool) {
     return false;

src/validators/WebAuthValidator.sol

@@ -20,7 +20,11 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator {
   using JSONParserLib for JSONParserLib.Item;
   using JSONParserLib for string;
 
+  /// @dev P256Verify precompile implementation, as defined in RIP-7212, is found at
+  /// https://github.com/matter-labs/era-contracts/blob/main/system-contracts/contracts/precompiles/P256Verify.yul
   address private constant P256_VERIFIER = address(0x100);
+
+  // check for secure validation: bit 0 = 1 (user present), bit 2 = 1 (user verified)
   bytes1 private constant AUTH_DATA_MASK = 0x05;
   bytes32 private constant LOW_S_MAX = 0x7fffffff800000007fffffffffffffffde737d56d38bcf4279dce5617e3192a8;
   bytes32 private constant HIGH_R_MAX = 0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551;
@@ -63,7 +67,7 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator {
     upperKeyHalf[originDomain][msg.sender] = key32[1];
 
     // we're returning true if this was a new key, false for update
-    bool keyExists = initialLowerHalf == 0 && initialUpperHalf == 0;
+    bool keyExists = uint256(initialLowerHalf) == 0 && uint256(initialUpperHalf) == 0;
 
     emit PasskeyCreated(msg.sender, originDomain);
 
@@ -115,17 +119,19 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator {
       fatSignature
     );
 
+    // prevent signature replay https://yondon.blog/2019/01/01/how-not-to-use-ecdsa/
     if (uint256(rs[0]) <= 0 || rs[0] > HIGH_R_MAX || uint256(rs[1]) <= 0 || rs[1] > LOW_S_MAX) {
       return false;
     }
 
-    // check if the flags are set
+    // https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API/Authenticator_data#attestedcredentialdata
     if (authenticatorData[32] & AUTH_DATA_MASK != AUTH_DATA_MASK) {
       return false;
     }
 
-    // parse out the important fields (type, challenge, origin, crossOrigin): https://goo.gl/yabPex
+    // parse out the required fields (type, challenge, crossOrigin): https://goo.gl/yabPex
     JSONParserLib.Item memory root = JSONParserLib.parse(clientDataJSON);
+    // challenge should contain the transaction hash, ensuring that the transaction is signed
     string memory challenge = root.at('"challenge"').value().decodeString();
     bytes memory challengeData = Base64.decode(challenge);
     if (challengeData.length != 32) {
@@ -135,20 +141,27 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator {
       return false;
     }
 
+    // type ensures the signature was created from a validation request
     string memory type_ = root.at('"type"').value().decodeString();
     if (!Strings.equal("webauthn.get", type_)) {
       return false;
     }
 
+    // the origin determines which key to validate against
+    // as passkeys are linked to domains, so the storage mapping reflects that
     string memory origin = root.at('"origin"').value().decodeString();
     bytes32[2] memory pubkey;
     pubkey[0] = lowerKeyHalf[origin][msg.sender];
     pubkey[1] = upperKeyHalf[origin][msg.sender];
     // This really only validates the origin is set
-    if (pubkey[0] == 0 || pubkey[1] == 0) {
+    if (uint256(pubkey[0]) == 0 || uint256(pubkey[1]) == 0) {
       return false;
     }
 
+    // cross-origin validation is optional, but explicitly not supported.
+    // cross-origin requests would be from embedding the auth request
+    // from another domain. The current SSO setup uses a pop-up instead of
+    // an i-frame, so we're rejecting these until the implemention supports it
     JSONParserLib.Item memory crossOriginItem = root.at('"crossOrigin"');
     if (!crossOriginItem.isUndefined()) {
       string memory crossOrigin = crossOriginItem.value();
@@ -193,7 +206,7 @@ contract WebAuthValidator is VerifierCaller, IModuleValidator {
     bytes32 message,
     bytes32[2] calldata rs,
     bytes32[2] calldata pubKey
-  ) external view returns (bool valid) {
+  ) internal view returns (bool valid) {
     valid = callVerifier(P256_VERIFIER, message, rs, pubKey);
   }
 }