BASE - VictoryGod's results

Summary

Issues List

Total 46 issues

File -> L1CrossDomainMessenger.sol

[01]Add a require statement to ensure that the _portal address provided in the constructor is not a zero address. This prevents the contract from being initialized with an invalid portal address. 

require(address(_portal) != address(0), "Invalid portal address");

[02] Check for a valid recipient address in the _sendMessage function

Add a require statement to ensure that the recipient address _to is not a zero address. This prevents sending messages to an invalid address.

require(_to != address(0), "Invalid recipient address");

[03] Use IERC165 to indicate support for the L2OutputOracle interface

To make it easier for other contracts to interact with L2OutputOracle, you can use the IERC165 standard to indicate that the contract supports the L2OutputOracle interface.

import "@openzeppelin/contracts/utils/introspection/IERC165.sol";
contract L2OutputOracle is Initializable, Semver, Ownable, IERC165 {  ...  function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {  return interfaceId == type(L2OutputOracle).interfaceId || super.supportsInterface(interfaceId);  } }

[04] Use modifiers to improve readability 

Instead of having require() statements in the functions, you can use modifiers to improve the readability of the code.

modifier onlyProposer() {
    require(msg.sender == _proposer, "L2OutputOracle: only the proposer address can propose new outputs");
    _;
}

modifier onlyChallenger() {
    require(msg.sender == _challenger, "L2OutputOracle: only the challenger address can delete outputs");
    _;
}

function deleteL2Outputs(uint256 _l2OutputIndex) external onlyChallenger {
    ...
}

function proposeL2Output(
    bytes32 _outputRoot,
    uint256 _l2BlockNumber,
    bytes32 _l1BlockHash,
    uint256 _l1BlockNumber
) external payable onlyProposer {
    ...
}

[05] Update the error message for the timestamp require statement to include "or equal to" for better clarity. 

"L2OutputOracle: starting L2 timestamp must be less than or equal to the current time"

require(
            _startingTimestamp <= block.timestamp,
            "L2OutputOracle: starting L2 timestamp must be less than or equal to the current time"
        );

[06] Remove the solhint-disable-next-line ordering comment, as it is not necessary in this case. The function does not violate any Solidity ordering rules, and the comment can be safely removed without affecting the code's functionality.

File -> OptimismPortal.sol

[07] Gas limit for receive() function 

The contract sets a constant gas limit of 100,000 for the receive() function. This value may not be suitable for all use cases and may need to be adjusted in the future.

uint64 internal constant RECEIVE_DEFAULT_GAS_LIMIT = 100_000;

Consider making the gas limit configurable by the contract administrator or implement an algorithm to adjust the gas limit based on the actual gas usage of the receive() function.

[08] The code uses the magic number hex"01" in the SecureMerkleTrie.verifyInclusionProof() function call. It's better to define this value as a constant with a descriptive name to improve code readability and maintainability.

Example improvement:

bytes1 constant STORAGE_OCCUPIED = hex"01";  ...  require(  SecureMerkleTrie.verifyInclusionProof(  abi.encode(storageKey),  STORAGE_OCCUPIED,  _withdrawalProof,  _outputRootProof.messagePasserStorageRoot
),  "OptimismPortal: invalid withdrawal inclusion proof"  );

[09] Missing function documentation

The function documentation in the comment block is missing the description of the _tx parameter. Adding a description for this parameter will help improve code readability and maintainability. Example improvement:

/**
... * @param _tx              Withdrawal transaction object containing sender, target, and other relevant information. * ... */

[10] The function calls L2_ORACLE.getL2Output(_l2OutputIndex) twice. This can be optimized by storing the result in a variable and reusing it.

Example improvement:

Types.L2Output memory l2Output = L2_ORACLE.getL2Output(_l2OutputIndex);  bytes32 outputRoot = l2Output.outputRoot;  ...  require(  provenWithdrawal.timestamp == 0 ||  L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot !=  provenWithdrawal.outputRoot,  "OptimismPortal: withdrawal hash has already been proven"  );

[11] The code uses the magic number Constants.DEFAULT_L2_SENDER in multiple places. It's better to define this value as a constant with a descriptive name within the contract to improve code readability and maintainability.

Example improvement:

address constant DEFAULT_L2_SENDER = Constants.DEFAULT_L2_SENDER;  ...  require(  l2Sender == DEFAULT_L2_SENDER,  "OptimismPortal: can only trigger one withdrawal per transaction"  );  ...  l2Sender = DEFAULT_L2_SENDER;

