fhevm-solidity
fhevm-solidity copied to clipboard
feat: new async-decrypt API
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 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
@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 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
CiphertextTypeenum, you can already see btw that I prepared support for EUINT128 in the future despite not being implemented by fhevm yethttps://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?
API looks good! I just have one question that I posted above.
@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
CiphertextTypeenum, 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#L8Yeah 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 ?
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
requestDecryptiontransaction. 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:approveEUintXXXfunctions, called inside theOracle:toCiphertextinternal library functions - from therequestDecryptionin order to be able to send ciphertexts of different types in a singlerequestDecryptioncall. This will add some complexity in the oracle service as we will now have to loop on all stored array of handles in theverifiedEUintXXXsmappings (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 norequestDecryptionhappened meanwhile- but risk is inexistent as long as user ensures there is no call to anapproveEUintXXXfunction 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
fulfillRequestfunction of theOraclePredeploy
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.
Mocked mode is now ready. Only remaining work now is on oracle service side.