Or 1807 fix issues from the internal audit s feedback

op-e2e/e2e-dep-with/cross_domain_messenger_test.go

@@ -241,7 +241,7 @@ func TestSendNativeTokenMessageWithOnApprove(t *testing.T) {
  _, err = wait.ForReceiptOK(context.Background(), l1Client, tx.Hash())
  require.NoError(t, err)
 
- calldata := e2e.EncodeCallData(opts.From, opts.From, amount, uint32(200000), []byte{})
+ calldata := e2e.EncodeCallData(opts.From, uint32(200000), []byte{})
 
  l1BalanceBeforeDeposit, err := nativeTokenContract.BalanceOf(&bind.CallOpts{}, opts.From)
  require.NoError(t, err)

packages/tokamak/contracts-bedrock/scripts/Deploy.s.sol

@@ -860,7 +860,7 @@ contract Deploy is Deployer {
  nativeTokenAddress: cfg.nativeTokenAddress()
  })
  )
-  )
+ )
  });
 
  SystemConfig config = SystemConfig(systemConfigProxy);
@@ -924,7 +924,7 @@ contract Deploy is Deployer {
  SystemConfig(systemConfigProxy),
  SuperchainConfig(superchainConfigProxy)
  )
-  )
+ )
  });
 
  string memory version = L1StandardBridge(payable(l1StandardBridgeProxy)).version();
@@ -950,7 +950,7 @@ contract Deploy is Deployer {
  _innerCallData: abi.encodeCall(
  L1ERC721Bridge.initialize,
  (L1CrossDomainMessenger(l1CrossDomainMessengerProxy), SuperchainConfig(superchainConfigProxy))
-  )
+ )
  });
 
  L1ERC721Bridge bridge = L1ERC721Bridge(l1ERC721BridgeProxy);
@@ -1022,7 +1022,7 @@ contract Deploy is Deployer {
  OptimismPortal(payable(optimismPortalProxy)),
  SystemConfig(systemConfigProxy)
  )
-  )
+ )
  });
 
  L1CrossDomainMessenger messenger = L1CrossDomainMessenger(l1CrossDomainMessengerProxy);
@@ -1077,7 +1077,7 @@ contract Deploy is Deployer {
  cfg.l2OutputOracleChallenger(),
  cfg.finalizationPeriodSeconds()
  )
-  )
+ )
  });
 
  L2OutputOracle oracle = L2OutputOracle(l2OutputOracleProxy);
@@ -1121,7 +1121,7 @@ contract Deploy is Deployer {
  SystemConfig(systemConfigProxy),
  SuperchainConfig(superchainConfigProxy)
  )
-  )
+ )
  });
 
  OptimismPortal portal = OptimismPortal(payable(optimismPortalProxy));
@@ -1152,7 +1152,7 @@ contract Deploy is Deployer {
  ProtocolVersion.wrap(requiredProtocolVersion),
  ProtocolVersion.wrap(recommendedProtocolVersion)
  )
-  )
+ )
  });
 
  ProtocolVersions versions = ProtocolVersions(protocolVersionsProxy);

packages/tokamak/contracts-bedrock/scripts/DeployPeriphery.s.sol

@@ -277,7 +277,7 @@ contract DeployPeriphery is Deployer {
  dripcheck: CheckBalanceLow(mustGetAddress("CheckBalanceLow")),
  checkparams: abi.encode(
  CheckBalanceLow.Params({ target: mustGetAddress("FaucetProxy"), threshold: cfg.faucetDripV1Threshold() })
-  ),
+ ),
  actions: actions
  })
  });
@@ -306,7 +306,7 @@ contract DeployPeriphery is Deployer {
  dripcheck: CheckBalanceLow(mustGetAddress("CheckBalanceLow")),
  checkparams: abi.encode(
  CheckBalanceLow.Params({ target: mustGetAddress("FaucetProxy"), threshold: cfg.faucetDripV2Threshold() })
-  ),
+ ),
  actions: actions
  })
  });
@@ -341,7 +341,7 @@ contract DeployPeriphery is Deployer {
  target: mustGetAddress("FaucetProxy"),
  threshold: cfg.faucetAdminDripV1Threshold()
  })
-  ),
+ ),
  actions: actions
  })
  });
@@ -368,7 +368,7 @@ contract DeployPeriphery is Deployer {
  // Gelato represents ETH as 0xeeeee....eeeee
  0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE,
  cfg.faucetGelatoBalanceV1Value()
-  ),
+ ),
  value: cfg.faucetGelatoBalanceV1Value()
  });
  drippie.create({
@@ -383,7 +383,7 @@ contract DeployPeriphery is Deployer {
  threshold: cfg.faucetGelatoThreshold(),
  treasury: cfg.faucetGelatoTreasury()
  })
-  ),
+ ),
  actions: actions
  })
  });

packages/tokamak/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol

@@ -82,24 +82,20 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
  /// @notice unpack onApprove data
  /// @param _data Data used in OnApprove contract
  function unpackOnApproveData(bytes calldata _data)
- public
+ internal
  pure
