fhevm-solidity icon indicating copy to clipboard operation
fhevm-solidity copied to clipboard

feat: new async-decrypt API

Open jatZama opened this issue 1 year ago • 7 comments

This new API is cleaner and more concise (no more almost duplicated events for each types of ciphertexts) and allows users to request decryption of different types in a single requestDecryption transaction. Work in Progress status: DONE 1st commit ready : all the solidity code is done and ready for review already ✅ 2nd commit will come soon for the JS part (tests+mocked mode). ✅ NOW DONE!

For the solidity part, discussion and approval needed on this: I had to decouple the storage of honestly obtained ciphertext in privileged storage - via the OraclePredeploy:approveEUintXXX functions, called inside the Oracle:toCiphertext internal library functions - from the requestDecryption in order to be able to send ciphertexts of different types in a single requestDecryptioncall. This will add some complexity in the oracle service as we will now have to loop on all stored array of handles in the verifiedEUintXXXs mappings (for a fixed counter/key index and type) until we find at least one handle which comes from a honestly obtained ciphertext, i.e with corresponding ciphertext in privileged storage. Also security model guarantees are not 100% the same - in theory a user could approve a honestly obtained handle in a first transaction and (some other malicious user) request decryption in a later transaction even with non-honestly obtained identical handle if counter did not change i.e if no requestDecryption happened meanwhile- but risk is inexistent as long as user ensures there is no call to an approveEUintXXX function unless he wanted to request a decryption in the same transaction. In summary there should be no risk unless the smart contract of the dApp is malicious.

Also, it is now the responsibility of the relayer to do the correct abi-encoding of the concatenated decrypted variables and pass them as a single bytes array argument to the fulfillRequest function of the OraclePredeploy

jatZama avatar Apr 11 '24 22:04 jatZama

@jatZama Also I'm curious, maybe the idea to bind to ciphertexts is not ideal, like, what if we add another type, we can't redeploy smart contract. Maybe the ciphertext type should be just a number, I'm curious

david-zk avatar Apr 12 '24 08:04 david-zk

@jatZama Also I'm curious, maybe the idea to bind to ciphertexts is not ideal, like, what if we add another type, we can't redeploy smart contract. Maybe the ciphertext type should be just a number, I'm curious

Not sure to understand exactly what do you mean. Adding new types in the future should be more straigthforward, as you only need to update the CiphertextType enum, you can already see btw that I prepared support for EUINT128 in the future despite not being implemented by fhevm yet https://github.com/zama-ai/fhevm/blob/1fc5feda7fd4d448c3dd345aacd271ccd17ea908/oracle/OraclePredeploy.sol#L8

jatZama avatar Apr 12 '24 10:04 jatZama

@jatZama Also I'm curious, maybe the idea to bind to ciphertexts is not ideal, like, what if we add another type, we can't redeploy smart contract. Maybe the ciphertext type should be just a number, I'm curious

Not sure to understand exactly what do you mean. Adding new types in the future should be more straigthforward, as you only need to update the CiphertextType enum, you can already see btw that I prepared support for EUINT128 in the future despite not being implemented by fhevm yet

https://github.com/zama-ai/fhevm/blob/1fc5feda7fd4d448c3dd345aacd271ccd17ea908/oracle/OraclePredeploy.sol#L8

Yeah but these variables

    mapping(uint256 => ebool[]) internal verifiedEBools; // to check that ciphertexts have been honestly obtained via privileged storage
    mapping(uint256 => euint4[]) internal verifiedEUint4s;
    mapping(uint256 => euint8[]) internal verifiedEUint8s;
    mapping(uint256 => euint16[]) internal verifiedEUint16s;
    mapping(uint256 => euint32[]) internal verifiedEUint32s;
    mapping(uint256 => euint64[]) internal verifiedEUint64s;
    mapping(uint256 => eaddress[]) internal verifiedEAddresses;

Correct me if I'm wrong but I believe we can't redeploy smart contract with new variables?

david-zk avatar Apr 12 '24 11:04 david-zk

API looks good! I just have one question that I posted above.

dartdart26 avatar Apr 12 '24 12:04 dartdart26

