OR-1419 update onApprove logic

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

@@ -69,13 +69,33 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
     /// @inheritdoc CrossDomainMessenger
     function _sendMessage(address _to, uint64 _gasLimit, uint256 _value, bytes memory _data) internal override {
         require(msg.value == 0, "Deny depositing ETH");
-        if (_value > 0) {
-            IERC20(nativeTokenAddress).safeTransferFrom(msg.sender, address(this), _value);
-            IERC20(nativeTokenAddress).approve(address(PORTAL), _value);
-        }
         PORTAL.depositTransaction(_to, _value, _gasLimit, false, _data);
     }
 
+    /// @notice unpack onApprove data
+    /// @param _data     Data used in OnApprove contract
+    function unpackOnApproveData(bytes calldata _data)
+        public
+        pure
+        returns (address _from, address _to, uint256 _amount, uint32 _minGasLimit, bytes calldata _message)
+    {
+        require(_data.length >= 76, "On approve data for L1StandardBridge is too short");
+        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)
+        }
+    }
+
     /// @notice ERC20 onApprove callback
     /// @param _owner    Account that called approveAndCall
     /// @param _amount   Approved amount
@@ -91,13 +111,11 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
         returns (bool)
     {
         require(msg.sender == address(nativeTokenAddress), "only accept native token approve callback");
-        (uint32 _minGasLimit, bytes memory _message) = unpackOnApproveData(_data);
-
-        if (_amount > 0) {
-            IERC20(nativeTokenAddress).safeTransferFrom(_owner, address(this), _amount);
-            IERC20(nativeTokenAddress).approve(address(PORTAL), _amount);
-        }
-        PORTAL.depositTransaction(_owner, _amount, _minGasLimit, false, _message);
+        (address from, address to, uint256 amount, uint32 minGasLimit, bytes calldata message) =
+            unpackOnApproveData(_data);
+        require(_owner == from, "invalid encoded data: from");
+        require(_amount == amount, "invalid encoded data: amount");
+        _sendNativeTokenMessage(from, to, amount, minGasLimit, message);
         return true;
     }
 
@@ -127,6 +145,38 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
     )
         external
     {
+        // Triggers a message to the other messenger. Note that the amount of gas provided to the
+        // message is the amount of gas requested by the user PLUS the base gas value. We want to
+        // guarantee the property that the call to the target contract will always have at least
+        // the minimum gas limit specified by the user.
+        _sendNativeTokenMessage(msg.sender, _target, _amount, _minGasLimit, _message);
+    }
+
+    /// @notice Sends a deposit native token message internally to some target address on the other chain. Note that if
+    /// the call
+    ///         always reverts, then the message will be unrelayable, and any ETH sent will be
+    ///         permanently locked. The same will occur if the target on the other chain is
+    ///         considered unsafe (see the _isUnsafeTarget() function).
+    /// @param _sender      Sender address.
+    /// @param _target      Target contract or wallet address.
+    /// @param _amount      Amount of deposit native token.
+    /// @param _message     Message to trigger the target address with.
+    /// @param _minGasLimit Minimum gas limit that the message can be executed with.
+    function _sendNativeTokenMessage(
+        address _sender,
+        address _target,
+        uint256 _amount,
+        uint32 _minGasLimit,
+        bytes calldata _message
+    )
+        internal
+    {
+        // Collect native token
+        if (_amount > 0) {
+            IERC20(nativeTokenAddress).safeTransferFrom(_sender, address(this), _amount);
+            IERC20(nativeTokenAddress).approve(address(PORTAL), _amount);
+        }
+
         // Triggers a message to the other messenger. Note that the amount of gas provided to the
         // message is the amount of gas requested by the user PLUS the base gas value. We want to
         // guarantee the property that the call to the target contract will always have at least
@@ -136,12 +186,12 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, OnApprove, ISemver {
             baseGas(_message, _minGasLimit),
             _amount,
             abi.encodeWithSelector(
-                this.relayMessage.selector, messageNonce(), msg.sender, _target, _amount, _minGasLimit, _message
+                this.relayMessage.selector, messageNonce(), _sender, _target, _amount, _minGasLimit, _message
             )
         );
 
-        emit SentMessage(_target, msg.sender, _message, messageNonce(), _minGasLimit);
-        emit SentMessageExtension1(msg.sender, _amount);
+        emit SentMessage(_target, _sender, _message, messageNonce(), _minGasLimit);
+        emit SentMessageExtension1(_sender, _amount);
 
         unchecked {
             ++msgNonce;

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

@@ -107,6 +107,30 @@ contract L1StandardBridge is StandardBridge, OnApprove, ISemver {
         _initiateETHDeposit(msg.sender, msg.sender, RECEIVE_DEFAULT_GAS_LIMIT, bytes(""));
     }
 
+    /// @notice unpack onApprove data
+    /// @param _data     Data used in OnApprove contract
+    function unpackOnApproveData(bytes calldata _data)
+        public
+        pure
+        returns (address _from, address _to, uint256 _amount, uint32 _minGasLimit, bytes calldata _message)
+    {
+        require(_data.length >= 76, "On approve data for L1StandardBridge is too short");
+        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)
+        }
+    }
+
     /// @notice ERC20 onApprove callback
     /// @param _owner    Account that called approveAndCall
     /// @param _amount   Approved amount
