Skip to content

sherlock-audit/2024-08-morphl2

BranchesTags

Folders and files

NameName
Last commit message
Last commit date

Latest commit

 

History

5 Commits
.github/ISSUE_TEMPLATE
.github/ISSUE_TEMPLATE
 
 
morph
morph
 
 
.gitmodules
.gitmodules
 
 
README.md
README.md
 
 

Repository files navigation

MorphL2 contest details

Q&A

Q: On what chains are the smart contracts going to be deployed?

Ethereum, Morph L2

Q: If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of weird tokens you want to integrate?

Any ERC20 that meets the standards can use Standards ERC20 Gateway and CustomERC20Gateway. For other tokens with specific requirements (USDC), we will provide corresponding gateways

Q: Are there any limitations on values set by admins (or other roles) in the codebase, including restrictions on array lengths?

https://docs.google.com/spreadsheets/d/1KDQ8LkB53yw7f0m8a0LCTIdyiANjKyqC-sKUQEQGSf8/edit?usp=sharing

Q: Are there any limitations on values set by admins (or other roles) in protocols you integrate with, including restrictions on array lengths?

No

Q: For permissioned functions, please list all checks and requirements that will be made before calling the function.

Rollup.sol

  • function RevertBatch : Requires Batch data Incorrect or there is a successful challenge.
  • function setPause(bool _status) external onlyOwner: The batch can only be paused if it cannot be proved (to prevent the sequencer from being slashed if it does not do anything malicious) L1Staking.sol
  • function removeStaker(address[] memory _stakers) external onlyOwner：Ensure that the Sequencer can only be removed if they do not work properly for a long time (or other violations)

Q: Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification?

Rollup contracts on L1 is optionally compliant with EIP-4844

Q: Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?

gas oracle: interact with gasoralce.sol deployed on L2 to provide parameters used in Morph protocol staking oracle: interact with staking-related contracts on L2 to provide staking data

Q: Are there any hardcoded values that you intend to change before (some) deployments?

No

Q: If the codebase is to be deployed on an L2, what should be the behavior of the protocol in case of sequencer issues (if applicable)? Should Sherlock assume that the Sequencer won't misbehave, including going offline?

We use decentralized sequencers. We can assume majority (>2/3) sequencers won't misbehave. But it's possible sequencers go offline when we are doing a hard fork upgrade. Sherlock should assume the sequencer won't misbehave or go offline.

Q: Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

No

Q: Please discuss any design choices you made.

We use optimisitc zk-rollup approach to reduce the zkp cost of zk-rollup.

  • Since we currently lack reward distribution and the slash solution for the sequencers, the sequencers are not ready to open to public.
  • The bls signature verification in rollup contract has not been implemented due to the high gas cost. We plan to fix it when EIP-2537 is deployed on Ethereum, probably in Prague upgrade. The missing of signature verification process does not affect layer2 correctness since the validity is ensured by zkEVM. The only problem for now is that no method to ensure the submitted batch is generated by layer2 consensus, which is not a big issue since the sequencers are not open to public yet.
  • Submitter rotation is supported. Each sequencer will run a submitter
  • We only allow the whitelisted addresses to send the challenge request for now.

Q & A: https://docs.google.com/document/d/1lUr_Tu20FzOBB48wXAiQNJJGdB0Hl54xQs6euDEmmHg/edit?usp=sharing

Q: Please list any known issues and explicitly state the acceptable risks for each known issue.

  1. [Information] Contract owner can arbitrarily delete an unfinalized batch Acceptable risks: Any risk is acceptable

Target: morph/contracts/contracts/L1/rollup/Rollup.sol#L430–L457

Description: Unfinalized transaction batches can be reverted or deleted arbitrarily by the Rollup contract owner. The revertBatch function allows the owner to revert a sequence of batches as long as these batches are not finalized.

However, this raises some potential transaction censorship concerns, as valid unfinalized batches can be arbitrarily reverted by the contract owner even if no challenge has been initiated on them. Exploit Scenario Alice, the owner of the Rollup contract, reverts a series of batches by mistake. Note that these batches were valid and had no challenges. This prevents the finalization of the L2 chain, which prevents L1 withdrawals from completing.

