We audited three pull requests with the op-node, op-batcher, op-proposer, op-chain-ops, and op-service components being in scope:
Pull request #72 from the base commit at 68f2533 and the head commit at bb0ff70. In scope were the following files:
mantle-v2
├── op-batcher
│ ├── batcher
│ │ ├── batch_submitter.go
│ │ ├── channel_manager.go
│ │ ├── config.go
│ │ ├── driver_da.go
│ │ └── driver.go
│ ├── common
│ │ ├── types.go
│ │ └── utils.go
│ ├── flags
│ │ └── flags.go
│ └── metrics
│ ├── metrics.go
│ └── noop.go
├── op-chain-ops
│ ├── cmd
│ │ ├── check-migration
│ │ │ └── main.go
│ │ ├── op-migrate
│ │ │ └── main.go
│ │ ├── rollover
│ │ │ └── main.go
│ │ └── withdrawals
│ │ └── main.go
│ ├── crossdomain
│ │ ├── encoding.go
│ │ ├── hashing.go
│ │ ├── legacy_abi.go
│ │ ├── legacy_withdrawal.go
│ │ ├── message.go
│ │ ├── migrate.go
│ │ ├── params.go
│ │ ├── withdrawal.go
│ │ ├── withdrawals.go
│ │ └── witness.go
│ ├── eof
│ │ └── eof_crawler.go
│ ├── ether
│ │ ├── addresses.go
│ │ ├── cli.go
│ │ ├── migrate.go
│ │ └── storage.go
│ ├── genesis
│ │ ├── check.go
│ │ ├── config.go
│ │ ├── db_migration.go
│ │ ├── genesis.go
│ │ ├── layer_one.go
│ │ ├── layer_two.go
│ │ └── setters.go
│ ├── immutables
│ │ └── immutables.go
│ └── util
│ └── util.go
├── op-node
│ ├── chaincfg
│ │ └── chains.go
│ ├── cmd
│ │ └── genesis
│ │ └── cmd.go
│ ├── eth
│ │ ├── sync_status.go
│ │ └── types.go
│ ├── flags
│ │ └── flags.go
│ ├── metrics
│ │ └── metrics.go
│ ├── node
│ │ ├── config.go
│ │ └── node.go
│ ├── rollup
│ │ ├── da
│ │ │ └── datastore.go
│ │ ├── derive
│ │ │ ├── attributes.go
│ │ │ ├── calldata_source.go
│ │ │ ├── channel_bank.go
│ │ │ ├── channel_in_reader.go
│ │ │ ├── deposit_log.go
│ │ │ ├── engine_consolidate.go
│ │ │ ├── engine_queue.go
│ │ │ ├── error.go
│ │ │ ├── frame.go
│ │ │ ├── l1_block_info.go
│ │ │ ├── l1_retrieval.go
│ │ │ ├── l2block_util.go
│ │ │ ├── payload_util.go
│ │ │ ├── pipeline.go
│ │ │ └── system_config.go
│ │ ├── driver
│ │ │ ├── driver.go
│ │ │ └── state.go
│ │ ├── sync
│ │ │ ├── config.go
│ │ │ └── start.go
│ │ └── types.go
│ ├── service.go
│ ├── service_mantle.go
│ ├── sources
│ │ └── sync_client.go
│ └── withdrawals
│ └── utils.go
└── op-service
├── crypto
│ └── signature.go
└── txmgr
└── cli.go
Pull request #89 from the base commit at 5e3886c and the head commit at 76959dd. In scope were the following files:
mantle-v2
└── op-node
└── rollup
└── derive
├── deposit_log.go
└── l1_block_info.go
Pull request #98 from the base commit at 0f0861b and the head commit at 365f02a. In scope were the following files:
mantle-v2
└── op-chain-ops
├── genesis
│ └── config.go
└── immutables
└── immutables.go
Mantle V2 is a layer 2 (L2) scaling solution for Ethereum that uses fraud proofs instead of validity proofs for its security. The protocol aims to provide low transaction fees and high throughput while maintaining full EVM compatibility. Mantle V2 is built on top of Ethereum using the OP Stack and therefore shares many similarities with Optimism. The op-node, op-batcher, and op-proposer components collectively comprise the consensus layer that keeps the chain running and glues all the other layer 1 and layer 2 components together (though there is no consensus as per the Ethereum understanding). Adding op-geth to this set, which is the execution layer, makes up the Mantle network.
The op-node component is responsible for deriving the layer 2 blockchain which means that it listens for specific layer 1 events and layer 2 transactions, and keeps the chain running by continuously grouping this data into batches and passing them to the execution client in order to produce layer 2 blocks. The op-batcher component, in turn, listens to op-node for fresh batches, groups the batches into channels, compresses them, splits them into frames, and posts frames to the data availability layer.
The op-proposer component listens to op-node for the so-called layer 2 output roots which are Merkle roots. These are then posted to the layer 1 so that the smart contracts on layer 1 can receive messages from layer 2. This includes ERC-20 token bridging as well as general message passing. The op-chain-ops component is a tool that facilitates the administration of the chain. The op-service component contains common utilities used by other parts of the codebase.
Presented below is a summary of changes to the in-scope components grouped by pull request and component.
op-node
SystemConfig contract on Ethereum and then passed to the execution layer as part of the first transaction of each L2 block along with other SystemConfig parameters.requestsChannelBufferSize constant was increased from 128 to 1024 and the logic around handling the corresponding channel was changed.unsafeL2PayloadsChannelBufferSize constant was increased from 10 to 4096 and the logic around handling the corresponding channel was changed.op-batcher
op-proposer
op-chain-ops
GasPriceOracle contract was added.op-service
op-service component encompass support for the Key Management Service (KMS) from the Google Cloud Platform (GCP) in Ethereum operations. In case the Cloud HSM is expected to be used, new configuration parameters have been introduced to streamline the process.op-node
op-batcher, op-proposer, op-chain-ops, and op-service
op-chain-ops
L1_MNT_ADDRESS constant was added.op-node, op-batcher, op-proposer, and op-service
MantleDA was treated as a black-box during this engagement as we were not provided with access to the documentation, tests, or the development instance of the system. Similarly, it was also assumed that the in-scope code uses MantleDA correctly.
Having said that, MantleDA is an emerging technology and thus has a higher chance of having bugs. The current way to recover from a MantleDA outage or malfunctioning is to stop the sequencer, switch data availability to Ethereum calldata in the configuration file, and start the sequencer again. This means there is no automatic fallback mechanism and, in case such an outage happens, the Mantle blockchain loses liveness until the sequencer is restarted with the new config.
The op-batcher component uses a private key to interact with the MantleDA contract which defines which MantleDA blobs should be used to derive the layer 2 chain.
The RetrievalFramesFromDa function of OP-Node implements the logic for retrieving frames from Mantle DA. It requires a dataStoreId value, which undergoes a check to verify if it is equal to or smaller than 0. As the calldata_source calls RetrievalFramesFromDa with the dataStoreId decreased by 1, attempting to retrieve data from Mantle DA with a dataStoreId equal to 1 becomes impossible.
Consider removing the check from the RetrievalFramesFromDa function to permit the processing of a dataStoreId with a value of 0. In addition, since the dataStoreId is of type uint32 and so cannot be smaller than 0, the check becomes unnecessary.
Update: Resolved in pull request #117.
RequestL2Range Does Not Return Error if Channel Is FullThe RequestL2Range function queues a range of L2 blocks and returns early if the channel is full. However, it does not return an error in this case which means that the partial data will be processed.
Consider returning an error in case the channel is full in order to make callers aware.
Update: Acknowledged, not resolved. The Mantle team stated:
Not a valid issue.
The ReadWitnessData function of OP-Chain-Ops utilizes bufio's NewReader to iterate through the file and retrieve all entries. To read each line, the function employs ReadString, which reads until a new line is encountered. However, this approach poses an issue – if the last line of the file lacks a new line character (as is the case for text file witness.txt), the reader will trigger an EOF error, causing an early break from the loop failing to read the last line.
Consider redesigning the function logic in such a way the last line will be read correctly even if it does not end with a new line.
Update: Acknowledged, not resolved. The Mantle team stated:
Acknowledged, only used for upgrading, will not fix it.
The NewWithdrawal function of OP-Chains-Ops returns a Withdrawal struct with filled values. The Data field, which is of type hexutil.Bytes, is assigned data of type []byte. However, the assigned value should first be converted to the correct type using the hexutil.Bytes function.
Consider converting the data parameter to the hexutil.Bytes type.
Update: Not resolved. The Mantle team stated:
Not a valid issue.
The proposed changes are not thoroughly covered by the test suite. In addition, there are multiple tests that fail, making it difficult to confirm the correctness of the implementation. The following components require additional testing:
Consider reviewing the test suite for the above mentioned components to improve code quality.
Update: Not resolved.
The BaseFee parameter of type big.Int is checked for not being nil. However, a big.Int can be a negative number which does not make sense for BaseFee given that it should always be positive.
Consider checking the BaseFee parameter both for not being nil and being a positive number just like other big.Int parameters (e.g., chain ids are validated to be greater than 0 and not equal to 0).
Update: Acknowledged, will resolve.
Upon starting the OP-Batcher, the state is cleaned but the Mantle DA status is not.
Consider calling the clearMantleDAStatus function to clear the Mantle DA status.
Update: Acknowledged, not resolved.
The sleep is used to wait 0.1 ms for sequencerCh and stepReqCh to be ready. While this might mitigate the issue in the testing environment, it might behave differently in the production environment where load might be different and thus the sleep time might not provide the desired effect.
Consider refactoring the code to avoid using sleep and instead using more reliable mechanisms to sync the channels.
Update: Not resolved. The Mantle team stated:
Not a valid issue.
The implemented connection to Mantle DA is missing a defined timeout option. This might lead to issues when servers accept connections but fail to respond to calls. There are the following occurrences of connections to Mantle DA:
getFramesByDataStoreId of OP-NodegetFramesFromIndexerByDataStoreId of OP-NodecallEncode of OP-BatcherConsider adding a timeout mechanism to the above listed connections.
Update: Acknowledged, will resolve.
The implemented connection to Mantle DA is unencrypted. In a production environment, it is generally recommended to encrypt the connection. There are the following occurrences of unencrypted connections to Mantle DA:
getFramesByDataStoreId of OP-NodegetFramesFromIndexerByDataStoreId of OP-NodecallEncode of OP-BatcherConsider using encrypted connection instead.
Update: Acknowledged, will resolve.
Throughout the codebase, several typographical errors were found:
TxConfirmDataSubmiited should be TxConfirmDataSubmitted.openend should be opened.comment should be L1_MANTLE_TOKEN not L1_MANTLE_TOEKN.Consider addressing the above typographical errors.
Update: Resolved in pull request #124.
Throughout the codebase, several instances of redundant and unclear code were identified:
NewMantleDataStore function defined in datastore.go always returns error as nil. Thus, it is not necessary to return it and check it later.NewMantleDataStoreConfig function returns a config and an error. However, the error is always nil which makes returning an error unnecessary.MockDataStoreConfig variable is not used anywhere and should be removed.marshalDepositVersion1 function is not used anywhere and can be removed.getFramesByDataStoreId function. Consider calling GetData once and then use it in log.Debug and the return statement.getFramesFromIndexerByDataStoreId function. Consider calling GetData once and then use it in log.Debug and the return statement.if statements check for the BaseFee not being equal to 0. Consider moving the second if statement inside the first if statement block and removing the unnecessary check.ds.cfg.MantleDaSwitch if statement could be easier to read if it used an if statement with only true and false branches where the true branch contained code for handling MantleDA and the else branch contained code for Ethereum calldata.MNTValue and ETHValue. The correct encoding starts with the MNTValue parameter followed by ETHValue parameter. However, in multiple places, the structures are initilaized in the reverse order. While this does not cause an issue since the parameter names are used, it does hinder readability. Consider changing the order of the MNTValue and ETHValue parameters in the Decode and WithdrawalTransaction functions.Update: Resolved in pull request #125.
The audit uncovered one issue of medium severity, in addition to several other issues of a lower severity. Various recommendations have been provided to enhance the quality and documentation of the codebase. We also strongly recommend that Mantle implements more extensive QA procedures and tests before going live to prevent potentially undiscovered vulnerabilities from being exploited. This is especially crucial in areas of the code where testing was limited, such as the integration with Mantle DA. The Mantle team was very supportive throughout the audit period and answered questions in a timely manner.