BASE - shealtielanz's results

Low Risk & Non Critical Findings

Report

Low-Risk Findings

Low-Risk Findings List

Total ~ 12 Issues

L-01 Missing storage gaps in Implementation contracts.

Summary

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

Vulnerability Details

• SystemConfig.sol
• L2CrossDomainMessenger.sol
• SystemDictator.sol
• L2CrossDomainMessenger.sol
• OptimismPortal.sol

L-02 Missing zero address checks on Critical State changing parameters

Summary

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most addresses in setters, there are many places where this is missing in mostly in the proxy contracts.

Vulnerability Details

_changeAdmin in the proxy contracts.

    function _changeAdmin(address _admin) internal {
        address previous = _getAdmin();
        assembly {
            sstore(OWNER_KEY, _admin)
        }
        emit AdminChanged(previous, _admin);
    }

_setImplementation in the proxy contracts.

    function _setImplementation(address _implementation) internal {
        assembly {
            sstore(IMPLEMENTATION_KEY, _implementation)
        }
        emit Upgraded(_implementation);
    }

L-03 No need for a redundant Function where the Function createStandardL2Token() is redundant and might express a lack of trustworthiness of the protocol.

Summary

I understand the developers love an intuitive functions name and it's cool, but the function createStandardL2Token()'s only logic is to call the function createOptimismMintableERC20() which creates an instance of the OptimismMintableERC20 contract, but an issue results from the function being called that is the createOptimismMintableERC20() function having its visibility set to public which means it can also be called externally. this creates confusion for the external viewer as to why two functions will perform the same task. This in turn will reflect on the trustworthiness of the whole protocol.

Vulnerability Details

 function createStandardL2Token(
        address _remoteToken,
        string memory _name,
        string memory _symbol
    ) external returns (address) {
        return createOptimismMintableERC20(_remoteToken, _name, _symbol);
    }

createStandardL2Token

 function createOptimismMintableERC20(
        address _remoteToken,
        string memory _name,
        string memory _symbol
    ) public returns (address) {
    //more code ...
    }

createOptimismMintableERC20

Recommended Mitigation Step.

I recommended changing the name of the createOptimismMintableERC20() function to be more intuitive and making it the sole function to be called for creating an instance of the OptimismMintableERC20 contract, thereby making the code simple and more trustworthy.

L-04 Missing non-reentrant modifier on Critical function.

Summary

A critical function missing a non-reentrant modifier could cause the contract to be vulnerable to reentracy which could cause a lot of harm if exploited by an attacker.

Vulnerability Details

• withdraw function in L2StandardBridge.sol.

    function withdraw(
        address _l2Token,
        uint256 _amount,
        uint32 _minGasLimit,
        bytes calldata _extraData
    ) external payable virtual onlyEOA {
        _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _extraData);
    }

• withdrawTo function.

L-05 Ownership maybe be lost.

Summary

Ownable2Step and Ownable2StepUpgradeable prevent the contract ownership from mistakenly being transferred to an address that cannot handle it (e.g. due to a typo in the address), by requiring that the recipient of the owner permissions actively accept via a contract call of its own.

Vulnerability Details

SystemConfig.sol

import {
    OwnableUpgradeable
} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

L-06 Unsafe Down-cast

Summary

When a type is downcast to a smaller type, the higher-order bits are truncated, effectively applying a modulo to the original value. Without any other checks, this wrapping will lead to unexpected behavior and bugs

Vulnerability Details

minimumGasLimit() function SystemConfig.sol

       return uint64(_resourceConfig.maxResourceLimit) + uint64(_resourceConfig.systemTxMaxGas);

L-07 Unbounded Loops may result in DOS

Summary

Consider limiting the number of iterations in for-loops that can be called by anyone as they could call with an arbitrary amount of input causing the contract to surpass the block gas limit.

Vulnerability Details

getL1GasUsed function in GasPriceOracle.sol

    function getL1GasUsed(bytes memory _data) public view returns (uint256) {
        uint256 total = 0;
        uint256 length = _data.length;
        for (uint256 i = 0; i < length; i++) {
            if (_data[i] == 0) {
                total += 4;
            } else {
                total += 16;
            }
        }
        uint256 unsigned = total + overhead();
        return unsigned + (68 * 16);
    }
}

L-08 Critical Implementations Initializers not Locked.

Summary