Comments: To handle the rollback case, if a challenge succeeds, we need to check all the batches which are not finalized and decide how many batches to revert manually. The batches which have been finalized cannot be reverted (line 451)

  1. [Suggestion] Potential risks of not limiting the minimum FINALIZATION_PERIOD_SECONDS Acceptable risks: Any risk is acceptable

Issue: In the Rollup contract, the FINALIZATION_PERIOD_SECONDS parameter is used to determine the time for a batch to be finalized, which is a variable set by the owner through the updateFinalizePeriodSeconds function. In theory, the value of FINALIZATION_PERIOD_SECONDS should be equal to the finalization time of L1 blocks, so as to ensure that the data submitted from L2 to L1 is irreversible. However, in the updateFinalizePeriodSeconds function, there is no limit to the minimum value of FINALIZATION_PERIOD_SECONDS, meaning it can be set to a value much smaller than the finalization period of L1 blocks. If an L1 block reorganization or revert occurs, it will put the protocol at risk. Comments: we will set the value much larger (1 day in mainnet) than l1 finalization period

3 [Information] DoS risk when prevReplayIndex is too long Acceptable risks: Any risk is acceptable

Issue: In the L1CrossDomainMessenger contract, when a cross-chain message fails, users can discard the message and retrieve the corresponding funds through the dropMessage function. The drop operation will loop through dropCrossDomainMessage starting from prevReplayIndex[_lastIndex] until lastIndex is 0. If the lastIndex value is too large, it may lead to a DoS risk. In fact, as cross-chain messages accumulate, the messageNonce value will always keep increasing. When it accumulates to a huge value and a replayMessage operation is performed, prevReplayIndex[_nextQueueIndex] will start looping from a huge value, which will significantly increase the DoS risk.

Comments: In the design of replayMessage, users are only allowed to replay a message a maximum of MaxReplayTimes times. By default, MaxReplayTimes is set to 3. Generally, we do not anticipate changing this parameter under normal circumstances. 4 [Medium] Risk of not verifying cross-chain gas limit when executing messages Acceptable risks: Any risk is acceptable

Issue: In the protocol, when a user performs a sendMessage operation, they will specify a gasLimit so that this message can be executed on the corresponding chain with this gasLimit. However, when L2 executes a crosschain message from L1, it does not include this gasLimit. Similarly, when L1 executes a cross-chain message from L2, it does not include this gasLimit either. Additionally, when L2 sends a message to L1, it does not check the maximum/minimum value of the gasLimit passed in by the user. Therefore, if the gas required by the relayer to execute the user's message far exceeds the gasLimit provided by the user, it will cause a loss of funds for the relayer.

Comments: For the message from L1 to L2, the Sequencer node in L2 will convert it into an L2 transaction type and execute it. The GasLimit for this transaction type is determined by the GasLimit included in the L1 to L2 message. However, for the message from L2 to L1, it is executed by the user themselves in L1 and does not require the provision of a GasLimit parameter.

  1. Risk that the sequencer contract cannot be restarted Acceptable risks: Any risk is acceptable

Issue: In the Staking contract, users can request to withdraw their staked ETH through the withdrawETH function. At the same time, the protocol will remove the requester from the stakers list. When the length of the stakers list becomes 0, it means that all sequencers have exited the network. At this point, the staking contract will pause the sequencer contract. Unfortunately, for the sequencer contract, it can only perform an unpause operation at version 0. Therefore, after all sequencers exit the network, whitelisted users cannot restart the sequencer through the register function, and the sequencer contract will be permanently paused. This will lead to the protocol only being able to resolve the issue by deploying new staking and sequencer contracts, during which time the L2 may be down.

Comments: At present, BLS signatures and instant ZKP have not been implemented, and block generation must be done by the Sequencer. Once all Sequencers have exited, L2 will no longer be able to generate blocks, necessitating a hard fork upgrade. The L1 contract will also undergo an upgrade to recreate the SequencerSet. After the implementation of BLS signatures and instant ZKP, we will open the SequencerSet for permissionless joining, allowing others to join and continue operating in the event of all Sequencers exiting.

