sherlock-audit / 2023-01-optimism-judging

22 stars 8 forks source link

0xWeiss - [M-03] Insufficient validation while initializing #291

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

0xWeiss

medium

[M-03] Insufficient validation while initializing

Summary

In the following function:

  https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L110-L125

   function initialize(
  address \_owner,
  uint256 \_overhead,
 uint256 \_scalar,
 bytes32 \_batcherHash,
  uint64 \_gasLimit,
  address \_unsafeBlockSigner
  ) public initializer {
   require(\_gasLimit >= MINIMUM_GAS_LIMIT, "SystemConfig: gas limit 
 too low");
 \_\_Ownable_init();
 transferOwnership(\_owner);
 overhead = \_overhead;
  scalar = \_scalar;
 batcherHash = \_batcherHash;
  gasLimit = \_gasLimit;
  \_setUnsafeBlockSigner(\_unsafeBlockSigner);
    }

there is no validation for the _owner and _unsafeBlockSigner addresses.

Vulnerability Detail

It does not check if owner is not 0 and _unsafeBlockSigner is not 0, meaning that if there is an error ownership will be transferes to address(0), losign ownership of the contract.

Impact

Losign ownership of the contract due to no input validation. The medium severity is granted due to code4rena similar reports that are medium severity. Example: https://solodit.xyz/issues/3537

Code Snippet

  https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol?plain=1#L110-L125

Tool used

Manual Review

Recommendation

Add require statements such as:

 require(_owner != address(0), "SystemConfig: owner is the zero address");
 require(_unsafeBlockSigner != address(0), "SystemConfig: _unsafeBlockSigner is the zero address");
rcstanciu commented 1 year ago

Comment from Optimism


Description: Insufficient validation while initializing

Reason: This report is false, but actionable. With our current deployment process, it is incredibly unlikely that the SystemConfig contract will be deployed with an _owner or _unsafeBlockSigner equal to zero.

Action: To prevent this issue from occuring for people who fork the OP Stack and choose a different deployment process, we should add a check that validates that the _owner and _unsafeBlockSigner are now equal to zero at the time of the SystemConfig contract's creation.