@@ -122,8 +146,11 @@ contract L1StandardBridge is StandardBridge, OnApprove, ISemver {
         returns (bool)
     {
         require(msg.sender == address(nativeTokenAddress), "only accept native token approve callback");
-        (uint32 _minGasLimit, bytes memory _message) = unpackOnApproveData(_data);
-        _initiateBridgeNativeToken(_owner, _owner, _amount, _minGasLimit, _message);
+        (address from, address to, uint256 amount, uint32 minGasLimit, bytes memory message) =
+            unpackOnApproveData(_data);
+        require(_owner == from, "invalid encoded data: from");
+        require(_amount == amount, "invalid encoded data: amount");
+        _initiateBridgeNativeToken(from, to, amount, minGasLimit, message);
         return true;
     }
 

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

@@ -6,34 +6,6 @@ abstract contract OnApprove {
         return interfaceId == OnApprove.onApprove.selector || interfaceId == OnApprove.supportsInterface.selector;
     }
 
-    /// @notice unpack onApprove data
-    /// @param _data     Data used in OnApprove contract
-    function unpackOnApproveData(bytes memory _data) public pure returns (uint32 _minGasLimit, bytes memory _message) {
-        if (_data.length < 4) {
-            _minGasLimit = 200_000;
-            _message = bytes("");
-        }
-        assembly {
-            // The layout of a "bytes memory" is:
-            // The first 32 bytes: length of a "bytes memory"
-            // The next 4 bytes: _minGasLimit
-            // The rest: _message
-
-            let _pos := _data
-            // Get _data.length
-            let _length := mload(_pos)
-            // Pass first 32 bytes. Now the pointer "pos" is pointing to _minGasLimit
-            _pos := add(_pos, 32)
-            // Load value from the next 4 bytes
-            // mload() works with 32 bytes so we need shift right 32-4=28(bytes) = 224(bits)
-            _minGasLimit := shr(224, mload(_pos))
-            if iszero(eq(_length, 4)) {
-                // Pass 4 bytes to get embedded _message
-                _message := add(_pos, 4)
-            }
-        }
-    }
-
     function onApprove(
         address _owner,
         address _spender,

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

@@ -196,7 +196,7 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
     ///         otherwise any deposited funds will be lost due to address aliasing.
     // solhint-disable-next-line ordering
     receive() external payable {
-        revert("Not allow deposit to ERC-20: ETH");
+        revert("Only allow native token");
         // depositTransaction(msg.sender, msg.value, RECEIVE_DEFAULT_GAS_LIMIT, false, bytes(""));
     }
 
@@ -232,7 +232,7 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         // Prevent users from creating a deposit transaction where this address is the message
         // sender on L2. Because this is checked here, we do not need to check again in
         // `finalizeWithdrawalTransaction`.
-        require(_tx.target != address(this), "OptimismPortal: you cannot send messages to the portal contract");
+        require(_tx.target != address(this), "OptimismPortal: cannot send messages to this");
 
         // Get the output root and load onto the stack to prevent multiple mloads. This will
         // revert if there is no output root for the given block number.
@@ -256,7 +256,7 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         require(
             provenWithdrawal.timestamp == 0
                 || l2Oracle.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot != provenWithdrawal.outputRoot,
-            "OptimismPortal: withdrawal hash has already been proven"
+            "OptimismPortal: already been proven"
         );
 
         // Compute the storage slot of the withdrawal hash in the L2ToL1MessagePasser contract.
@@ -360,7 +360,7 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
                 IERC20(nativeTokenAddress).approve(
                     _tx.target, _tx.value + IERC20(nativeTokenAddress).allowance(address(this), _tx.target)
                 ),
-                "Optimism approved token failed"
+                "Optimism approve failed"
             );
             depositedAmount -= _tx.value;
         }
@@ -389,6 +389,39 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         }
     }
 
+    /// @notice unpack onApprove data
+    /// @param _data     Data used in OnApprove contract
+    function unpackOnApproveData(bytes calldata _data)
+        public
+        pure
+        returns (
+            address _from,
+            address _to,
+            uint256 _amount,
+            uint32 _gasLimit,
+            bool _isCreation,
+            bytes calldata _message
+        )
+    {
+        require(_data.length >= 77, "invalid onApprove data");
+        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: _gasLimit
+            // The next 1 byte: _isCreation
+            // The rest: _message
+            _from := shr(96, calldataload(_data.offset))
+            _to := shr(96, calldataload(add(_data.offset, 20)))
+            _amount := calldataload(add(_data.offset, 40))
+            _gasLimit := shr(224, calldataload(add(_data.offset, 72)))
+            _isCreation := shr(248, calldataload(add(_data.offset, 76)))
+            _message.offset := add(_data.offset, 77)
+            _message.length := sub(_data.length, 77)
+        }
+    }
+
     /// @notice ERC20 onApprove callback
     /// @param _owner    Account that called approveAndCall
     /// @param _amount   Approved amount