- returns (address _from, address _to, uint256 _amount, uint32 _minGasLimit, bytes calldata _message)
+ returns (address _to, uint32 _minGasLimit, bytes calldata _message)
  {
- require(_data.length >= 76, "Invalid onApprove data for L1CrossDomainMessenger");
+ require(_data.length >= 24, "Invalid onApprove data for L1CrossDomainMessenger");
  assembly {
  // The layout of a "bytes calldata" is:
- // The first 20 bytes: _from
  // The next 20 bytes: _to
- // The next 32 bytes: _amount
  // The next 4 bytes: _minGasLimit
  // The rest: _message
- _from := shr(96, calldataload(_data.offset))
- _to := shr(96, calldataload(add(_data.offset, 20)))
- _amount := calldataload(add(_data.offset, 40))
- _minGasLimit := shr(224, calldataload(add(_data.offset, 72)))
- _message.offset := add(_data.offset, 76)
- _message.length := sub(_data.length, 76)
+ _to := shr(96, calldataload(_data.offset))
+ _minGasLimit := shr(224, calldataload(add(_data.offset, 20)))
+ _message.offset := add(_data.offset, 24)
+ _message.length := sub(_data.length, 24)
  }
  }
 
@@ -118,10 +114,8 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
  returns (bool)
  {
  require(msg.sender == address(nativeTokenAddress()), "only accept native token approve callback");
- (address from, address to, uint256 amount, uint32 minGasLimit, bytes calldata message) =
- unpackOnApproveData(_data);
- require(_owner == from && _amount == amount && amount > 0, "invalid onApprove data");
- _sendNativeTokenMessage(from, to, amount, minGasLimit, message);
+ (address to, uint32 minGasLimit, bytes calldata message) = unpackOnApproveData(_data);
+ _sendNativeTokenMessage(_owner, to, _amount, minGasLimit, message);
  return true;
  }
 
@@ -300,17 +294,21 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
  }
 
  xDomainMsgSender = _sender;
- // _target must not be address(0). otherwise, this transaction will be reverted
+ // _target must not be address(0). otherwise, this transaction could be reverted
  if (_value != 0 && _target != address(0)) {
  IERC20(_nativeTokenAddress).approve(_target, _value);
  }
+ // _target is expected to perform a transferFrom to collect token
  bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, 0, _message);
- if (_value != 0 && _target != address(0) && !success) {
+ if (_value != 0 && _target != address(0)) {
  IERC20(_nativeTokenAddress).approve(_target, 0);
  }
  xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
 
  if (success) {
+ // This check is identical to the one above, but it ensures that the same message cannot be relayed
+ // twice, and adds a layer of protection against reentrancy.
+ assert(successfulMessages[versionedHash] == false);
  successfulMessages[versionedHash] = true;
  emit RelayedMessage(versionedHash);
  } else {

packages/tokamak/contracts-bedrock/src/L1/L1StandardBridge.sol

@@ -151,24 +151,20 @@ contract L1StandardBridge is StandardBridge, OnApprove, ISemver {
  /// @notice unpack onApprove data
  /// @param _data Data used in OnApprove contract
  function unpackOnApproveData(bytes calldata _data)
- public
+ internal
  pure
- returns (address _from, address _to, uint256 _amount, uint32 _minGasLimit, bytes calldata _message)
+ returns (address _to, uint32 _minGasLimit, bytes calldata _message)
  {
- require(_data.length >= 76, "Invalid onApprove data for L1StandardBridge");
+ require(_data.length >= 24, "Invalid onApprove data for L1StandardBridge");
  assembly {
  // The layout of a "bytes calldata" is:
- // The first 20 bytes: _from
- // The next 20 bytes: _to
- // The next 32 bytes: _amount
+ // The first 20 bytes: _to
  // The next 4 bytes: _minGasLimit
  // The rest: _message
- _from := shr(96, calldataload(_data.offset))
- _to := shr(96, calldataload(add(_data.offset, 20)))
- _amount := calldataload(add(_data.offset, 40))
- _minGasLimit := shr(224, calldataload(add(_data.offset, 72)))
- _message.offset := add(_data.offset, 76)
- _message.length := sub(_data.length, 76)
+ _to := shr(96, calldataload(_data.offset))
+ _minGasLimit := shr(224, calldataload(add(_data.offset, 20)))
+ _message.offset := add(_data.offset, 24)
+ _message.length := sub(_data.length, 24)
  }
  }
 
@@ -188,10 +184,8 @@ contract L1StandardBridge is StandardBridge, OnApprove, ISemver {
  {
  address _nativeTokenAddress = nativeTokenAddress();
  require(msg.sender == _nativeTokenAddress, "only accept native token approve callback");
- (address from, address to, uint256 amount, uint32 minGasLimit, bytes memory message) =
- unpackOnApproveData(_data);
- require(_owner == from && _amount == amount && amount > 0, "invalid onApprove data");
- _initiateBridgeNativeToken(from, to, amount, minGasLimit, message);
+ (address to, uint32 minGasLimit, bytes memory message) = unpackOnApproveData(_data);
+ _initiateBridgeNativeToken(_owner, to, _amount, minGasLimit, message);
  return true;
  }
 
@@ -248,6 +242,7 @@ contract L1StandardBridge is StandardBridge, OnApprove, ISemver {
  internal
  override
  {
+ require(msg.value != 0, "StandardBridge: msg.value is zero amount");
  require(msg.value == _amount, "StandardBridge: bridging ETH must include sufficient ETH value");
  deposits[address(0)][Predeploys.ETH] = deposits[address(0)][Predeploys.ETH] + _amount;
  _emitETHBridgeInitiated(_from, _to, _amount, _extraData);