@jatZama Also I'm curious, maybe the idea to bind to ciphertexts is not ideal, like, what if we add another type, we can't redeploy smart contract. Maybe the ciphertext type should be just a number, I'm curious

Not sure to understand exactly what do you mean. Adding new types in the future should be more straigthforward, as you only need to update the CiphertextType enum, you can already see btw that I prepared support for EUINT128 in the future despite not being implemented by fhevm yet https://github.com/zama-ai/fhevm/blob/1fc5feda7fd4d448c3dd345aacd271ccd17ea908/oracle/OraclePredeploy.sol#L8

Yeah but these variables

    mapping(uint256 => ebool[]) internal verifiedEBools; // to check that ciphertexts have been honestly obtained via privileged storage
    mapping(uint256 => euint4[]) internal verifiedEUint4s;
    mapping(uint256 => euint8[]) internal verifiedEUint8s;
    mapping(uint256 => euint16[]) internal verifiedEUint16s;
    mapping(uint256 => euint32[]) internal verifiedEUint32s;
    mapping(uint256 => euint64[]) internal verifiedEUint64s;
    mapping(uint256 => eaddress[]) internal verifiedEAddresses;

Correct me if I'm wrong but I believe we can't redeploy smart contract with new variables?

The issue is that we still need to store honestly obtained ciphertexts inside the privileged storage of OraclePredeploy. If we need to add other mappings in the future we could make the OraclePredeploy contract upgradable, using UUPS pattern, but I am not sure this is a good solution security-wise, and this was never discussed before. Wdyt @dartdart26 ?

jatZama avatar Apr 12 '24 13:04 jatZama

This new API is cleaner and more concise (no more almost duplicated events for each types of ciphertexts) and allows users to request decryption of different types in a single requestDecryption transaction. Work in Progress status: 1st commit ready : all the solidity code is done and ready for review already ✅ 2nd commit will come soon for the JS part (tests+mocked mode). 🚧

For the solidity part, discussion and approval needed on this: I had to decouple the storage of honestly obtained ciphertext in privileged storage - via the OraclePredeploy:approveEUintXXX functions, called inside the Oracle:toCiphertext internal library functions - from the requestDecryption in order to be able to send ciphertexts of different types in a single requestDecryptioncall. This will add some complexity in the oracle service as we will now have to loop on all stored array of handles in the verifiedEUintXXXs mappings (for a fixed counter/key index and type) until we find at least one handle which comes from a honestly obtained ciphertext, i.e with corresponding ciphertext in privileged storage. Also security model guarantees are not 100% the same - in theory a user could approve a honestly obtained handle in a first transaction and (some other malicious user) request decryption in a later transaction even with non-honestly obtained identical handle if counter did not change i.e if no requestDecryption happened meanwhile- but risk is inexistent as long as user ensures there is no call to an approveEUintXXX function unless he wanted to request a decryption in the same transaction. In summary there should be no risk unless the smart contract of the dApp is malicious.

Also, it is now the responsibility of the relayer to do the correct abi-encoding of the concatenated decrypted variables and pass them as a single bytes array argument to the fulfillRequest function of the OraclePredeploy

Just realized thanks to @dartdart26 that ciphertext would still get verified even after unwrapping them to uint256, which makes sense since custom types are not native. This allowed me to simplify quite a bit the PR, by removing all the verifiedCiphertexts mappings , and also removing some risk because now we no more have decoupling between ciphertext verification and decryption request, everything happens during the request. Please check new commit, btw it also solves the concern @david-zama raised about lack of upgradability. The only question still pending is : should I remove the CiphertextType enum? It is better to keep it for readability for the moment at least I think, also I am using it for the no-op logic during the request. Maybe in the future we could remove it if we decide to implement a standard no-op operator common for all encrypted types. For instance for the moment, eaddress supports only operators which are not supported by ebool, so I had to do use a different no-op in this case.

jat9292 avatar Apr 12 '24 14:04 jat9292

Mocked mode is now ready. Only remaining work now is on oracle service side.

jatZama avatar Apr 16 '24 10:04 jatZama