@@ -403,10 +436,14 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         override
         returns (bool)
     {
-        require(msg.sender == address(nativeTokenAddress), "only accept native token approve callback");
-        (uint32 _minGasLimit, bytes memory _message) = unpackOnApproveData(_data);
-        _depositTransaction(_owner, _owner, _amount, _minGasLimit, false, _message);
-        return true;
+        (address from, address to, uint256 amount, uint32 gasLimit, bool isCreation, bytes calldata message) =
+            unpackOnApproveData(_data);
+        if (msg.sender == nativeTokenAddress && _owner == from && _amount == amount) {
+            _depositTransaction(from, to, amount, gasLimit, isCreation, message, true);
+            return true;
+        } else {
+            return false;
+        }
     }
 
     // @notice Public interface for Accepting deposits of L1's ERC20 as L2's native token and data, and emits a
@@ -425,11 +462,11 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         uint256 _value,
         uint64 _gasLimit,
         bool _isCreation,
-        bytes memory _data
+        bytes calldata _data
     )
         external
     {
-        _depositTransaction(msg.sender, _to, _value, _gasLimit, _isCreation, _data);
+        _depositTransaction(msg.sender, _to, _value, _gasLimit, _isCreation, _data, false);
     }
 
     // @notice Accepts deposits of L1's ERC20 as L2's native token and data, and emits a TransactionDeposited event for
@@ -449,7 +486,8 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         uint256 _value,
         uint64 _gasLimit,
         bool _isCreation,
-        bytes memory _data
+        bytes calldata _data,
+        bool isOnApproveTrigger
     )
         internal
         metered(_gasLimit)
@@ -463,7 +501,7 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         // Just to be safe, make sure that people specify address(0) as the target when doing
         // contract creations.
         if (_isCreation) {
-            require(_to == address(0), "OptimismPortal: must send to address(0) when creating a contract");
+            require(_to == address(0), "OptimismPortal: _to should be address(0) when creating a contract");
         }
 
         // Prevent depositing transactions that have too small of a gas limit. Users should pay
@@ -477,9 +515,9 @@ contract OptimismPortal is Initializable, ResourceMetering, OnApprove, ISemver {
         require(_data.length <= 120_000, "OptimismPortal: data too large");
 
         // Transform the from-address to its alias if the caller is a contract.
-        address from = msg.sender;
-        if (msg.sender != tx.origin) {
-            from = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
+        address from = _sender;
+        if (_sender != tx.origin && !isOnApproveTrigger) {
+            from = AddressAliasHelper.applyL1ToL2Alias(_sender);
         }
 
         // Compute the opaque data that will be emitted as part of the TransactionDeposited event.

packages/tokamak/contracts-bedrock/test/OptimismPortal.t.sol

@@ -97,7 +97,7 @@ contract OptimismPortal_Test is Portal_Initializer {
 
     /// @dev Tests that `receive` reverted deposits ETH.
     function test_receive_reverts() external {
-        vm.expectRevert("Not allow deposit to ERC-20: ETH");
+        vm.expectRevert("Only allow native token");
         vm.deal(alice, 2 ** 64);
         vm.prank(alice, alice);
         (bool s,) = address(op).call{ value: 100 }(hex"");
@@ -108,7 +108,7 @@ contract OptimismPortal_Test is Portal_Initializer {
     ///      for a contract creation deposit.
     function test_depositTransaction_contractCreation_reverts() external {
         // contract creation must have a target of address(0)
-        vm.expectRevert("OptimismPortal: must send to address(0) when creating a contract");
+        vm.expectRevert("OptimismPortal: _to should be address(0) when creating a contract");
         op.depositTransaction(address(1), 0, 0, true, hex"");
     }
 
@@ -415,7 +415,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
     /// @dev Tests that `proveWithdrawalTransaction` reverts when the target is the portal contract.
     function test_proveWithdrawalTransaction_onSelfCall_reverts() external {
         _defaultTx.target = address(op);
-        vm.expectRevert("OptimismPortal: you cannot send messages to the portal contract");
+        vm.expectRevert("OptimismPortal: cannot send messages to this");
         op.proveWithdrawalTransaction(_defaultTx, _proposedOutputIndex, _outputRootProof, _withdrawalProof);
     }
 
@@ -443,7 +443,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         emit WithdrawalProven(_withdrawalHash, alice, bob);
         op.proveWithdrawalTransaction(_defaultTx, _proposedOutputIndex, _outputRootProof, _withdrawalProof);
 
-        vm.expectRevert("OptimismPortal: withdrawal hash has already been proven");
+        vm.expectRevert("OptimismPortal: already been proven");
         op.proveWithdrawalTransaction(_defaultTx, _proposedOutputIndex, _outputRootProof, _withdrawalProof);
     }