chore(ctb): Remove some todos in messenger and portal (#3012)

* chore(ctb): Remove some todos in messenger and portal

* chore: Remove unresolved TODOs

* feat(ctb): ensure replayed messages have zero value
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

@@ -141,7 +141,6 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
     ) public payable metered(_gasLimit) {
         // Just to be safe, make sure that people specify address(0) as the target when doing
         // contract creations.
-        // TODO: Do we really need this? Prevents some user error, but adds gas.
         if (_isCreation) {
             require(
                 _to == address(0),

packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol

@@ -16,7 +16,6 @@ import { OptimismMintableERC20 } from "../universal/OptimismMintableERC20.sol";
  *         Examples of some token types that may not be properly supported by this contract include,
  *         but are not limited to: tokens with transfer fees, rebasing tokens, and
  *         tokens with blocklists.
- *         TODO: ensure that this has 1:1 backwards compatibility
  */
 contract L2StandardBridge is StandardBridge, Semver {
     /**

packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol

@@ -56,11 +56,8 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
 
     // the version is encoded in the nonce
     function test_L1MessengerMessageVersion() external {
-        (,uint16 version) = Encoding.decodeVersionedNonce(L1Messenger.messageNonce());
-        assertEq(
-            version,
-            L1Messenger.MESSAGE_VERSION()
-        );
+        (, uint16 version) = Encoding.decodeVersionedNonce(L1Messenger.messageNonce());
+        assertEq(version, L1Messenger.MESSAGE_VERSION());
     }
 
     // sendMessage: should be able to send a single message
@@ -172,7 +169,6 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
         address sender = PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
         bytes memory message = hex"1111";
 
-        // set the value of op.l2Sender() to be the L2 Cross Domain Messenger.
         vm.prank(address(op));
         vm.expectRevert("Message cannot be replayed.");
         L1Messenger.relayMessage(0, sender, target, 0, 0, message);
@@ -182,6 +178,18 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
         L1Messenger.relayMessage(0, sender, target, 0, 0, message);
     }
 
+    // relayMessage: should revert if eth is sent from a contract other than the standard bridge
+    function test_L1MessengerReplayMessageWithValue() external {
+        address target = address(0xabcd);
+        address sender = PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
+        bytes memory message = hex"1111";
+
+        vm.expectRevert(
+            "CrossDomainMessenger: Value must be zero unless message is from a system address."
+        );
+        L1Messenger.relayMessage{ value: 100 }(0, sender, target, 0, 0, message);
+    }
+
     // relayMessage: the xDomainMessageSender is reset to the original value
     function test_L1MessengerxDomainMessageSenderResets() external {
         vm.expectRevert("xDomainMessageSender is not set");

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

@@ -186,9 +186,6 @@ abstract contract CrossDomainMessenger is
      * @return Amount of gas required to guarantee message receipt.
      */
     function baseGas(bytes memory _message) public pure returns (uint32) {
-        // TODO: Values here are meant to be good enough to get a devnet running. We need to do
-        // some simple experimentation with the smallest and largest possible message sizes to find
-        // the correct constant and dynamic overhead values.
         return (uint32(_message.length) * MIN_GAS_DYNAMIC_OVERHEAD) + MIN_GAS_CONSTANT_OVERHEAD;
     }
 
@@ -263,13 +260,13 @@ abstract contract CrossDomainMessenger is
             // Should never happen.
             require(msg.value == _value, "Mismatched message value.");
         } else {
-            // TODO(tynes): could require that msg.value == 0 here
-            // to prevent eth from getting stuck
+            require(
+                msg.value == 0,
+                "CrossDomainMessenger: Value must be zero unless message is from a system address."
+            );
             require(receivedMessages[versionedHash], "Message cannot be replayed.");
         }
 
-        // TODO: Should blocking happen on sending or receiving side?
-        // TODO: Should this just return with an event instead of reverting?
         require(
             blockedSystemAddresses[_target] == false,
             "Cannot send message to blocked system address."
@@ -277,7 +274,6 @@ abstract contract CrossDomainMessenger is
 
         require(successfulMessages[versionedHash] == false, "Message has already been relayed.");
 
-        // TODO: Make sure this will always give us enough gas.
         require(
             gasleft() >= _minGasLimit + RELAY_GAS_REQUIRED,
             "Insufficient gas to relay message."

packages/contracts-bedrock/contracts/universal/StandardBridge.sol

@@ -442,7 +442,6 @@ abstract contract StandardBridge is Initializable {
 
             OptimismMintableERC20(_localToken).burn(_from, _amount);
         } else {
-            // TODO: Do we need to confirm that the transfer was successful?
             IERC20(_localToken).safeTransferFrom(_from, address(this), _amount);
             deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] + _amount;
         }