Audit Report: https://github.com/Secure3Audit/Secure3Academy/tree/main/audit_reports//Mantle_V2

Findings

High: 0/8

Medium: 4/29

Found 10.8% of H/M bugs

TakeAways

1. Make extensive research about the project before starting

   1. Inclusive of attack surfaces on project kinds

   2. Read reports on similar projects and hacks.

   3. Do not avoid the little complex details.

2. Make out time to research before audit starts so as not to eat into audit time with research.

3. Draw out a proper diagram if project too complex to make mental map.

4. Understand all technical components that make a project, don't gloss over any.

5. Use Divide and conquer for huge codebases with individual components and conquer as much components as possible.

6. Look more into junctions two protocols, like a project making a swap on Uniswap.

7. Make a write up on findings same day as found

   1. This process gives clarity of current codebase

   2. Cancels out false positives.

   3. Kills the rush at the end mindset.

Reports

User forced to overpay for gas on L2. Mantle V2 Mantle V2

Description

When supplying gas limit for transaction L1->L2, the bytes for message sent is called as such

require( _gasLimit >= minimumGasLimit(uint64(_data.length)), "OptimismPortal: gas limit too small" );

With minimum gas limit function being:

function minimumGasLimit(uint64 _byteCount) public pure returns (uint64) { 
    return _byteCount * 16 + 21000; 
    // @audit-issue over estimates gas cause 0 should be 4 and not 16 and if data is big will make it impossible to send as will always reach gas limit of 30M 
}

The challenge with this is that the zero bytes do not cost 16 gas, but rather cost 4 gas, meaning for each zero byte in the message, the user is charged 14 gas more…

Recommendation

Take cognizance of the zero bytes in the message and estimate accordingly.

EigenDA/MantleDA is not checked validated for liveness

Description

The EigenDA/MantleDA is not validated for liveness like it is done for other services/clients/components of the system.

l1Client, err := opclient.DialEthClientWithTimeout(ctx, cfg.L1EthRpc, opclient.DefaultDialTimeout)
    if err != nil {
        return nil, err
    }

    l2Client, err := opclient.DialEthClientWithTimeout(ctx, cfg.L2EthRpc, opclient.DefaultDialTimeout)
    if err != nil {
        return nil, err
    }

    rollupClient, err := opclient.DialRollupClientWithTimeout(ctx, cfg.RollupRpc, opclient.DefaultDialTimeout)
    if err != nil {
        return nil, err
    }

As it is seen above when NewBatchSubmitterFromCLIConfig is called in batch_submitter, it takes account of checking the liveness of the three components that constitute the mantle V1 chain: EthDA, Sequencer and RollupRPC. This should be all fine if the batcher is interested in Just submitting transactions straight to ETHDA, but that is not the case as required in MantleV2, as V2 introduces another external component, which is EigenDA. It should also check for the liveness of these forth service as the service as stated by the EigenDA team could possibly have outages: EigenDA is new software, and we don’t expect protocols to trust it blindly. If, for whatever reason, EigenDA goes down….

This is also important as the service to which the sequencer connects to directly, the disperse service, is a centralized service that is run by EigenDA team. All request to EigenDA has to pass through this single service before the request is dispersed to nodes in the quorum set. This introduces a single point of failure that is critical to the working of MantleDA.

Recommendation

The MantleDA service should be checked for liveness when starting the batch_submitter and if it is not live, then it should exit just like it does for the other components, as this component is as crucial as the others when rcfg.MantleDaSwitch is true.

if rcfg.MantleDaSwitch{
// Code to check for liveness
}

Overestimation could lead to permanent locking of funds on L1

Over estimation of gas in L2 could lead to locking of funds permanently in L1. the block Gas Limit on ethereum currently is 30M gas, which means any transaction that exceeds 30M gas will not be included in a block for execution. This is particularly important when a user is withdrawing from L2 to L1. As seen in SendMessage function on L2CrossDomainMessenger,

_sendMessage( 
    _ethAmount, 
    OTHER_MESSENGER,
    baseGas(_message, _minGasLimit), 
    abi.encodeWithSelector( 
        L1CrossDomainMessenger.relayMessage.selector, 
        messageNonce(), 
        msg.sender, 
        _target, 
        msg.value, 
        _ethAmount, 
        _minGasLimit,
        _message
    ) 
);

