Summary
Type: Layer 2 & Rollups
Timeline: September 10, 2025 → September 22, 2025
Languages: Solidity
Findings
Total issues: 43 (43 resolved)
Critical: 0 (0 resolved) · High: 2 (2 resolved) · Medium: 7 (7 resolved) · Low: 11 (11 resolved)
Notes & Additional Information
23 notes raised (23 resolved)
OpenZeppelin audited the jovaynetwork/jovay-contracts repository at commit 24f525f of the audit/mainnet/dev branch.
Following the conclusion of the fix review, the final post-audit commit is bdaf093.
In scope were the following files:
rollup_contracts
└── contracts
├── L1
│ ├── bridge
│ │ ├── L1BridgeProof.sol
│ │ ├── L1ERC20Bridge.sol
│ │ ├── L1ETHBridge.sol
│ │ └── interfaces
│ │ ├── IL1BridgeProof.sol
│ │ ├── IL1ERC20Bridge.sol
│ │ └── IL1ETHBridge.sol
│ ├── core
│ │ ├── L1Mailbox.sol
│ │ └── Rollup.sol
│ ├── interfaces
│ │ ├── IL1MailQueue.sol
│ │ ├── IL1Mailbox.sol
│ │ └── IRollup.sol
│ └── libraries
│ ├── codec
│ │ └── BatchHeaderCodec.sol
│ └── verifier
│ ├── ITeeRollupVerifier.sol
│ ├── IZkRollupVerifier.sol
│ └── WithdrawTrieVerifier.sol
│
├── L2
│ ├── bridge
│ │ ├── L2ERC20Bridge.sol
│ │ ├── L2ETHBridge.sol
│ │ └── interfaces
│ │ ├── IL2ERC20Bridge.sol
│ │ └── IL2ETHBridge.sol
│ ├── core
│ │ ├── L1GasOracle.sol
│ │ ├── L2CoinBase.sol
│ │ └── L2Mailbox.sol
│ ├── interfaces
│ │ ├── IClaimAmount.sol
│ │ ├── IL2MailQueue.sol
│ │ └── IL2Mailbox.sol
│ └── libraries
│ └── common
│ └── AppendOnlyMerkleTree.sol
│
└── common
├── BridgeBase.sol
├── ERC20Token.sol
├── MailBoxBase.sol
├── TokenBridge.sol
└── interfaces
├── IBridgeBase.sol
├── IERC20Token.sol
├── IGasPriceOracle.sol
├── IMailBoxBase.sol
└── ITokenBridge.sol
The Jovay Network is a rollup built with data availability and finality checkpoints on Ethereum. The core L1 component is the Rollup contract, which accepts batch commitments, verifies them against a supported proof system, and records the resulting state roots and message roots. Data availability for each batch is committed on L1 via blobs, following the EIP-4844 specification, and batch linkage is enforced through parent and rolling-hash fields.
Cross-domain messaging is provided by the L1Mailbox and L2Mailbox contracts. The L1 mailbox accumulates L1→L2 messages in a queue, while the L2 mailbox maintains an append-only historic Merkle tree of L2→L1 messages whose root is posted back to L1 during verification. After a batch is verified, messages can be executed on the destination chain with standard inclusion proofs. On L2, message execution is performed by a designated relayer.
Asset movement relies on paired ETH and ERC-20 bridges on L1 and L2. Deposits originate on L1 and are finalized on L2 once the corresponding message is relayed. Withdrawals originate on L2, are committed into the L2 message root, and are finalized on L1 after inclusion is proven. Token mappings are maintained symmetrically across layers to ensure consistent asset representation.
The current verification mode supports TEE proofs. Zero-Knowledge (ZK) verification hooks exist in the codebase but are not active. Governance and operational controls follow standard patterns: contracts are upgradeable and pausable, relayers are allowlisted, and administrative parameters can be updated by the owner to adapt the system while preserving the batch-commit, verify, and finalize lifecycle.
The system depends on trusted relayers and privileged contract owners to facilitate message passing and parameter management between L1 and L2. Correct operation requires these actors to behave honestly and reliably.
L1 → L2 Messages: No additional on-chain verification is performed. The relayer is solely responsible for honestly delivering the data. L2 → L1 Messages: Messages (submitted as batches of transactions) are verified on-chain. In the current version of the bridge, only TEE proofs are supported. For the purposes of this audit, it is assumed that TEE proof verification functions correctly.
In addition to ETH, the bridge supports ERC-20 token deposits. For the bridge to operate correctly, these must be standard ERC-20 tokens. The following are not supported and may cause reverts or unsafe behavior:
This audit was performed under the assumption that only standard ERC-20 tokens are used.
The specific responsibilities and assumptions for each role are as follows:
L1 and L2 Bridge Contract Owners
ERC-20 Bridge Contract Owner
L1Mailbox Owner
L1Mailbox contract.Rollup contract address, ensuring only valid contracts with the expected behavior are registered.gasLimit (which determines user deposit fees), withdrawer addresses (which can withdraw fees from the contract), and the baseFee used for computing user fees.latestQueueIndex, which determines the latest L2→L1 message in the queue. This must only be modified during mailbox upgrades.L2Mailbox Owner
L2Mailbox contract.Rollup Owner
Rollup contract, as well as proof verifier addresses.Rollup contract.L1GasOracle Owner
L2Coinbase Owner
Relayers
SentMsg event from the L1Mailbox (representing deposits) and relaying the exact same data to the L2Mailbox by calling relayMsg.Rollup contract on the L1 side.Rollup owner reverts the faulty batches. amount_ As ETH Value in L2 ERC-20 Bridge withdrawThe L2ERC20Bridge contract is responsible for handling deposits (L1 → L2) and withdrawals (L2 → L1) of ERC-20 tokens. When a user withdraws tokens, the bridge burns the specified token amount on L2 and then sends a cross-domain message to the L1 bridge via the L2Mailbox contract. This is achieved by calling the mailbox’s standard sendMsg function, which takes several parameters: the destination (toBridge), the ETH value to forward with the message (value), the encoded message data, the gas limit, and the sender's address. The value parameter here is intended to specify the amount of ETH (if any) that should accompany the message.
However, in the current implementation, the withdraw function mistakenly uses the ERC-20 token amount (amount_) as the value argument to the sendMsg function. This is incorrect because amount_ refers to the quantity of ERC-20 tokens being withdrawn, not ETH. For ERC-20 withdrawals, the ETH value should always be 0, since no ETH needs to be transferred with the message. If left uncorrected, this error can cause the mailbox to interpret the ERC-20 token amount as ETH, leading to inconsistent cross-domain accounting or potential ETH transfer attempts where no ETH was intended. This may result in loss of funds for the user or unexpected message execution failures on the L1 bridge side.
Consider updating the withdraw function so that the value passed to IMailBoxBase.sendMsg is 0 when withdrawing ERC-20 tokens. In addition, consider ensuring that only ETH withdrawals set a non-zero value.
Update: Resolved in pull request #18.
When an ERC-20 token is deposited in the L1ERC20Bridge contract, the contract checks whether the "mirror token" (its L2 counterpart)has been set. If so, the original tokens are transferred to the bridge contract and a message is emitted for relaying to the L2 side, where the corresponding amount of the mirror token is minted for the user. For this mechanism to work correctly, both sides of the bridge should maintain identical mappings between L1 tokens and their corresponding L2 counterparts.
On the L1 side, the bridge owner calls setTokenMapping, which stores a setTokenMapping message in the mailbox that is intended for the L2 side. Relayers are then expected to pass this message to the L2Mailbox contract, which subsequently calls the setTokenMapping of the L2ERC20Bridge contract. However, the setTokenMapping function in L2ERC20Bridge is currently only callable by the owner of the contract. The owner cannot be the mailbox contract, because in that case, it would be impossible to call some crucial functions (e.g., the pause function). As a result, the token mapping update fails on L2, leaving the two sides out of sync. Without consistent mappings, ERC-20 bridging is not possible.
Consider also allowing the L2Mailbox contract to call the L2ERC20Bridge.setTokenMapping function, ensuring that token mappings are properly propagated across both sides of the bridge.
Update: Resolved in pull request #24.
l2MsgRoot Across ProofsThe verifyBatch function of the Rollup contract allows relayers to perform an on-chain verification of a batch of L2 transactions that has been previously committed. To do so, the relayer provides the batch header, the state root after the batch, and the l2MsgRoot. In the current implementation, only one proof type is supported (TEE proofs). However, the system is designed to also support ZK proofs in the future, and a batch will only be considered verified once both proofs have been submitted and validated.
The contract enforces the consistency of the _postStateRoot across proofs by checking that finalizedStateRoots[_batchIndex] is either zero (for the first submitted proof) or equal to the value from the first proof (for the second proof). However, no such check is performed for the l2MsgRoot. As a result, the second submitted proof can overwrite it.
While it is true that if both proofs are valid, they should inherently agree on the l2MsgRoot since identical state roots imply identical included messages, but relying on this assumption weakens the intended security model. The rationale for requiring two distinct proofs is to increase robustness against bugs in a single proof system or verifier. If such a bug were to exist, it could allow both proofs to pass verification while producing different l2MsgRoot values. In that case, the two proofs would not actually be attesting the same messages, yet the system would fail to detect the inconsistency and would incorrectly accept the second value as accurate.
Consider explicitly checking that both proofs agree not only on the postStateRoot but also on the l2MsgRoot to strengthen the security guarantees.
Update: Resolved in pull request #24.
The L1 message queue in the L1Mailbox contract is difficult to reason about due to commented-out code paths and inconsistent state handling. The owner-settable setLastQueueIndex exists but is not coherently integrated with the rest of the queue flow. Meanwhile, getMsg appears to assume that old entries are popped, yet popFront() is commented out in popMsgs, so entries are never actually removed. In particular, this results in stableRollingHash never being updated. This can cause getMsg to return rolling hashes that do not match the true queue state (especially after manual updates to lastestQueueIndex or in the special case that it returns stableRollingHash, that is always outdated), creating ambiguity in how finalized L1 messages are tracked and risking downstream batch-commit inconsistencies.
Consider clearly documenting the intended queue lifecycle and enforcing it in the code. If messages are meant to be removed, reinstate popping in popMsgs and keep indices in sync. Otherwise, refactor getMsg to avoid relying on lastestQueueIndex. In addition, remove dead code or owner-only escape hatches that can desynchronize state.
Update: Resolved in pull request #34. The Jovay team stated:
We need to provide further clarification on this matter, as the scenarios for mainnet and testnet are distinct.
Testnet Scenario (Upgrade Path): A previous version of the contract is already deployed on the testnet. In this version, elements from
msgQueueare actually removed, and thesetLastQueueIndexfunction does not exist. Due to the performance impact of deleting elements frommsgQueue, we decided to modify the logic to no longer delete them. This change must be implemented via an upgrade. Consequently, we introduced thesetLastQueueIndexfunction.During the upgrade, the
setLastQueueIndexfunction will be called once, usinglastestQueueIndexto track the index of the next message to be finalized. It is important to note that the queue is expected to be empty before this call is made. Following the upgrade, the logic of thegetMsgfunction is also updated to calculate the index of the element to read frommsgQueuebased onlastestQueueIndex. We will elaborate on this process further in the upgrade documentation to prevent any ambiguity.Mainnet Scenario (Fresh Deployment): In contrast, the contract has not yet been deployed on mainnet. For the mainnet deployment, elements in
msgQueuewill never be deleted, andthe setLastQueueIndexfunction will not be called. As a result,lastestQueueIndexwill remain 0. Fetching data via the currentgetMsgfunction appears to work correctly under this clean-state condition.
Relayers are responsible for bridging messages between L1 and L2. On the L2 side, they submit messages to the L2Mailbox contract by calling the relayMsg function and providing the message details. This message can be a finalizeDeposit call for ETH or ERC-20 tokens, or a setTokenMapping invocation.
The relayMsg function stores the hash of the message in receiveMsgMap to prevent double submissions and then attempts to execute the action described in the message using a low-level call. If the low-level call fails, the transaction does not revert. This design allows ETH deposits to be finalized manually by the user, since the user can later finalize the transaction by calling claimDeposit.
In contrast, for setTokenMapping and finalizeDeposit for ERC-20 tokens, there does not exist a similar mechanism that would allow users to manually finalize the execution if the low-level call fails. Since the message hash is already stored in receiveMsgMap, the relayer cannot resubmit the message. In the case of setTokenMapping, the owner on the L1 side can resubmit the transaction that will create a new message that the relayer will transfer on the L2 side. However, the ERC-20 token deposit will remain incomplete, and deposited tokens will remain locked in the L1Mailbox contract without any tokens being delivered to the user on L2.
Consider adding a manual finalization function for ERC-20 deposits and setTokenMapping, similar to the claimDeposit function for ETH.
Update: Resolved in pull request #42. The Jovay team stated:
We have added failure-handling logic for ERC-20 token transfers. For the
setTokenMappingfunction, we think an in-contract failure-handling mechanism is unnecessary. If a transaction fails, the expected behavior is for the relayer to simply retry it.
layer2ChainId Can Break Batch Verification and Cause DoSThe layer2ChainId variable is initialized in initialize and can later be updated via setL2ChainId. Batches are committed through commitBatch, which records their hash in committedBatches. Later, verification occurs in verifyBatch, where layer2ChainId is passed to _verifyTeeProof as part of the commitment input. If layer2ChainId is updated after batches are committed but before they are verified, proof commitments will no longer match. Since verification requires sequential order (_batchIndex == _verifiedBatchIndex + 1), this mismatch halts progress entirely, introducing a DoS risk.
Consider binding the chain ID to each batch at commit time and referencing that stored value during verification. Alternatively, consider restricting setL2ChainId to only be callable when there are no pending unverified batches.
Update: Resolved in pull request #17.
The withdraw function of the L2ETHbridge contract enforces that msg.value is non-zero, but it does not verify that the amount_ argument, which determines the actual withdrawal amount, is also greater than 0. This allows a malicious user to call withdraw with a minimal non-zero msg.value while setting amount_ and gasLimit_ to zero. In such a call, the mailbox still emits a finalizeWithdraw message, and at the end of the execution, the attacker is refunded the full msg.value. The relayer must then include this message in a batch and subsequently submit and verify it on L1.
As a result, anyone with a small amount of native tokens on the L2 side can spam the system with a large number of zero-amount withdrawal messages, only paying the local L2 gas cost. This could result in a DoS condition, as the relayer would have to submit batches filled with spam withdrawal requests, making it significantly harder for legitimate withdrawals to be processed.
In the withdraw function, consider enforcing that the amount_ argument is also non-zero. In addition, consider requiring a minimum non-refundable gas fee to discourage spamming with economically worthless withdrawals.
Update: Resolved in pull request #15. The Jovay team stated:
Implementing a minimum withdrawal limit would require us to also add a minimum deposit limit to prevent users from getting their funds stuck. Therefore, we have adjusted our fix for M-04 to only ensure that
amount_is greater than 0.
L2MailboxWhen a user calls the withdraw function on the L2 side of the bridge, they must also specify the gasLimit. In the sendMsg function of the L2Mailbox contract, this parameter is used to compute the fee the user pays, which serves as a compensation for the relayers to cover the gas costs of bridging messages. Unlike the L1Mailbox contract, the L2Mailbox contract does not enforce any bounds on the gasLimit.
If the gasLimit is set to zero, the user avoids paying any fee while still creating valid withdrawal messages that relayers must later submit and verify on L1. An attacker could therefore spam the system at negligible cost, forcing relayers to process withdrawals without being compensated for them. Although relayers do not need to relay each individual transaction on L1 but only batches of transactions, which reduces the gas cost per transaction, the introduction of ZK proofs in the future, whose on-chain verification is not cheap, could make this problem significant.
Moreover, the issue becomes worse because users are allowed to include in a withdrawal a msg_ argument of type bytes and arbitrary length, which can increase significant the size of the blob. However, the fee estimation logic in L2Mailbox does not account for the length of the submitted data. The same issue also exists in L1Mailbox, in the case of deposits. In addition, the L2Mailbox contract does not include any mechanism to account for the total fees collected, such as the feeBalance variable in L1Mailbox, nor does it provide a withdrawal function for these fees. This results in untracked funds within the contract and no way for designated addresses to collect them.
Consider introducing bounds for the gasLimit in the L2Mailbox contract, making fees proportional to the length of the provided data and adding proper fee accounting along with a withdrawal function that is only callable by authorized addresses to ensure fair fee handling.
Update: Resolved in pull request #16. The Jovay team stated:
We need to clarify that the fees for a transaction initiated on L2—including the transaction fee and the cost of verification on L1—are all encompassed within the L2 transaction fee. Therefore, by design, the
L2Mailboxcontract itself does not handle any fees, and consequently, there is no need to implement fee-handling logic within it. To prevent fees on L2, we are simply settingbaseFeein theL2Mailboxcontract to zero. As a result, even when a user passes agasLimit_for a withdrawal, the calculated fee is nullified and effectively returned to users.
In finalizeDeposit, the implementation forwards gasleft() / 2 to the recipient. However, this approach is arbitrary and may leave too little gas for the callee. A safer and clearer pattern is to reserve a fixed, audited budget for post-call bookkeeping and forwarding the rest.
Consider replacing the gasleft() / 2 approach with a fixed, well-audited reserve pattern, forwarding the remainder to the target.
Update: Resolved in pull request #35.
ADMIN_ROLE Definition Instead of Using DEFAULT_ADMIN_ROLEThe ERC20Token contract introduces a custom ADMIN_ROLE and sets it as the admin for all roles. However, OpenZeppelin’s AccessControl already provides DEFAULT_ADMIN_ROLE with the same purpose. Defining a parallel ADMIN_ROLE creates ambiguity and increases the risk of errors, especially since DEFAULT_ADMIN_ROLE remains unassigned. By default, any new role in OpenZeppelin contracts has DEFAULT_ADMIN_ROLE as its admin, meaning those roles cannot be granted unless DEFAULT_ADMIN_ROLE is also held. This setup can lead to inconsistent privilege management and potential loss of control if new roles are introduced. Furthermore, explicitly redefining role admins is gas-inefficient.
Consider using the built-in DEFAULT_ADMIN_ROLE instead of introducing a custom ADMIN_ROLE to reduce the risk of errors and optimize gas consumption.
Update: Resolved in pull request #21.
The proper way to deposit ETH for bridging is by calling the deposit function of the L1ETHBridge contract. This function calls L1Mailbox.sendMsg, which transfers the deposited ETH to the L1Mailbox contract, where it remains locked until it is used to cover future withdrawals from L2 back to L1. However, the L1Mailbox contract inherits a receive() function from the MailboxBase contract, allowing users to send ETH directly to it. This behavior is error-prone and there are many incidents on other protocols where users without a proper understanding of the bridge logic, sent ETH directly to the contract, resulting in permanently lock funds. Similar issues exist in the ERC-20 version of the bridge.
On the L2 side, users may also send native tokens directly to the L2Mailbox contract instead of calling the withdraw function. While the receive() function cannot be removed entirely (since it is required for receiving freshly minted local ETH after deposits on the L1 side), it still creates confusion and risk for the users. In addition, the relayMsgWithProof functions of L1Bridge and L1Mailbox are marked as payable, even though there is no need for the users to send ETH when calling them. The ETH required for withdrawal is already held within the L1Mailbox contract. As such, declaring these functions payable increases the risk of accidental ETH transfers that serve no purpose.
Consider removing the receive() function from the L1Mailbox or overriding it with explicit intended functionality, only declaring as payable functions that truly require ETH transfers, making the mechanism for sending newly minted local ETH to the L2Mailbox more explicit and, in the ERC-20 version of the bridge, allowing recovery of tokens sent directly to the contract.
Update: Resolved in pull request #20 and pull request #46.
The bridge owner uses the setTokenMapping function to define which ERC-20 tokens are permitted for deposits on the L1 side of the bridge and their corresponding tokens on L2. The bridge will only function correctly if both sides maintain identical mappings at all times.
However, the setTokenMapping function can also be used to change the corresponding L2 token for an L1 token. This flexibility introduces a risk. If a deposit is initiated before a mapping update, but the relayer submits the message only after the update has been applied on both sides, the finalizeDeposit function will validate against the new mapping instead of the original one. In such a scenario, the deposit cannot be finalized, leaving the bridging process stuck.
Consider either not allowing the setTokenMapping function to update existing token's counterpart or, if updates are required, designing a careful and explicit procedure for updates, such as pausing both sides of the bridge before applying any change.
Update: Resolved in pull request #20. The Jovay team stated:
The
relayMsgmechanism on L2 ensures that L1 messages are executed on L2 in nonce order without causing disorder. So, this problem will not occur.
In the sendMsg function of the L1Mailbox contract, the user-provided gasLimit_ is required to be strictly less than the l2GasLimit and greater than or equal to l2FinalizeDepositGasUsed. During initialization, as well as in the setL2GasLimit and setL2FinalizeDepositGasUsed owner-only callable functions, the contract only checks that l2GasLimit is not strictly less than l2FinalizeDepositGasUsed. This means that it is acceptable for the owner to set these two values as equal. If such a configuration occurs, there will be no valid gasLimit_ values that satisfy the conditions in sendMsg, which would effectively block all deposits from being executed.
Consider tightening the logic in the setter functions by enforcing that l2GasLimit is strictly greater than l2FinalizeDepositGasUsed.
Update: Resolved in pull request #36.
L1Mailbox and L2MailboxThe sendMsg functions in both L1Mailbox and L2Mailbox are similar in logic and each emits a SentMsg event. However, the event arguments are not aligned between the two contracts. In the L1Mailbox contract, the fifth argument is data, whereas in the L2Mailbox contract, it is just msg_. Similarly, the first argument in AppendMsg emitted by L1Mailbox represents the next index, while in the L2Mailbox contract, it represents the actual index where the message was appended. This inconsistency may cause confusion for users and protocols that integrate with the bridge contracts and will also complicate the relayers' job, as it requires different decoding logic for similar events.
Consider aligning the structure and semantics of events emitted by the two mailbox contracts to ensure consistency.
Update: Resolved. The Jovay team stated:
The messages on both sides serve different purposes: L1 requires data to serve as transaction information sent to L2, while the messages in the L2 mailbox are solely for message processing.
initialize for lastBatchByteLengthThe L1GasOracle contract relies on the relayer-provided lastBatchByteLength value to compute the L1 gas per byte. While both upper and lower bounds are enforced in setNewBatchBlobFeeAndTxFee, the initialize function only enforces the lower bound.
Consider adding an upper-bound check in initialize.
Update: Resolved in pull request #37.
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, multiple instances of floating pragma directives were identified:
L1BridgeProof.sol has the solidity ^0.8.0floating pragma directive.L1ERC20Bridge.sol has the solidity ^0.8.0 floating pragma directive.L1ETHBridge.sol has the solidity ^0.8.0 floating pragma directive.IL1BridgeProof.sol has the solidity ^0.8.0 floating pragma directive.IL1ERC20Bridge.sol has the solidity ^0.8.0 floating pragma directive.IL1ETHBridge.sol has the solidity ^0.8.0 floating pragma directive.L1Mailbox.sol has the solidity ^0.8.0 floating pragma directive.Rollup.sol has the solidity ^0.8.0 floating pragma directive.IL1MailQueue.sol has the solidity ^0.8.0 floating pragma directive.IL1Mailbox.sol has the solidity ^0.8.0 floating pragma directive.IRollup.sol has the solidity ^0.8.16 floating pragma directive.BatchHeaderCodec.sol has the solidity ^0.8.16 floating pragma directive.ITeeRollupVerifier.sol has the solidity ^0.8.24 floating pragma directive.IZkRollupVerifier.sol has the solidity ^0.8.24 floating pragma directive.WithdrawTrieVerifier.sol has the solidity ^0.8.24 floating pragma directive.L2ERC20Bridge.sol has the solidity ^0.8.0 floating pragma directive.L2ETHBridge.sol has the solidity ^0.8.0 floating pragma directive.IL2ERC20Bridge.sol has the solidity ^0.8.0 floating pragma directive.IL2ETHBridge.sol has the solidity ^0.8.0 floating pragma directive.L1GasOracle.sol has the solidity ^0.8.0 floating pragma directive.L2CoinBase.sol has the solidity ^0.8.0 floating pragma directive.L2Mailbox.sol has the solidity ^0.8.0 floating pragma directive.IClaimAmount.sol has the solidity ^0.8.0 floating pragma directive.IL2MailQueue.sol has the solidity ^0.8.0 floating pragma directive.IL2Mailbox.sol has the solidity ^0.8.0 floating pragma directive.AppendOnlyMerkleTree.sol has the solidity ^0.8.24 floating pragma directive.BridgeBase.sol has the solidity ^0.8.0 floating pragma directive.ERC20Token.sol has the solidity ^0.8.0 floating pragma directive.MailBoxBase.sol has the solidity ^0.8.0 floating pragma directive.TokenBridge.sol has the solidity ^0.8.0 floating pragma directive.IBridgeBase.sol has the solidity ^0.8.0 floating pragma directive.IERC20Token.sol has the solidity ^0.8.0 floating pragma directive.IGasPriceOracle.sol has the solidity ^0.8.0 floating pragma directive.IMailBoxBase.sol has the solidity ^0.8.0 floating pragma directive.ITokenBridge.sol has the solidity ^0.8.0 floating pragma directive.Consider using fixed pragma directives.
Update: Resolved in pull request #38.
For each ERC-20 deposited on L1, an equivalent amount of the bridged token must be minted on L2. Conversely, when the bridged token is withdrawn on L2, the corresponding amount of ERC-20 is released on L1. Preserving this one-to-one correspondence between the two sides is a critical invariant of the system.
For each ERC-20 token that is allowed to be deposited on L1, a mirror token is deployed on L2 (ERC20Token contract) with access-controlled mint and burn functions. The corresponding L2ERC20Bridge contract is expected to hold the MINTER_ROLE and BURNER_ROLE. However, the ERC20Token contract exposes multiple burn functions (a burn for burning tokens from the caller, a burn callable only by the BURNER_ROLE, that can burn tokens from any address without needing any approval, and a burnFrom function that allows the MINTER_ROLE to burn tokens from an address that has previously given approval). Only the second one is used by the L2ERC20Bridge contract.
If additional addresses are granted burner rights, they could burn tokens directly from user accounts. This would desynchronize balances between the L1 and L2 sides (breaking an important invariant) and also between the actual balance of the L2 token and its balance as it is tracked by the L2ERC20Bridge contract. This desynchronization will block bridging operations, because in withdraw, it is checked that the two balances are equal.
Consider removing the extra burn and burnFrom functions and granting minter and burner roles exclusively to the L2ERC20Bridge contract.
Update: Resolved in pull request #46 at commit e5c60e3.
The Solidity ABI requires 0x40 (free-memory pointer) to remain 32-byte aligned. However, in the loadAndValidate function of the BatchHeaderCodec contract, after copying the header, the code updates the pointer with mstore(0x40, add(batchPtr, length)), but length is 105, resulting in a value that is not divisible by 32. Further Solidity allocations will therefore start at an odd address and may unexpectedly clobber previously written data or lead to out-of-bounds reads, breaking compiler assumptions and potentially causing undefined behavior or reverts down the line.
Furthermore, each of the store functions of the contract writes directly to memory using inline assembly but does not update the free memory pointer. While their usage within Rollup is safe (since the free memory pointer is adjusted in advance), relying on them in other contexts could lead to memory corruption.
Consider increasing the free memory pointer in loadAndValidate by 128 (the nearest multiple of 32 above 105) to maintain alignment with Solidity compiler expectation and avoid using the store functions outside the Rollup contract.
Update: Resolved in pull request #46 and pull request #57.
When operations with address parameters are performed, it is crucial to ensure the address is not set to zero. Setting an address to zero is problematic because it has special burn/renounce semantics. This action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.
Throughout the codebase, multiple instances of missing zero-address checks were identified:
target_ operation within the contract L1Mailbox in L1Mailbox.soltarget_ operation within the contract L1Mailbox in L1Mailbox.sol_target operation within the contract L1Mailbox in L1Mailbox.sol_l1_mail_box operation within the contract Rollup in Rollup.soll2Token_ operation within the contract L2ERC20Bridge in L2ERC20Bridge.solto_ operation within the contract L2ETHBridge in L2ETHBridge.sol_l2EthBridge operation within the contract L2CoinBase in L2CoinBase.solrefundAddress_ operation within the contract L2Mailbox in L2Mailbox.soladmin address in the contract ERC20Token in ERC20Token.solConsider always performing a zero-address check before assigning a state variable.
Update: Resolved in pull request #40.
When a relayer commits a batch in the Rollup contract, its data hash is computed using the _getBlobDataHash function. This function iterates through all blobs and stops when it reaches a zero blob hash, which indicates that no further blobs are present. However, for the first blob, if it is empty, the function does not revert. Instead, it produces the keccak256 hash of the empty string. Although relayers are expected to be honest and there is little incentive for submitting empty batches, it would be better to avoid it.
Consider modifying the implementation so that committing to empty batches reverts.
Update: Resolved in pull request #41.
When a withdrawal is initiated on the L2 side, the relayer includes it in a batch that is later relayed to L1. To finalize the withdrawal on L1, the user must provide a proof that their finalizeWithdraw message is indeed part of a batch. This verification relies on Merkle tree proofs, and a custom library has been developed for this purpose.
While the integration of this library with the bridge contracts is secure, the library itself lacks important safety checks typically expected in Merkle proof verification (e.g., ensuring that the provided proof length has the correct size for the given Merkle tree). Without such checks, it is possible to construct proofs for intermediate tree nodes rather than just for the leaves of the tree.
Consider documenting the limitations of the custom Merkle tree library and be aware that reusing it outside the current integration may introduce risks.
Update: Resolved in pull request #46.
withdrawAllIn the L2CoinBase contract, the withdrawAll function calls withdraw and both functions have an onlyWithdrawer modifier. As a result, the access-control check is applied twice during every call of withdrawAll.
Consider removing the modifier from withdrawAll to reduce gas consumption.
Update: Resolved in pull request #46.
The L2CoinBase contract inherits from OpenZeppelin's OwnableUpgradeable, PausableUpgradeable, and ReentrancyGuardUpgradeable contracts. However, in its initialize function, it only calls _Ownable_init and does not initialize the other two parent contracts. As a result, the _status variable of the ReentrancyGuardUpgradeable contract is left at its default value of 0 instead of being set to _NOT_ENTERED (1), as intended. While this creates an inconsistency, it will not affect the contract in practice, and will be fixed after the first call of a function with a nonReentrant modifier.
Consider explicitly initializing all parent contracts to ensure consistent and predictable contract state.
Update: Resolved in pull request #46.
rollupTimeLimit to 32 bitsWithin Rollup.sol, in line 355, the storage variable rollupTimeLimit is declared as uint64, but the setter accepts a uint32 parameter:
function setRollupTimeLimit(uint32 _rollupTimeLimit) external onlyOwner {
rollupTimeLimit = _rollupTimeLimit;
}
Values above 4,294,967,295 cannot be provided, silently capping the configurable time-limit to 32 bits and creating an arbitrary, undocumented restriction.
Consider aligning the uint sizes for consistency.
Update: Resolved in pull request #46.
setZkVerifierAddressWithin Rollup.sol, in line 373, the parameter description says "The verifier address of tee." even though the function actually sets the ZK verifier address. This documentation error can mislead developers and reviewers.
Consider correcting this documentation error.
Update: Resolved in pull request #46.
Within ERC20Token.sol, TRANSFER_ROLE is defined alongside the other role constants, yet no function in the contract checks or assigns it.
Consider removing this role declaration.
Update: Resolved in pull request #46.
Throughout the codebase, multiple instances of unused state variables were identified:
Rollup.sol, the maxTxsInChunk state variableRollup.sol, the maxBlockInChunk state variableRollup.sol, the maxCallDataInChunk state variableRollup.sol, the l1BlobNumberLimit state variableRollup.sol, the rollupTimeLimit state variableTo improve the overall clarity and intent of the codebase, consider removing any unused state variables.
Update: Resolved. The Jovay team stated:
These
publicstate variables are used by the relayer, so we cannot remove them.
Throughout the codebase, multiple instances of unused imports were identified:
import "./IL1BridgeProof.sol"; in IL1ERC20Bridge.sol.import "../../../common/interfaces/ITokenBridge.sol"; in IL1ERC20Bridge.sol.import "./IL1BridgeProof.sol"; in IL1ETHBridge.sol.import {L2Mailbox} from "../core/L2Mailbox.sol"; in L2ETHBridge.sol.Consider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #46.
The use of non-explicit imports in the codebase can decrease code clarity and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity file or when inheritance chains are long.
Throughout the codebase, multiple instances of non-explicit imports were identified:
L1BridgeProof.solL1ERC20Bridge.solL1ERC20Bridge.solL1ETHBridge.solL1ETHBridge.solL1Mailbox.solL1Mailbox.solRollup.solRollup.solL2ERC20Bridge.solL2ERC20Bridge.solL2ETHBridge.solL2CoinBase.solFollowing the principle that clearer code is better code, consider using the named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.
Update: Resolved in pull request #46.
++i) Can Save Gas in LoopsThroughout the codebase, multiple opportunities where the subject optimization can be applied were identified:
Consider using the prefix-increment operator (++i) instead of the postfix-increment operator (i++) in order to save gas. This optimization skips storing the value before the incremental operation, as the return value of the expression is ignored.
Update: Resolved in pull request #46.
Function modifiers should be ordered as follows: visibility, mutability, virtual, override, and custom modifiers.
Throughout the codebase, multiple instances of functions having an incorrect order of modifiers were identified:
setTokenMapping function in L1ERC20Bridge.soldeposit function in L1ERC20Bridge.solsetNewBatchBlobFeeAndTxFee function in L1GasOracle.solsetBlobBaseFeeScalaAndTxFeeScala function in L1GasOracle.solsetL1Profit function in L1GasOracle.solsetTotalScala function in L1GasOracle.solsetMaxL1ExecGasUsedLimit function in L1GasOracle.solsetMaxL1BlobGasUsedLimit function in L1GasOracle.soladdRelayer function in L1GasOracle.solremoveRelayer function in L1GasOracle.solsetL2EthBridge function in L2CoinBase.soladdWithdrawer function in L2CoinBase.solremoveWithdrawer function in L2CoinBase.soladdWhiteAddress function in L2CoinBase.solremoveWhiteAddress function in L2CoinBase.solwithdraw function in L2CoinBase.solwithdrawAll function in L2CoinBase.solsetL1MailBox function in L2Mailbox.solsetTokenMapping function in ITokenBridge.solTo improve the project's overall legibility, consider reordering the modifier order of functions as recommended by the Solidity Style Guide.
Update: Resolved in pull request #46.
When state variables use public visibility in a contract, a getter method for the variable is automatically included.
Throughout the codebase, multiple instances of redundant getter functions were identified:
L1Mailbox contract in L1Mailbox.sol, the nextMsgIndex function is redundant because the pendingQueueIndex state variable already has a getter.Rollup contract in Rollup.sol, the getL2MsgRoot function is redundant because the l2MsgRoots state variable already has a getter.To improve the overall clarity, intent, and readability of the codebase, consider removing the redundant getter functions.
Update: Resolved in pull request #46. The Jovay team stated:
getL2MsgRootis used by other components. So, while we will not remove it, we have added the relevant documentation.
Since Solidity 0.8.18, mappings can include named parameters to provide more clarity about their purpose. Named parameters allow mappings to be declared in the form mapping(KeyType KeyName? => ValueType ValueName?). This feature enhances code readability and maintainability.
Throughout the codebase, multiple instances of mappings without named parameters were identified:
committedBatches state variable in the Rollup contractfinalizedStateRoots state variable in the Rollup contractl2MsgRoots state variable in the Rollup contractl1MsgCount state variable in the Rollup contractisRelayer state variable in the Rollup contractisRelayer state variable in the L1GasOracle contractisWithdrawer state variable in the L2CoinBase contractwhiteListOnL1 state variable in the L2CoinBase contractreceiveMsgStatus state variable in the L2Mailbox contractsendMsgMap state variable in the MailBoxBase contractreceiveMsgMap state variable in the MailBoxBase contractisBridge state variable in the MailBoxBase contracttokenMapping state variable in the TokenBridge contractbalanceOf state variable in the TokenBridge contractConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #46.
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.
Throughout the codebase, multiple instances of contracts not having a security contact were identified:
L1ERC20Bridge contractL1ETHBridge contractL1GasOracle contractL1Mailbox contractRollup contractL2ERC20Bridge contractL2ETHBridge contractL2CoinBase contractL2Mailbox contractConsider 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 #46.
Throughout the codebase, multiple instances of literal values with unexplained meanings were identified:
1000000 literal number in L1ERC20Bridge.sol100 literal number in Rollup.sol1e6 literal number in L1GasOracle.sol6 literal number in L1GasOracle.sol128 literal number in L1GasOracle.sol1024 literal number in L1GasOracle.sol110 literal number in L1GasOracle.sol100 literal number in L1GasOracle.sol100 literal number in L1GasOracle.sol100 literal number in L1GasOracle.sol100 literal number in L1GasOracle.sol100 literal number in L1GasOracle.solConsider defining and using constant variables instead of using literals to improve the clarity and maintainability of the codebase.
Update: Resolved in pull request #46.
public Function or Variable Prefixed With UnderscoreAs per the Solidity style convention, public functions and variables should not be prefixed with an underscore.
In AppendOnlyMerkleTree.sol, the _branches variable is public but has been prefixed with an underscore.
Consider removing the underscore prefix from the identifiers of public functions and variables.
Update: Resolved in pull request #46.
The IClaimAmount.sol file name does not match the IClaim contract name.
To make the codebase easier to understand for developers and reviewers, consider renaming the files to match the contract names.
Update: Resolved. The Jovay team stated:
We have removed this file as it was unused.
Within AppendOnlyMerkleTree.sol, the append routine can be invoked even if the zero-hash cache has not been pre-computed. The guard statement that should enforce initialization is currently commented out. Therefore, a caller can execute _appendMsgHash with every value in _zeroHashes still equal to 0. While the function will seemingly work, every internal node that is computed will be mixed with a zero value instead of the canonical hash of zero.
Once the tree is later initialized, the cached zero values change, making all previously stored branches and the exposed _msgRoot mathematically incorrect. All subsequent consistency checks or proof verifications that rely on the Merkle root will fail permanently. Since the function has been declared as internal, the bug will surface in any inheriting contract that does not call _initializeMerkleTree first. In addition, _initializeMerkleTree never explicitly assigns _zeroHashes[0] = bytes32(0), instead relying on the default value. This omission reduces clarity and increases the likelihood of misuse.
Consider enforcing initialization by re-enabling the guard check and explicitly assigning _zeroHashes[0] = bytes32(0) during _initializeMerkleTree to avoid ambiguity and potential misuse.
Update: Resolved in pull request #46.
In the _transferERC20 function of the L1ERC20Bridge contract, there is a comment suggesting that the design allows for the handling of fee-on-transfer tokens. The design flow is as follows:
_amount using safeTransferFrom. A fee-on-transfer token may deliver lass than amount_ to the contract.amount_.However, for tokens with fees on transfer, the actual amount received will be less that amount_, and therefore the actual balance will be less that the tracked one. As a result, the require statement enforcing the balance check will fail for such tokens, making them unsupported in practice.
Consider adjusting the implementation to correctly handle the reduced received amount for tokens with fees. Alternatively, consider removing the comment entirely if the protocol does not plan to support such tokens.
Update: Resolved in pull request #46. The Jovay team stated:
We do not support fee-on-transfer tokens, currently.
Initialize Emits Two OwnershipTransferred EventsWithin BridgeBase.sol, initialize first calls OwnableUpgradeable.__Ownable_init(), which unconditionally sets the owner to _msgSender() (the account performing the initialization). A few lines later, the code executes _transferOwnership(owner) to hand over control to the owner argument supplied by the caller. This pattern produces two OwnershipTransferred events in the same transaction, which can confuse off-chain indexers and on-chain logic that relies on the first event as the definitive owner.
A cleaner pattern would be to set the final owner only once. In fact, the reliance on outdated OpenZeppelin upgradeable libraries (v4.x) makes this clumsier than necessary: __Ownable_init() in v4.x always sets the owner to msg.sender, while OpenZeppelin v5.x introduces __Ownable_init(address initialOwner) to set the desired owner directly in a single step.
Consider upgrading the OpenZeppelin dependencies across the codebase to the latest version and using __Ownable_init(address initialOwner) in initializers to avoid the double ownership transfer.
Update: Resolved in pull request #59.
Throughout the codebase, there are several functions that either emit unnecessary events or fail to emit an event despite modifying important system parameters.
The removeRelayer of the L1GasOracle contract flips the mapping entry to false without checking that _oldRelayer is currently marked true. The owner can therefore "remove" any arbitrary address—including one that was never a relayer or the zero address—while still emitting RemoveRelayer. Off-chain indexers relying on events will record a state transition that never actually happened, causing data inconsistencies. Similar issues arise in MailboxBase::removeBridge.
The following functions update the state without an event emission:
initialize function in L1Mailbox.solsetRollup function in L1Mailbox.solsetWithdrawer function in L1Mailbox.solwithdrawDepositFee function in L1Mailbox.solsetLastQueueIndex function in L1Mailbox.solinitialize function in Rollup.solrevertBatches function in Rollup.solrevertBatches function in Rollup.soladdRelayer function in Rollup.solremoveRelayer function in Rollup.solsetMaxTxsInChunk function in Rollup.solsetMaxBlockInChunk function in Rollup.solsetMaxCallDataInChunk function in Rollup.solsetL1BlobNumberLimit function in Rollup.solsetRollupTimeLimit function in Rollup.solsetL2ChainId function in Rollup.solsetTeeVerifierAddress function in Rollup.solsetZkVerifierAddress function in Rollup.solwithdraw function in L2ERC20Bridge.solwithdraw function in L2ERC20Bridge.solfinalizeDeposit function in L2ERC20Bridge.solclaimDeposit function in L2ETHBridge.solclaimDeposit function in L2ETHBridge.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solinitialize function in L1GasOracle.solCalcL1FeePerByte function in L1GasOracle.solsetNewBatchBlobFeeAndTxFee function in L1GasOracle.solsetBlobBaseFeeScalaAndTxFeeScala function in L1GasOracle.solsetL1Profit function in L1GasOracle.solsetTotalScala function in L1GasOracle.solsetMaxL1ExecGasUsedLimit function in L1GasOracle.solsetMaxL1BlobGasUsedLimit function in L1GasOracle.soladdRelayer function in L1GasOracle.solremoveRelayer function in L1GasOracle.solinitialize function in L2CoinBase.soladdWithdrawer function in L2CoinBase.solremoveWithdrawer function in L2CoinBase.soladdWhiteAddress function in L2CoinBase.solremoveWhiteAddress function in L2CoinBase.solwithdraw function in L2CoinBase.solwithdrawAll function in L2CoinBase.solinitialize function in L2Mailbox.solinitialize function in L2Mailbox.solinitialize function in L2Mailbox.solsetL1MailBox function in L2Mailbox.solinitialize function in BridgeBase.solsetMailBox function in BridgeBase.solsetToBridge function in BridgeBase.soladdBridge function in MailBoxBase.solremoveBridge function in MailBoxBase.solConsider emitting events whenever there are state changes and checking that the state has been actually changed before emitting an event. Doing so will help improve the clarity of the codebase and make it less error-prone.
Update: Resolved in pull request #46.
During development, having well described TODO/Fixme comments will make the process of tracking and solving them easier. However, left unaddressed, these comments might age and important information for the security of the system might be forgotten by the time it is released to production. As such, these comments should be tracked in the project's issue backlog and resolved before the system is deployed.
Throughout the codebase, multiple instances of TODO/Fixme comments were identified:
L1ERC20Bridge.solL1ETHBridge.solRollup.solL2ERC20Bridge.solL2ETHBridge.sol.Consider removing all instances of TODO/Fixme comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO/Fixme comment to the corresponding issues backlog entry.
Update: Resolved in pull request #46 and pull request #53.
calldata Instead of memoryWhen dealing with the parameters of external functions, it is more gas-efficient to read their arguments directly from calldata instead of storing them to memory. calldata is a read-only region of memory that contains the arguments of incoming external function calls. This makes using calldata as the data location for such parameters cheaper and more efficient compared to memory. Thus, using calldata in such situations will generally save gas and improve the performance of a smart contract.
Throughout the codebase, multiple instances where function parameters should use calldata instead of memory were identified:
L1BridgeProof.sol, the msg_ parameterL1BridgeProof.sol, the proof_ parameterL1ERC20Bridge.sol, the msg_ parameterL1ERC20Bridge.sol, the msg_ parameterL1ETHBridge.sol, the msg_ parameterL1ETHBridge.sol, the msg_ parameterL1Mailbox.sol, the msg_ parameterL1Mailbox.sol, the proof_ parameterL2ERC20Bridge.sol, the msg_ parameterL2ETHBridge.sol, the msg_ parameterConsider using calldata as the data location for the parameters of external functions to optimize gas usage.
Update: Resolved in pull request #46.
The scope of this audit covered the Bridge contracts, which are designed for deployment on both the L1 and L2 networks. Two versions of these contracts were reviewed: one supporting ETH bridging and another for bridging ERC-20 tokens. The Rollup contract was also audited, which will be deployed on the L1 and is the contract where relayers will post batches of L2 transactions along with their proofs. Several complementary contracts used for verifying Merkle proofs, estimating L1 gas fees on the L2 side, and transferring the collected fees were also reviewed.
The review identified 2 high-severity, 7 medium-severity, and several lower-severity issues. These findings provide constructive insights that can help further strengthen the system’s robustness, consistency, and maintainability.
Two distinct proof systems are in the process of being implemented. This indicates that the design is still evolving and will benefit from a thorough follow-up audit as development progresses.
The Jovay team was highly responsive throughout the engagement, addressing questions promptly and showing a strong commitment to strengthening the security of their codebase. We would like to thank them for their close collaboration and constructive engagement.