6 [Medium] Potential risks of cross-chain funds being locked Acceptable risks: Any risk is acceptable

Issue: In the L1CrossDomainMessenger contract, users can modify the gasLimit of a message that has already occurred but failed through the replayMessage function to re-execute the cross-chain message. However, there is a maxReplayTimes parameter to limit the maximum number of times a user can replay the message. Once this limit is exceeded, the message can never be executed again. However, the protocol does not handle messages that have exceeded the maximum number of replays, which will result in the user's funds being permanently locked in the protocol and unable to be withdrawn.

Comments: We will handle these error messages manually off-chain and strive to implement more effective error handling mechanisms in the future.

7 [Information] Reentrancy risk when L2 executes messages Acceptable risks: Any risk is acceptable unless it's high severity

Issue: In the L2CrossDomainMessenger contract, the counterpart role can execute cross-chain messages from L1 through the relayMessage function. It is important to note that it first executes the message through a call, and then sets the isL1MessageExecuted status of the message to true. If the counterpart role is an upgradeable contract, there will be a reentrancy risk, and the project team should remain vigilant. Comments: NO NEED CHANGE- it will not lead to fund loss

8 [Medium] Risk of over-privilege Acceptable risks: Any risk is acceptable

Issue: In the protocol, all contracts within the audit scope are upgradeable contracts, using the OpenZeppelin upgradeable model, which means there is an owner role that can upgrade the contracts. This will pose an excessive privilege risk. In the Staking contract, the owner role can add whitelisted users, and whitelisted users can stake to become sequencers. Additionally, the owner can modify the minimum stake amount through the updateParams function. Similarly, in the Rollup contract, the owner can modify the verifier address through the updateVerifier function. This will also lead to an excessive privilege risk for the owner.

Comments: NO NEED CHANGE Once the contract is deployed on the mainnet, administrative permissions, such as admin privileges, will be transferred to the Multisig for enhanced security and governance.

9 Potential for Invalid BLS Key Acceptable risks: Any risk is acceptable

Issue: A BLS public key for BLS12-381 with 128 bits of security should have a public key of either 96 bytes (which would result in a BLS signature of 48 bytes) or a public key of 48 bytes (which would result in a signature of 96 bytes). When registering BLS keys in the Staking contract, the only check for the size of the BLS key is that the length is equal to 256 bytes. While this ensures that someone cannot register with a BLS key exceeding the 256-byte limit, it does not prevent any malformed BLS keys with a length of less than 256 bytes. The absence of this check allows sequencers to accidentally register with BLS keys for a different curve or with a different amount of bits of security. If enough sequencers register with malformed BLS keys and this is sent to L2, it is possible that many of the sequencers will not be able to produce a valid signature, thus stalling the liveness of the L2 itself. While checking the appropriate length of the BLS does not preclude accounts from registering incorrect keys on purpose, it can reduce the number of accidental registries. Consider inserting a check when registering that the BLS key is of the appropriate length for the correct curve for the amount of security desired.

Comments: We enforce users to use G2-based key generation for public keys and G1-based generation for aggregate signatures. In other words, the public key consists of 96 bytes, while the aggregate signature consists of 48 bytes. However, in the precompiled contract implemented in Ethereum bls12-381, 256 bytes are used to represent points on G2 (source code), and 128 bytes are used to represent points on G1 (source code). Therefore, we enforce users to provide their public keys as the encoded 256-byte representation, indicating that they are generated based on G2.

In our Layer2 consensus client, we also provide the generation of BLS public keys based on the aforementioned rules, which corresponds to the implementation in the contract.

10 Malicious Sequencer Can Block Others from Challenging an Incorrect Batch Acceptable risks: Any risk is acceptable