[12] The function calls L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex) twice. This can be optimized by storing the result in a variable and reusing it. 

Example improvement:

Types.OutputProposal memory proposal = L2_ORACLE.getL2Output(  provenWithdrawal.l2OutputIndex
);  ...  require(  proposal.outputRoot == provenWithdrawal.outputRoot,  "OptimismPortal: output root proven is not the same as current output root"  );

[13] The function uses SafeCall.callWithMinGas() to call the target contract. Although this provides some safety, it may lead to inefficient gas usage. Consider using the call function instead and handling potential errors in a more efficient way.

(bool success, bytes memory returnData) = _tx.target.call{gas: _tx.gasLimit, value: _tx.value}(_tx.data);

[14] Instead of using a separate if statement to handle the case where success is false and tx.origin is Constants.ESTIMATION_ADDRESS, consider using a require statement with a custom error message. This will simplify the code and make error handling more consistent.

Example improvement:

require(  !(success == false && tx.origin == Constants.ESTIMATION_ADDRESS),  "OptimismPortal: withdrawal failed"  );

[15] The depositTransaction function is marked as public. Consider using the external visibility modifier if it is only intended to be called from outside the contract.

[16] The maximum data length is set using a magic number 120_000. To improve code readability and maintainability, define a constant variable with a descriptive name. 

uint256 constant MAX_DEPOSIT_DATA_LENGTH = 120_000;  ...  require(_data.length <= MAX_DEPOSIT_DATA_LENGTH, "OptimismPortal: data too large");

[17] Error message consistency 

The error messages in require statements start with "OptimismPortal: " in the previous function but not in this one. To keep error messages consistent, consider adding the prefix to the error messages in this function as well.

require(  _to == address(0),  "OptimismPortal: must send to address(0) when creating a contract"  );  ...  require(  _gasLimit >= minimumGasLimit(uint64(_data.length)),  "OptimismPortal: gas limit too small"  );  ...  require(_data.length <= MAX_DEPOSIT_DATA_LENGTH, "OptimismPortal: data too large");

File -> OptimismMintableERC20Factory.sol

[18] Lack of input validation for _name and _symbol parameters

The createOptimismMintableERC20 function accepts _name and _symbol as parameters but does not perform any validation on these inputs. It is a best practice to validate user inputs to prevent potential issues such as deploying tokens with empty or invalid names and symbols. To improve this, you can add input validation checks for these parameters. For example:

require(bytes(_name).length > 0, "OptimismMintableERC20Factory: name must not be empty"); require(bytes(_symbol).length > 0, "OptimismMintableERC20Factory: symbol must not be empty");

[19] Redundant legacy function

The createStandardL2Token function is marked as a legacy function and is identical to the createOptimismMintableERC20 function. It is recommended to remove redundant code to improve code quality and maintainability. If the legacy function is no longer needed, consider removing it from the contract.

[20] Inconsistent event argument ordering

The StandardL2TokenCreated and OptimismMintableERC20Created events have different argument orderings, which might cause confusion for developers interacting with the contract. It is a best practice to maintain consistency in event argument ordering to improve code readability and reduce potential mistakes. You can update the OptimismMintableERC20Created event to have the same argument ordering as the StandardL2TokenCreated event:

event OptimismMintableERC20Created(  address indexed remoteToken,  address indexed localToken,  address deployer );

[21] Lack of access control

Currently, any user can call the createOptimismMintableERC20 function to deploy a new token. Depending on your requirements, you may want to implement access control to restrict token creation to specific users or roles. One possible solution is to use the OpenZeppelin AccessControl library, which allows you to define roles and permissions for your contract.

[22] No documentation for the Semver contract

File -> SystemConfig.sol

[23] Use of external visibility for some functions

In the smart contract, you have some external functions like unsafeBlockSigner(), setUnsafeBlockSigner(), setBatcherHash(), setGasConfig(), setGasLimit(), resourceConfig(), and setResourceConfig(). These functions can be called from other contracts. If these functions are only meant to be called from this contract or derived contracts, you should consider changing their visibility to public or internal.

[24] Improve storage usage for overhead and scalar

Currently, overhead and scalar are stored as separate uint256 variables. However, they both represent gas overheads and are related. You can save some storage space by packing these two variables into a single uint256 variable using bitwise operations. Here's an example of how to do this:

uint256 private gasConfig;
function getOverhead() public view returns (uint256) {  return gasConfig & 0xFFFFFFFFFFFFFFFF; }
function getScalar() public view returns (uint256) {  return gasConfig >> 64; }
function setGasConfig(uint256 _overhead, uint256 _scalar) external onlyOwner {  gasConfig = (_scalar << 64) | _overhead;
bytes memory data = abi.encode(_overhead, _scalar);  emit ConfigUpdate(VERSION, UpdateType.GAS_CONFIG, data); }

[25] Possible precision loss when computing target resource limit

In _setResourceConfig(), you check for precision loss when computing the target resource limit. However, this check might not be sufficient as it only checks if the result of the division and multiplication is equal to the original value. It's recommended to use a more robust check for precision loss, such as using a library like SafeMath or PRBMath for arithmetic operations

[26] The code contains magic numbers like 1 and 0, which may be confusing to understand. It's better to use named constants for these values to improve code readability.

uint256 constant MIN_DENOMINATOR = 1; uint256 constant MIN_ELASTICITY_MULTIPLIER = 0;

Then replace the magic numbers in the require statements with these constants:

require(  _config.baseFeeMaxChangeDenominator > MIN_DENOMINATOR,  "SystemConfig: denominator must be larger than 1" );
require(  _config.elasticityMultiplier > MIN_ELASTICITY_MULTIPLIER,  "SystemConfig: elasticity multiplier cannot be 0" );

[27] The function _setResourceConfig accepts a ResourceMetering.ResourceConfig struct as input. It's important to validate the input data before processing it. Although the function has some require statements to check the input data, it's a good practice to have a separate function to validate the input data and call that function at the beginning of the _setResourceConfig function.