in baseGas calculation,

function baseGas(bytes calldata _message, uint32 _minGasLimit) public pure returns (uint64) {
        return
            // Constant overhead
            RELAY_CONSTANT_OVERHEAD +
            // Calldata overhead
            (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD) +
            // Dynamic overhead (EIP-150)
            // @submitted does not take into account 0 bytes, as zero bytes cost about 4 gas perbyte and not 16, so over estimates. 
            ((_minGasLimit * MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR) /
                MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR) +
            // Gas reserved for the worst-case cost of 3/5 of the `CALL` opcode's dynamic gas
            // factors. (Conservative)
            RELAY_CALL_OVERHEAD +
            // Relay reserved gas (to ensure execution of `relayMessage` completes after the
            // subcontext finishes executing) (Conservative)
            RELAY_RESERVED_GAS +
            // Gas reserved for the execution between the `hasMinGas` check and the `CALL`
            // opcode. (Conservative)
            RELAY_GAS_CHECK_BUFFER;
    }

It calculates the message gas cost per byte like so: (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD). This approach could lead to locking of funds, as zero bytes of data cost 4 gas while a non zero byte cost 16 gas. As can be seen, it multiplies the length of the byte by 16 gas, which will be problematic if the message happens to contain alot of zeros, meaning it will add 12 gas extra per each zero byte. Suppose a user sends a large amount of data, which is supposed meant to cost 20M gas, and the messenger estimates it to cost more than 30M gas, and the transaction makes it out of the l2 and 7 days wait period has passed and the user tries to withdraw, he will never be able to as no miner will include the transaction as it cost 30M gas or if included will keep failing due to hitting block gas limit, thus locking the user funds permanently.

Recommendation

Take cognisance of zero values in a message when estimating…

Relay message on L1 will fail due to incorrect gas estimation.

The two reserved constant gas amount for relaying a message

/// @notice Gas reserved for the execution between the hasMinGas check and the external /// call in relayMessage. uint64 public constant RELAY_GAS_CHECK_BUFFER = 5_000;

/// @notice Gas reserved for finalizing the execution of relayMessage after the safe call. uint64 public constant RELAY_RESERVED_GAS = 40_000;

As defined initially in OP stacks https://github.com/ethereum-optimism/optimism/blob/5877ee24cc9aaef5848c1372e0e196707fb336a0/packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol#L110C5-L116C1

Should not be applicable estimates to Mantle V2, this is because the estimates are far too small for the computational work that the v2 consumes compared to the v1/Opstack, for example the RELAY_GAS_CHECK_BUFFER is assigned a gas of 5k, which is just the gas amount to call SSTORE and reassign xDomainMsgSender from a non-zero value to another non-zero value:

xDomainMsgSender = _sender;

But the new introduced computation adds a call to an external contract

bool mntSuccess = true; 
if (_mntValue!=0){ 
    mntSuccess = IERC20(L1_MNT_ADDRESS).approve(_target, _mntValue); 
} 
// @audit-issue should cost more gas with calls to predeploys, especially _mntValue. 
xDomainMsgSender = _sender; bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _ethValue, _message);

Which will definitely consume more gas than the 5k supplied gas than reserved for it than the buffer, since this is the case the call function below will have significantly less gas than RELAY_CALL_OVERHEAD, meaning when the call operation below is performed

bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _ethValue, _message);

gasleft - RELAY_RESERVED_GAS < RELAY_CALL_OVERHEAD. which will lead to the transaction failing. As most of the gas has already been consumed by the approve function call.

And if by any chance there is more gas left which is unlikely, there is another approve call just below the safecall operation which was also no estimated for, that will eat up gas reserved for RELAY_RESERVED_GAS to complete the relay function.

if (_mntValue!=0){ mntSuccess = IERC20(L1_MNT_ADDRESS).approve(_target, 0); }

Recommendation

Take into account the new approve functions in the fixed gas estimations…

ERC 20 approve is assigned twice and validated once.

Description

The approve function of the ERC20 token is called twice, first time the approve is called with a amount > 0 and second time to approve an exactly zero amount, at each time a bool response returned.
But it is only validated once, meaning the first response is overriden by the second response, without a Validation if the first was a success response.

Recommendation

Check both times that the response is successful (true)…