OpenZeppelin’s best practice is to lock the Initializers in the function by calling the _disableintializers function in the contract but this isn't done here in this protocol.

Vulnerability Details

• SystemConfig.sol
• L2CrossDomainMessenger.sol
• SystemDictator.sol
• L2CrossDomainMessenger.sol
• OptimismPortal.sol

L-09 The onlyEOA modifier can be bypassed.

Summary

The onlyEOA modifier is used on a function to ensure that it cannot be called by a contract however this assumption is wrong due to the fact that the check of the modifier can be passed during the creation of a contract via its constructor, and could be exploited if it's unintended by the developers that a Smart Contract should call that function.

Vulnerability Details

receive() function in L2StandardBridge.sol

  /**
     * @notice Allows EOAs to bridge ETH by sending directly to the bridge.
     */
    receive() external payable override onlyEOA {
        _initiateWithdrawal(
            Predeploys.LEGACY_ERC20_ETH,
            msg.sender,
            msg.sender,
            msg.value,
            RECEIVE_DEFAULT_GAS_LIMIT,
            bytes("")
        );
    }

Recommendation

add additional checks to ensure it's an EOA and be careful as to the implications.

L-10 Loss of Precision.

Summary

Division by large numbers may result in the result being zero, due to solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator

Vulnerability Details

getL1Fee function in GasPriceOracle.sol

uint256 scaled = unscaled / divisor;

L-11 Division by Zero not prevented.

Summary

The divisions below take an input parameter that does not have any zero-value checks, which may lead to the functions reverting when zero is passed.

Vulnerability Details

getL1Fee function in GasPriceOracle.sol

uint256 scaled = unscaled / divisor;

_setResourceConfig function in the SystemConfig.sol

            ((_config.maxResourceLimit / _config.elasticityMultiplier) *
                _config.elasticityMultiplier) == _config.maxResourceLimit,

[L‑12] Missing checks for the basefee and l1FeeScalar input values.

The input values for the basefee and l1FeeScalar variables have no checks to ensure that their values are greater than zero. If this ever happens, it will lead to disastrous consequences which will affect all other functionalities that are dependent on these variables like the fee calculations.

function setL1BlockValues(
        uint64 _number,
        uint64 _timestamp,
        uint256 _basefee,
        bytes32 _hash,
        uint64 _sequenceNumber,
        bytes32 _batcherHash,
        uint256 _l1FeeOverhead,
        uint256 _l1FeeScalar
    ) external {
        require(
            msg.sender == DEPOSITOR_ACCOUNT,
            "L1Block: only the depositor account can set L1 block values"
        );

        number = _number;
        timestamp = _timestamp;
        basefee = _basefee;
        hash = _hash;
        sequenceNumber = _sequenceNumber;
        batcherHash = _batcherHash;
        l1FeeOverhead = _l1FeeOverhead;
        l1FeeScalar = _l1FeeScalar;
    }

Non-Critical Findings

Non-Critical Issues

Non-Critical Issues List

Total ~ 4 Issues

NC-01 Consider Using Named Mapping

Summary

Consider moving to solidity version 0.8.18 or later, and using named mappings to make it easier to understand the purpose of each mapping.

Vulnerability Details

OptimismPortal.sol

    mapping(bytes32 => bool) public finalizedWithdrawals;

    /**
     * @notice A mapping of withdrawal hashes to `ProvenWithdrawal` data.
     */
    mapping(bytes32 => ProvenWithdrawal) public provenWithdrawals;

NC-02 Use constants instead of Magic Numbers

Summary

Link to code

        return _byteCount * 16 + 21000;

SystemDictator.sol.

          currentStep = 1;
          function step1() public onlyOwner step(1)
              function step2() public onlyOwner step(2) 

Vulnerability Details

NC-03 Use Double Quote for string literals.

Summary

Best practice is to use double quotes for string literals rather than single literals.

Vulnerability Details

Link to code

     *         bytes32(uint256(keccak256('eip1967.proxy.admin')) - 1)
     *         bytes32(uint256(keccak256('eip1967.proxy.implementation')) - 1)

NC-04 Use Immutable in place of constants

Summary

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant while it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.

Vulnerability Details

SystemConfig.sol

  bytes32 public constant UNSAFE_BLOCK_SIGNER_SLOT = keccak256("systemconfig.unsafeblocksigner");