rdubois-crypto / FreshCryptoLib

Cryptographic Primitives for Blockchain Systems (solidity, cairo, C and rust)
MIT License
124 stars 22 forks source link

Unable to use `memory` variables #9

Closed carlosfaria94 closed 11 months ago

carlosfaria94 commented 12 months ago

I'm trying to use this library with the ERC-4337 reference implementation for a PoC.

  function _validateSignature(
    UserOperation calldata userOp,
    bytes32 userOpHash
  ) internal virtual override returns (uint256 validationData) {
    (
      bytes32 keyHash,
      bytes memory authenticatorData,
      bytes1 authenticatorDataFlagMask,
      bytes memory clientData,
      bytes32 clientChallenge,
      uint256 clientChallengeDataOffset,
      uint256[2] memory rs
    ) = abi.decode(userOp.signature, (bytes32, bytes, bytes1, bytes, bytes32, uint256, uint256[2]));

    PassKeyId memory publicKey = authorisedKeys[keyHash];
    if (
      FCL_WebAuthn.checkSignature(
        authenticatorData,
        authenticatorDataFlagMask,
        clientData,
        clientChallenge,
        clientChallengeDataOffset,
        rs,
        [publicKey.pubKeyX, publicKey.pubKeyY]
      )
    ) {
      return 0;
    }

    return 1;
  }

I need do decode the UserOperation, and thus it needs to be stored in memory, however your library heavily use calldata (e.g. calldataload and calldatacopy) making it harder (or not possible?).

There is another way to do this?

Thank you for your contribution!

rdubois-crypto commented 12 months ago

Hi, thanks for your interest and staring of the repo.

A solution is either to deploy the library then call it, or modifying the type from calldata to memory recursively. Hope that helps.

carlosfaria94 commented 11 months ago

Thank you @rdubois-crypto

obatirou commented 11 months ago

@rdubois-crypto As other use cases (ERC-1271 more precisely) use memory variables, would you be open to a proposition of implementation to be compatible with memory for FCL_WebAuthn ? It would avoid deployment of the library + an external call or forking FCL + modifying code. Having it directly in the FCL repo would reduce the amount of errors when building on top. Also, if I am not mistaken, at least in foundry, to deploy the lib and call it, a user would have to modify the FCL code to have at least one function external or public. All function of FCL_Webauthn.sol are internal hence the lib should be embedded in the calling contract https://github.com/rdubois-crypto/FreshCryptoLib/blob/548e9b112126dbf2054ab12a32c6ff8b7594ab56/solidity/src/FCL_Webauthn.sol#L38

Either the library should use memory or should have external functions (imo and if I am not mistaken) What are your thoughts on this ?

Update: see issue #21