Sequencers can submit new batches by calling the commitBatch function. This function only verifies that it was called by a sequencer and does some basic checks, such as ensuring the correct batch index and parent hash. Correctness of other data, such as a supplied withdrawalRoot , is not guaranteed by commitBatch , as challengers are able to challenge any batch, and if it is incorrect, the sequencer responsible for it will be slashed. If a batch is unchallenged until its finalization time, it will be finalized. Every time a batch is committed, its data is saved and the lastCommittedBatchIndex increased. If a batch is challenged, the finalization time of all remaining committed and unfinalized batches is increased in order to provide enough time to challenge them afterwards in case they are incorrect, as it is impossible to challenge multiple batches at the same time. This prevents a DoS attack where a sequencer submits two malicious batches and because one of them is being challenged, and only one batch can be challenged at a time, the second malicious batch will reach its finalization time without being challenged. However, any sequencer can exploit this safety mechanism in order to finalize a malicious batch. The attack involves calling commitBatch with a malicious batch and then committing a lot of fake batches afterwards, so that this for loop will achieve the block gas limit and will always revert with the Out Of Gas exception. This will block any attempt to call challengeState until the malicious batch is finalized. Consider limiting the number of batches that can be committed in a given period of time, so that it is always possible to challenge state, or redesigning the protocol so that it is possible to challenge multiple batches at the same time, so that the for loop in challengeState is no longer necessary. Comments: For now the challengers are whitelisted and they need deposit ETH to start a challenge

  1. claimReward may fail to execute Acceptable risks: Any risk is acceptable

L2Staking.sol - function claimReward, claimCommission /// @notice delegator claim reward /// @param delegatee delegatee address, claim all if empty /// @param targetEpochIndex up to the epoch index that the delegator wants to claim function claimReward(address delegatee, uint256 targetEpochIndex) external nonReentrant { if (delegatee == address(0)) { IDistribute(DISTRIBUTE_CONTRACT).claimAll(_msgSender(), targetEpochIndex); } else { IDistribute(DISTRIBUTE_CONTRACT).claim(delegatee, _msgSender(), targetEpochIndex); } }

Issue: If a delegator claims more than 60 epochs across all delegatees, the gas limit may be exceeded and the transaction may not be executed successfully.

Comments: we are preparing to optimize gas consumption or refactor the distribution reward model.

Q: We will report issues where the core protocol functionality is inaccessible for at least 7 days. Would you like to override this value?

We assume our sequencer can respond in 2 days to submit zk proof if the rollup contract is challenged (If there is DoS attack lasting more than 2 days, the sequencer can not submit the proof in time, this is a known issue.)

Q: Please provide links to previous audits (if any).

Last round: https://drive.google.com/file/d/1UnSL5lK6trUm5zP-4q51XC7JrsszBROr/view?usp=sharing

second round: https://drive.google.com/file/d/1AFNkZOiDC_kyEr5hyPTB-om4gymcL0Yp/view?usp=drive_link pw: 5jrs79YqgKhWFxCw first round: https://drive.google.com/file/d/1d-_MhIA0NIqGmRbO_GEcQByHNJJbFLBY/view?usp=drive_link

Q: Please list any relevant protocol resources.

https://docs.google.com/document/d/1KMOXBkgMXAGW3c25ZOvOhStZ3Vjl9KTJLfl4Rb86rkE/edit?usp=sharing

https://docs.morphl2.io/docs/build-on-morph/developer-navigation-page

Testnet is running, chain_id =2810 https://docs.morphl2.io/docs/quick-start/wallet-setup/ diff after tesnet https://github.com/morph-l2/morph/compare/v0.2.0-beta...main

Q: Additional audit information.

Diff after ToB audit: https://github.com/morph-l2/morph/compare/tob_audit...main The Morph L2 will not re-org by design, so issues of L2 re-org are invalid. But if there is any issue about Ethereum L1 re-org leading to financial loss, that issue is valid.

Audit scope

morph @ 22ca805e2d09c9d0bddb3e8a52ddd7d3435ce769

About

No description, website, or topics provided.

Resources

Readme
Activity
Custom properties

Stars

1 star

Watchers

0 watching

Forks

1 fork
Report repository

Releases

No releases published

Packages

No packages published

Contributors 2

  •  
  •  

Languages