function _validateResourceConfig(ResourceMetering.ResourceConfig memory _config) internal pure { // Add require statements to validate the input data. }

Then call this function at the beginning of _setResourceConfig:

function _setResourceConfig(ResourceMetering.ResourceConfig memory _config) internal {  // Validate input data.  _validateResourceConfig(_config);
// Rest of the code. }

[28] The error messages in the require statements are quite concise, but they could be made more descriptive to provide better context for debugging.

require(  _config.minimumBaseFee <= _config.maximumBaseFee,  "SystemConfig: minimum base fee must be less than or equal to maximum base fee" );
require(  _config.baseFeeMaxChangeDenominator > MIN_DENOMINATOR,  "SystemConfig: base fee max change denominator must be greater than 1" );

[29] While the function has comments explaining the purpose of each require statement, it would be helpful to have a brief comment at the beginning of the function explaining its overall purpose and how it fits into the broader context of the contract.

/**
@dev Sets the resource configuration for the system. * @param _config The new resource configuration. */ function _setResourceConfig(ResourceMetering.ResourceConfig memory _config) internal {  // ... }

File -> SystemDictator.sol

[30] Possible precision loss when computing target resource limit

In _setResourceConfig(), you check for precision loss when computing the target resource limit. However, this check might not be sufficient as it only checks if the result of the division and multiplication is equal to the original value. It's recommended to use a more robust check for precision loss, such as using a library like SafeMath or PRBMath for arithmetic operations

[31] it is essential to provide proper documentation and comments using the NatSpec format to improve code readability and maintainability.

[32]the functions step1(), step2(), step3() and step4() are marked as public, which means they can be called by any external contract or user. Since both functions are only intended to be called by the contract owner, consider changing their visibility to external

File -> GasPriceOracle.sol

[33] Use of for loop in getL1GasUsed function:

The getL1GasUsed function iterates through the entire input data using a for loop, which can be inefficient and consume a lot of gas if the input data is large. Using a more efficient approach or library to process the input data.

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);  }

[34] The code uses magic numbers such as 4, 16, and 68 in the getL1GasUsed function. It is recommended to use constant variables with descriptive names instead of magic numbers to improve code readability and maintainability.

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; // Use a constant variable instead of 4  } else {  total += 16; // Use a constant variable instead of 16  }  }  uint256 unsigned = total + overhead();  return unsigned + (68 * 16); // Use constant variables instead of 68 and 16  }

File -> L1Block.sol

[35] The contract doesn't emit any events when the state is updated. It's a good practice to emit events when the state changes, especially for important values like block data or fees. This can help track changes and provide a better user experience for developers using your contract.

event L1BlockValuesUpdated(  uint64 number,  uint64 timestamp,  uint256 basefee,  bytes32 hash,  uint64 sequenceNumber,  bytes32 batcherHash,  uint256 l1FeeOverhead,  uint256 l1FeeScalar );
function setL1BlockValues(  // ...  ) external {  // ...
emit L1BlockValuesUpdated(  _number,  _timestamp,  _basefee,  _hash,  _sequenceNumber,  _batcherHash,  _l1FeeOverhead,             _l1FeeScalar
);  }

[36] The constructor function is marked with a @custom:semver tag, but it doesn't have any custom logic. If you don't need to add any custom logic to the constructor, you can remove this tag.

File -> L2CrossDomainMessenger.sol

[37] The contract doesn't emit any events when a message is sent or received. It's a good practice to emit events when important actions are performed, such as message passing between L1 and L2. This can help track changes and provide a better user experience for developers using your contract.

event L2MessageSent(address indexed _to, uint64 _gasLimit, uint256 _value, bytes _data);
function _sendMessage(  address _to,  uint64 _gasLimit,  uint256 _value,  bytes memory _data ) internal override {  L2ToL1MessagePasser(payable(Predeploys.L2_TO_L1_MESSAGE_PASSER)).initiateWithdrawal{  value: _value }(_to, _gasLimit, _data);
emit L2MessageSent(_to, _gasLimit, _value, _data);  }

[38] The initialize() function is marked public, which allows anyone to call it. However, as it's an initializer function, it should only be called once during the deployment of the contract. To prevent unintended usage, you can restrict its visibility to internal or private.

function initialize() internal initializer {  __CrossDomainMessenger_init();  }

[39] Use of AddressAliasHelper.undoL1ToL2Alias

The _isOtherMessenger function uses AddressAliasHelper.undoL1ToL2Alias(msg.sender) to check if the msg.sender is the expected OTHER_MESSENGER. While this seems to work, it's a good practice to use the canonical address directly instead of relying on the address alias helper. You can store the canonical address of the L1 messenger in a separate state variable during the constructor and use it for comparison.

address public immutable L1_MESSENGER_CANONICAL;
constructor(address _l1CrossDomainMessenger) {  // ...  L1_MESSENGER_CANONICAL = AddressAliasHelper.undoL1ToL2Alias(_l1CrossDomainMessenger);  }
function _isOtherMessenger() internal view override returns (bool) {  return msg.sender == L1_MESSENGER_CANONICAL;  }

File -> L2StandardBridge.sol

[40] Reentrancy protection is missing

The contract does not implement any reentrancy protection mechanism such as the ReentrancyGuard provided by OpenZeppelin. This could lead to potential reentrancy attacks if there are any callbacks in the token contracts used with this bridge. To mitigate this vulnerability, consider adding the ReentrancyGuard and using the nonReentrant modifier for external functions that interact with token contracts, like withdraw, withdrawTo, and finalizeDeposit. Example:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract L2StandardBridge is StandardBridge, Semver, ReentrancyGuard {  // ...  function withdraw(  // ...  ) external payable virtual onlyEOA nonReentrant {  // ...  }
function withdrawTo(  // ...  ) external payable virtual nonReentrant {  // ...  }
function finalizeDeposit(  // ...  ) external payable virtual nonReentrant {  // ...  } }

File -> L2ToL1MessagePasser.sol

[41] The constructor function is marked with a @custom:semver tag, but it doesn't have any custom logic. If you don't need to add any custom logic to the constructor, you can remove this tag.

File -> L2OutputOracle.sol

[42] Use for arithmetic operations

Although Solidity 0.8.x has built-in overflow and underflow checks, it is still a good practice to use SafeMath for arithmetic operations to ensure safety.

import "@openzeppelin/contracts/utils/math/SafeMath.sol";
contract L2OutputOracle is Initializable, Semver, Ownable {  using SafeMath for uint256;  ... }

File -> ALL Contracts

[43] Upgradeability

The contract is missing an upgradeability mechanism, which means that if any issues are found or new features need to be added, there might be no way to upgrade the contract without deploying a new one and migrating the state.

[44] Reentrancy

The contract does not use the ReentrancyGuard modifier to protect against reentrancy attacks. You should consider adding ReentrancyGuard to the contract inheritance list and using the nonReentrant modifier on external functions that deal with token transfers or other state-changing operations.

[45] Code readability : Use better variable names for readability 

[46] Function writing that does not comply with the Solidity Style Guide

for example : L2OutputOracle.sol

Description

Order of Functions; ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

(https://docs.soliditylang.org/en/v0.8.17/style-guide.html)

Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private

within a grouping, place the view and pure functions last