Summary
Type: Layer 2 & Rollups
Timeline: September 10, 2025 → September 22, 2025
Languages: Solidity
Findings
Total issues: 22 (22 resolved)
Critical: 0 (0 resolved) · High: 0 (0 resolved) · Medium: 2 (2 resolved) · Low: 5 (5 resolved)
Notes & Additional Information
15 notes raised (15 resolved)
OpenZeppelin audited the jovaynetwork/jovay-contracts repository at commit 24f525f.
Following the conclusion of the fix review, the final post-audit commit is bdaf093.
In scope were the following files. The root of the tree is located at jovay-contracts/rollup_contracts/contracts/L1/tee_verifier/src:
src
├── interfaces
│ └── ITeeRollupVerifier.sol
├── DcapAttestationRouter.sol
├── MeasurementDao.sol
├── TEECacheVerifier.sol
└── TEEVerifierProxy.sol
In addition, two patches were introduced: one adding verification for version 5 quotes (0001-support-v5-quote.patch) and another containing a crucial one-line fix for the PCCSRouter contract (0001-bugfix-collaterals-expiration-check.patch). A diff audit of V5QuoteVerifier.sol against V4QuoteVerifier.sol located in the same directory was performed, along with a diff audit of PCCSRouter.sol against the pre-patch version.
The system under review provides an on-chain verification mechanism for TEE (Trusted Execution Environment) attestations, designed to integrate with a rollup architecture. Its primary function is to validate that a given data commitment was generated within a genuine and up-to-date Intel SGX or TDX enclave. This is achieved by processing attestation quotes through a series of smart contracts that check the quote's cryptographic validity and cross-reference its measurements against a trusted registry. The system is modular, separating the user-facing entry point, internal routing logic, gas-saving cache, measurement data, and the external verification library into distinct, interconnected components.
TEEVerifierProxyThe TEEVerifierProxy contract serves as the primary entry point for the verification system. It abstracts the underlying complexity of the attestation process from the end-user, which is typically another smart contract (in this system, the Rollup contract). Its state is limited to configuration and access control and it does not store any data related to the proofs themselves. Its main role is to forward the verification requests to the appropriate logic contract.
Core Functionality:
aggrProof) for verification via the verifyProof function. Here, "proof" may mean a proof in the traditional sense, in the form of a non-interactive zero-knowledge (ZK) proof, or a TEE proof, also known as a quote. In later versions of this system, ZK proofs will be supported. Currently, however, only TEE proofs are supported. A TEE proof establishes a commitment to public data, whose computation is attested to by a P-256 ECDSA signature generated using the private key of a trusted execution environment with verified measurements.verifyProof call to the configured DcapAttestationRouter to perform the actual verification.onlyAuthorized modifier, restricting access to the verifyProof function to whitelisted callers managed by the contract owner.DcapAttestationRouterThis contract is the central hub of the verification logic. It orchestrates the attestation process by routing the proof to the appropriate components based on its configuration and the properties of the proof itself. It determines whether to perform a full verification via the external automata-dcap-attestation library or to use the internal TEECacheVerifier for an expedited check.
Procedural Flow:
toVerifyMr is enabled, it parses the TEE type (SGX or TDX) and calls the MeasurementDao to verify the TEE's measurements (e.g., MRENCLAVE, RTMR, and MRTD) against a trusted whitelist.CacheOption is enabled, it checks if the quote's attestation key is already initialized in the TEECacheVerifier. If so, it uses the cache for a faster, on-chain verification.dcapAttestation contract (the external library) for full, comprehensive verification.TEECacheVerifierThe TEECacheVerifier contract provides an on-chain mechanism for verifying quotes, designed to optimize gas costs and reduce reliance on the more complex external dcapAttestation contract. It works by caching previously verified attestation keys. If a quote is signed by a key that has already been fully validated and cached, this contract can verify it on-chain by checking the ECDSA signature against the quote's header and body.
Key Features:
verifyAndAttestOnChain to perform on-chain ECDSA signature verification for quotes signed by a cached key, supporting v3, v4, and v5 quote formatsinitializeCache, deleteKey, and clearupAllKey) and authorized callersMeasurementDaoThe MeasurementDao is a stateful contract that acts as an allowlist registry for trusted TEE measurements. It stores and manages cryptographic identifiers corresponding to approved enclave software. The DcapAttestationRouter contract queries this contract to ensure that a quote originates from an authorized and secure application.
Managed Measurements:
This external set of contracts, included as a dependency, performs the heavy lifting of full attestation verification. It is responsible for parsing the complex structures of different quote versions, verifying cryptographic signatures, and validating the entire certificate chain for the attestation collaterals against data stored in a separate on-chain PCCS system. The scope includes a patch that adds support for Quote v5, and another patch that fixes a critical bug in collateral validation.
Key Components:
PCCSRouter: Manages and verifies the chain of trust for Platform Certificate and Key Services (PCKS) collaterals by fetching them from various Data Access Object contracts. A critical bug-fix patch was applied to this contract to ensure it correctly checks that the verification timestamp is within the collateral's issuedAt and expiredAt dates.V5QuoteVerifier: A new contract introduced via a patch to handle the specific structure and verification logic for the Intel TDX Quote Version 5 format. It parses TD15ReportBody structures and performs the necessary TCB and module checks.V5Structs: A new contract defining the data structures (e.g., TD15ReportBody and ECDSAQuoteV5AuthData) necessary to parse and handle the version 5 quote format on-chain.The system's security is founded on the cryptographic guarantees of Intel's DCAP attestation protocol, the integrity of the bundled Automata Network automata-dcap-attestation contracts, and the proper administrative management of the on-chain components. It assumes that a secure and trusted off-chain environment generates the proofs, and the on-chain system's role is solely to verify the integrity of those proofs. The security model does not extend to the validity of the data within the commitment itself, only to the authenticity of its origin.
automata-dcap-attestation library. Any vulnerability in this dependency will compromise the entire verification process.owner populating the MeasurementDao with the correct and secure measurement values (MRENCLAVE, MRSIGNER, RTMRs, and MRTDs). Failure to do so could lead to the system trusting proofs from insecure or malicious enclaves.MRENCLAVE, MRSIGNER), and verify that they match the trusted values registered on-chain in the MeasurementDao. Without this transparency, users must blindly trust that the whitelisted measurements correspond to secure and non-malicious application logic.TEECacheVerifier.dcapAttestation contract is configured with its fee (_feeBP) set to zero. The DcapAttestationRouter calls verifyAndAttestOnChain on this external contract without passing any msg.value, meaning the call will fail if the external contract expects a fee to be paid.owner key is stored securely and that all administrative actions are performed in a secure environment, free from malware or phishing threats that could lead to key compromise.TEEVerifierProxy (removed) and TEECacheVerifier contracts automatically authorize their owners in their constructors. Authorizations are subsequently used to restrict access to functions such as verifyProof and initializeCache, which modify the contract's state. Notably, when ownership is transferred via transferOwnership, the previous owner’s authorization remains. This is the intended design and the previous owner is assumed to not act maliciously.The following privileged roles were identified in the system under review:
owner role holds significant and centralized power across the entire system. This owner is fully trusted to:
DcapAttestationRouter, MeasurementDao, TEECacheVerifier, and the external dcapAttestation contract.MeasurementDao contract. An attacker who compromises the owner key can whitelist malicious TEE software, thereby breaking the entire system's trust model.TEECacheVerifier, including the ability to add, delete, or clear keys.TEEVerifierProxy, DcapAttestationRouter, and TEECacheVerifier are configured by the owner. Corresponding functions gated by the onlyAuthorized modifier are only allowed to be called by authorized callers. The owner also has the ability to enable or disable caller restriction: disabling the caller restriction allows these functions to be called publicly. The list of authorized callers, and what their authorization allows them to do, differs in each contract.
TEEVerifierProxy: Authorized callers can call verifyProof. The authorized callers should at least include the Rollup contract.DcapAttestationRouter: Authorized callers can call verifyProof. The authorized callers should at least include the TEEVerifierProxy contract.TEECacheVerifier: Authorized callers can call verifyAndAttestOnChain and initializeCache. The authorized callers should at least include the DcapAttestationRouter contract.The DcapAttestationRouter contract is designed to verify attestation proofs from different Trusted Execution Environments (TEEs) and quote versions. The verification logic relies on parsing a serialized data structure returned by an attestation service to extract a security commitment, which corresponds to the reportData field in the original TEE report. The contract uses a set of predefined constants (USER_DATA_V3_OFFSET, USER_DATA_V4_OFFSET, and USER_DATA_V5_OFFSET) to locate this commitment based on the quote version.
However, the implementation does not correctly handle all valid permutations of version 4 quotes. While version 4 quotes can be generated for both SGX and TDX enclaves, the underlying report structure and the offset of the reportData field differ between the two. The _verifyProof function checks the quote version but fails to differentiate by TEE type. For any v4 quote, it unconditionally uses the USER_DATA_V4_OFFSET value, which is calculated for a TDX report. When the contract processes a v4 quote from an SGX enclave, it applies this incorrect offset, causing it to read from the wrong location in the serialized data and failing to extract the valid commitment.
Consider modifying the _verifyProof function to ensure that it correctly distinguishes between TEE types for version 4 quotes. The logic should be updated to read the TEE type from the quote header and use a conditional check. When the quoteVersion is 4, this check should apply the USER_DATA_V4_OFFSET for a TDX quote but apply the correct, distinct offset required for an SGX quote.
Update: Resolved in pull request #9 at commit 3fac035. The team states that only v5 quotes will be used, and the code for handling v3/v4 quotes is kept now for compatibility but will be removed later.
The owner-only deletion functions delete_mr_enclave, delete_rtMr, anddelete_mrtd, and the authorized-only deleteKey function linearly scan their respective arrays to locate items before removal, resulting in O(n) complexity. With large lists, these operations risk exceeding block gas limits, leading to out-of-gas failures and potential DoS of administrative maintenance. OpenZeppelin's EnumerableMap and EnumerableSet structs perform deletions, as well as additions and existence checking, in O(1) time. In addition, they encompass both the mapping and accompanying list in one object, which would simplify the code.
Consider importing EnumerableMap and EnumerableSet, implementing mr as a Bytes32ToBytes32Map, and implementing rtmr, mrtdMap, and _verificationCache as BytesSets. Alternatively, consider recording item index in mappings at insertion time to enable O(1) deletions, or enforcing an upper bound on list sizes to keep worst-case gas within safe limits.
Update: Resolved in pull request #11 at commit eddd42d. The Jovay team added four index mappings (mrEnclaveIndex, rtmrIndex, mrtdIndex, _keyIndex) to store the list positions of added items.
_verifyMeasurementThe verifyProof function is designed to support both SGX and TDX types. During the verification, the _verifyMeasurement function reads a 4-byte teeType from the quote header offset 4 and routes to SGX or TDX measurement verification accordingly. Especially, when the teeType is SGX_TEE, verifyMeasurementSGX will be called. Otherwise, verifyMeasurementTDX will be called. However, even if teeType is not TDX_TEE (e.g., a corrupted quote), verifyMeasurementTDX will still be called, which may lead to unexpected behavior.
Consider adding a check to ensure that teeType is either SGX_TEE or TDX_TEE. Alternatively, consider returning false or reverting the transaction.
Update: Resolved in pull request #25 at commit 558d082, and pull request #50 at commit 52d1cf2.
Throughout the codebase, multiple instances of missing docstrings were identified:
DcapAttestationRouter.sol, none of the constants, state variables, and functions have docstrings.MeasurementDao.sol, none of the constants and state variables as well as most of the functions do not have docstrings.TEECacheVerifier.sol, none of the constants, state variables, and functions have docstrings.TEEVerifierProxy.sol, none of the state variables and functions have docstrings.ITeeRollupVerifier.sol, the verifyProof function has an incomplete docstring. The return values are not documented.Where there is documentation, it is often sparse and does not provide enough information for the reader to understand the code. For example, the docstrings for the add_rtMr and delete_rtMr functions mention the parameter rtmr3 but do not explain its meaning.
Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved in pull request #27 at commits 55210fc and 973ac1c.
Using an outdated version of Solidity can lead to vulnerabilities and unexpected behavior in contracts. It is important to keep the Solidity compiler version up to date. In addition, floating pragma directives were observed throughout the codebase. Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled. However, throughout the codebase, the files in scope are using an outdated Solidity version with a floating pragma (^0.8.0).
Consider taking advantage of the latest Solidity version to improve the overall readability and security of the codebase. Regardless of which version of Solidity is used, consider pinning the version consistently throughout the codebase using a fixed pragma directive in order to prevent bugs due to incompatible future releases.
Update: Resolved in pull request #26 at commit 9c22490.
The functions for clearing mappings, clearup_mr_enclave, clearup_rtMr, clearup_mrtd, and clearupAllKey iterate through each element of the corresponding list, running in O(n) time. If the lists grow too large, this operation can exceed the block gas limit and revert. This can prevent the measurement or cache clearing, blocking a desired administrative function. In clearupAllKey, the fact that the _initializedKeys list contains at most 10,000 keys is meant to mitigate this issue. However, at 2,900 gas per deletion, clearing 10,000 keys would cost 29 million gas, which exceeds the block gas limit of Ethereum. The measurement lists have no length restrictions.
Erasing the stored memory of each entry of a list will necessarily take O(n) time: OpenZeppelin's EnumerableMap and EnumerableSet, for example, have O(n) clear functions built in. If clearing the entire list at once is never needed, or there is reason to believe that the lists will never grow too large, this may be acceptable: it is recommended to document the rationale in this case. However, if this functionality is necessary and the lists may grow large in practice, there are alternative methods that sidestep the entire issue by not actually clearing the memory, some examples of which are provided below. Note that these are given in terms of mappings and lists, but each of these options can be implemented mutatis mutandis using EnumerableMap and EnumerableSet structs:
_initializedKeys list (e.g., 1000 items).MeasurementDao or TEECacheVerifier contract, and update the corresponding address in DcapAttestationRouter. This implies that the mr, rtmr, and mrtdMap mappings and their corresponding lists would all be cleared at the same time: to clear only one of them, entries of the other two would have to be manually added to the new contract.mapping(uint256 version => mapping(bytes measurement => bool isApproved)) rtmr; where rtmr[0][x] == true indicates that x is in the first set of rtmr measurements. Note that, in order to make this secure and prevent keys from older versions from being used, an auxiliary variable uint256 currentVersionNumber would have to be created, and measurements or keys would need to be added, deleted, or checked for existence only from mappings or lists with first coordinate currentVersionNumber.Consider implementing one of the above-listed options to guarantee the owner's ability to clear each list.
Update: Resolved in pull request #27 at commits eac3d4d and 95d61f8.
_loadDataIfNotExpiredThe collateral validity gate in PCCSRouter._loadDataIfNotExpired relies on the pcsDaoAddr’s getCollateralValidity function to return validity expiration bounds. It only guards on non-empty return data (ret.length > 0) and upon the successful decoding of two variables (uint64 issuedAt and uint64 expiredAt). However, the getCollateralValidity function will always return two named uint64 values that default to zero when the validity slot is missing (see getCollateralValidity and _loadPcsValidity), which causes the ret.length > 0 check to always succeed. The current check can still treat missing collaterals as valid when callers supply a zero timestamp to getCertHashWithTimestamp or getCrlHashWithTimestamp, leading to unexpected results.
Consider aligning with the upstream implementation by requiring both decoded values to be non‑zero values (issuedAt != 0 && expiredAt != 0) and then applying the range check.
Update: Resolved in pull request #27 at commit 6b1a05a.
Both DcapAttestationRouter.sol and TEEVerifierProxy.sol lack an SPDX License Identifier, whereas other contracts in the repository include it. This omission introduces ambiguity about the intended license, and can create compliance and distribution risks for downstream users and integrations.
Consider adding // SPDX-License-Identifier: MIT as the first line in both files to align with the repository standard and eliminate ambiguity.
Update: Resolved in pull request #28 at commit b707359 and pull request #55 at commit a6c6386.
Throughout the codebase, multiple instances of unused imports and code were identified:
DcapAttestationRouter.sol line 4, TEEVerifierProxy.sol line 4, and TEECacheVerifier.sol line 9, console is imported but not used.MeasurementDao.sol line 7, the console is commented out instead of being deleted.TEECacheVerifier, the disableCallerRestriction function is commented out but not deleted.TEECacheVerifier, in the deleteKey function, the check on the number of keys is commented out.verifyAndAttestOnChain function of TEECacheVerifier, the version is passed in as an argument, but there is commented-out code that extracts the version from the raw quote.V5QuoteVerifier, the _verifySGXQuote function is never used.To improve code quality, readability, and maintainability, consider removing all unused imports and code.
Update: Resolved in pull request #29.
MeasurementDAO and TEECacheVerifierThe owner-only registration functions add_mr_enclave, add_rtMr, add_mrtd, and the authorized-only registration function initializeCache, do not validate parameters completely:
add_rtMr and add_mrtd accept arbitrary-length bytes instead of enforcing the expected 48‑byte SHA‑384 length.initializeCache accepts arbitrary-length bytes without enforcing the expected 64-byte ECDSA attestation key length.add_rtMr, add_mrtd, add_mr_enclave, and initializeCache reject zero values.When the owner or an authorized user accidentally registers incorrect-length or zero values, subsequent verification fails, and unnecessary gas costs are incurred.
Consider enforcing strict parameter validation in the aforementioned functions by checking input length and value.
Update: Resolved in pull request #31 at commit 81278fa.
The _verifyProof function of the DcapAttestationRouter contract calls CacheAttestation.parseAttestationKey before checking whether caching is enabled. If CacheOption == false, then this call is unnecessary and wastes gas.
Consider fetching CacheOption into a local variable and checking it before calling CacheAttestation.parseAttestationKey.
Update: Resolved in pull request #49 at commits 2bde615 and 679b35d.
Throughout the codebase, multiple instances of functions updating the state without an event emission were identified:
In DcapAttestationRouter.sol:
setAuthorized functionenableCallerRestriction functiondisableCallerRestriction function_setConfig functionenableVerifyMRTD functiondisableVerifyMRTD functionIn MeasurementDao.sol:
add_mr_enclave functiondelete_mr_enclave functionclearup_mr_enclave functionadd_rtMr functiondelete_rtMr functionclearup_rtMr functionadd_mrtd functiondelete_mrtd functionclearup_mrtd functionIn TEECacheVerifier.sol:
setAuthorized functionenableCallerRestriction functioninitializeCache functiondeleteKey functionclearupAllKey functionIn TEEVerifierProxy.sol:
setAuthorized functionenableCallerRestriction functiondisableCallerRestriction function_setConfig functionConsider emitting events whenever there are state changes to improve the clarity of the codebase and make it less error-prone.
Update: Resolved in pull request #27 at commit d6b881d.
Throughout the codebase, multiple instances of state variables lacking an explicitly declared visibility were identified:
DcapAttestationRouter.sol, the _authorized state variableDcapAttestationRouter.sol, the _isCallerRestricted state variableTEECacheVerifier.sol, the _authorized state variableTEECacheVerifier.sol, the _isCallerRestricted state variableTEEVerifierProxy.sol, the _authorized state variableTEEVerifierProxy.sol, the _isCallerRestricted state variableFor improved code clarity, consider always explicitly declaring the visibility of state variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #33 at commit 7fee799.
Providing a specific security contact (such as an email address or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
None of the contracts in scope have a security contact.
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved in pull request #27 at commit 8111e7c.
require StatementsSince Solidity version 0.8.26, custom error support has been added to require statements. Initially, this feature was only available through the IR pipeline. However, Solidity version 0.8.27 extended support for this feature to the legacy pipeline as well.
Throughout the codebase, multiple instances of if-revert statements were identified that could be replaced with require statements:
In DcapAttestationRouter.sol:
In MeasurementDao.sol:
In TEECacheVerifier.sol:
In TEEVerifierProxy.sol:
For conciseness, consider replacing if-revert statements with require statements.
Update: Resolved in pull request #27 at commit 973ac1c7.
In ITeeRollupVerifier.sol, in the NatSpec for the interface, the title is declared as IRollupVerifier, whereas the interface itself is ITeeRollupVerifier.
For improved consistency, consider changing the title to match the name of the contract.
Update: Resolved in pull request #27 at commit b2e9947.
The TEEVerifierProxy, DcapAttestationRouter, and TEECacheVerifier contracts all have identical storage variables (_authorized and _isCallerRestricted) and identical implementations of the onlyAuthorized modifier (functions setAuthorized, enableCallerRestriction, and disableCallerRestriction). Instead of manually duplicating the code for access-control logic, the code could be simplified using inheritance.
Consider importing and inheriting OpenZeppelin's AccessControl or AccessControlEnumerable to securely implement role-based authorization and simplify the codebase. Alternatively, consider creating a separate contract implementing the desired authorization functionality and have all three contracts inherit that.
Update: Resolved in pull request #52 at commits 6883ee3 and 92b5fa6.
In smart contract development, contract names should clearly and accurately reflect their functionality and governance structure. Using established terms for patterns like proxies or DAOs helps align developer and auditor expectations with the contract's actual behavior. Misleading names can cause fundamental misunderstandings about a system's architecture and trust assumptions.
Two contracts in the system have names that are inconsistent with their underlying implementation, creating a potential for confusion:
TEEVerifierProxy contract is named as a "Proxy," a term that strongly implies an upgradeability pattern using delegatecall. However, the contract only forwards calls via a standard external call and does not provide any upgradeability mechanism.MeasurementDao contract uses the "DAO" suffix. This is misleading as "DAO" in the blockchain space refers to a Decentralized Autonomous Organization, which implies community governance. While the name may be intended to signify a "Data Access Object," this is not a standard convention in Solidity and creates ambiguity. Furthermore, although the owner could be a DAO's governance contract, this is not apparent from the code itself, making the name unclear regardless of the intended meaning.To ensure that the names accurately represent the contracts' roles and avoid confusion, consider renaming them. For example, TEEVerifierProxy could be renamed to TEEVerifierForwarder to better describe its forwarding pattern, and MeasurementDao could be renamed to MeasurementRegistry to clarify that it functions as a centrally-controlled list of approved values.
Update: Resolved in pull request #52 at commit 049401e.
The flow of information through the smart contracts in the current architecture, beginning with a call to Rollup.verifyBatch, is Rollup → TEEVerifierProxy → DcapAttestationRouter → (MeasurementDao, TEECacheVerifier, or AutomataDcapAttestationFee). In this flow:
CALL operation (as opposed to a DELEGATECALL).Rollup has an _authorized mapping. This is an allowlist that determines which contracts may call its essential functions, each of which is given the onlyAuthorized modifier.While this architecture does not raise any particular security concerns, it is non-standard and possibly overly complex. Depending on the design goals of the system, it may be considerably simplified:
Rollup contracts may send calls to four TEEVerifierProxy contracts (with two rollups pointing to the same proxy), which point to three different DcapAttestationRouter contracts, which point to two different MeasurementDao and TEECacheVerifier contracts each. If, as appears to be the case in a normal transaction flow, only one contract is expected to be able to call the onlyAuthorized functions, the _authorized whitelist could be replaced with a single address. This would yield a one-to-one mapping of callers to callees, making the chain of calls easier to determine. Note that, if desired, either design may be implemented in conjunction with a role-based access control system such as OpenZeppelin's AccessControl or AccessControlEnumerable.TEEVerifierProxy only contains the address of the DcapAttestationRouter instance and an allowlist for access control. Its only essential function directly forwards the verifyProof call to DcapAttestationRouter. The same result can be achieved by having Rollup store the address of the correct instance of DcapAttestationRouter and calling DcapAttestationRouter directly. The upgradeability is the same since the address in Rollup can be changed to point to the new router, when a new instance of DcapAttestationRouter is deployed.CALL operation is not a standard proxy pattern. Typically, proxies for upgradeability use the DELEGATECALL operation to maintain their own storage while operating functions of the callee. Currently, the recommended proxy pattern in most cases is the Universal Upgradeable Proxy Standard (UUPS) pattern.Consider simplifying the architecture design and/or implementing DcapAttestationRouter as a UUPSUpgradeable contract, while the TEEVerifierProxy can be removed and replaced with a standard ERC1967Proxy. The implementation of upgradeability requires careful review with respect to initialization and storage layout.
Update: Resolved in pull request #61 at commit fb0cb8f. The Jovay team has disregarded the proxy design and removed the TEEVerifierPorxy contract. The authorization allow list mechanism has been kept, and the logic has been moved to AccessControl.sol in pull request #52 at commit 6883ee3.
Throughout the codebase, multiple instances of inconsistent formatting and naming were identified:
TEEVerifierProxy and TEECacheVerifier is in all capitals, but in ITeeRollupVerifier, only the first T is capitalized, and in DcapAttestationRouter, only the first D is capitalized.CacheOption storage variable in DcapAttestationRouter is in CapWords style (a.k.a. Pascal case), whereas the other storage variables are in mixedCase (a.k.a. camel case).mr and rtmr mappings are named without the "Map" suffix, whereas mrtdMap is given the "Map" suffix.add_rtMr, delete_rtMr, get_rtMr, and clearup_rtMr functions only capitalize the letter "M".MeasurementDao contract are written in snake case, whereas functions in Solidity are written in mixedCase.mr, rtmr, or mrtdMap are of the form add..., but the function for adding an attestation key to the cache is named initializeCache, and the function for checking the existence of a key in the cache is named isInitialized. This is confusing because the name initializeCache indicates that the cache itself is being initialized, ideally a one-time operation: correspondingly, isInitialized implies that the function is checking to see if the cache itself has been initialized. For improved clarity, consider renaming these functions to addKey and contains or something similar, respectively.ITeeRollupVerifier states "Verify zk proof", whereas the TEE verifier actually only verifies TEE proofs, and ZK proofs are currently unsupported.clearup_mr_enclave, clearup_rtMr, clearup_mrtd, and clearupAllKey functions, "up" should be removed from the name: "clear up" means to clarify or explain, while "clear" means to empty.MeasurementsDao, the functions for getting and clearing mappings and lists correctly refer to the entire list as the object being retrieved or cleared (for example, get_mrtd and clear_mrtd). However, the names for the functions getAllKey and clearupAllKey refer to the type of item in the list (key), rather than the list itself. Since the cache itself is being retrieved or cleared, consider renaming these functions to getCache and clearCache, respectively.Consider adhering to the Solidity style guide for formatting and establishing a consistent naming convention for functions and variables throughout the codebase.
Update: Resolved in pull request #51 and pull request #52.
initializeCacheIn the initializeCache function, there is no validation to confirm that the input key is not already in the _verificationCache mapping before it is added to the mapping and the _initializedKeys list. Currently, the _verifyProof function in the DcapAttestationRouter contract ensures that the key is not already added before invoking the initializeCache function. However, if other authorized users can directly call the initializeCache function, this could lead to unintended duplicate keys in the _initializedKeys list. This does not constitute a security vulnerability, as the deleteKey function removes all duplicate entries, ensuring that deleted keys do not persist. Nevertheless, this oversight temporarily permits duplicate entries and leads to unnecessary gas consumption.
Consider implementing a check to verify whether the key already exists in the _verificationCache before adding it to the _verificationCache mapping and the _initializedKeys list.
Update: Resolved in pull request #51 at commit f7b259b.
In the verifyMeasurementSGX, verifyMeasurementTDX, and verifyMRTD functions of MeasurementDao, and in the _verifyMeasurement function of DcapAttestationRouter, the substring method is called on the input quote to obtain mrEnclave, mrSigner, rtmr3, mrtd, and teeType. If the quote is not long enough when substring is called, the call will revert without error message. This may cause unexpected behavior, since in each of these functions the caller expects a boolean value. To adhere to the verification function's contract, if the quote isn't long enough, it should just return false.
Consider checking that the quote is long enough before calling substring to avoid unexpected reversions.
Update: Resolved in pull request #51 at commit 92cc621.
The audited scope covered a system for on-chain verification of TEE quotes. In this system, TEE verification is applied to the verification of state transitions in an L2 rollup and leverages the Automata DCAP Attestation library. The architecture is modular, with a clear separation of responsibilities across routing, proof verification, measurement whitelisting, and attestation key caching. Recent patches also introduced support for version 5 quote verification, as well as an important fix to the PCCS router.
Overall, the system correctly parses and verifies most quotes. During the review, it was identified that V4 SGX quotes were not being processed as intended. Some inefficiencies were also noted that, in certain edge cases, could create risks of accidental denial-of-service in administrative functions. In addition, the system’s security model rests on strong trust assumptions about the owner and authorized callers in each contract, underscoring the importance of carefully managing and safeguarding these roles. To further enhance the system, recommendations aimed at improving efficiency, clarity, and code standardization were also provided.
Overall, the codebase was found to be well-structured and thoughtfully designed. The Jovay team is appreciated for their responsiveness and in-depth technical engagement throughout the audit.