refactor and test: bridging contracts

audits/internal9/README.md

@@ -20,6 +20,7 @@ File                       |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovere
   WormholeMessenger.sol    |        0 |        0 |        0 |        0 |... 201,202,207 |
 ```
 Pay attention, please! No tests for contract on scope.
+[x] Fixed
 
 #### Wormhole security list
 https://docs.wormhole.com/wormhole/quick-start/cross-chain-dev/standard-relayer#other-considerations
@@ -35,6 +36,7 @@ Refunding overpayment of gasLimit - [?] - requires discussion
 Refunding overpayment of value sent - [?] - requires discussion
 ref: https://github.com/wormhole-foundation/hello-wormhole/blob/main/src/extensions/HelloWormholeRefunds.sol
 ```
+[x] Good point, need to set up these parameters and call the corresponding function on the L1 Relayer (added to test scripts)
 
 ### Security issues
 Details in [slither_full](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal9/analysis/slither_full.txt) <br>
@@ -56,6 +58,8 @@ so avoid casting bytes32 to address EVM.
         }
 ref: https://github.com/wormhole-foundation/wormhole-solidity-sdk/blob/main/src/Base.sol#L30C52-L30C74 SDK of Wormhole
 ```
+[x] Fixed
+
 ##### Re-design contracts
 Part of the code is common and has the nature of a repeatable pattern for all contracts. <br>
 It makes sense to separate it into a separate contract and replace magic numbers (20,12,4) with constants.
@@ -109,6 +113,7 @@ It makes sense to separate it into a separate contract and replace magic numbers
             }
         }
 ```
+[x] Fixed
 
 
 

contracts/bridges/BridgeMessenger.sol

@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import {IBridgeErrors} from "../interfaces/IBridgeErrors.sol";
+
+/// @title BridgeMessenger - Abstract smart contract for the bridge communication data processing
+/// @author Aleksandr Kuperman - <[email protected]>
+/// @author Andrey Lebedev - <[email protected]>
+/// @author Mariapia Moscatiello - <[email protected]>
+abstract contract BridgeMessenger is IBridgeErrors {
+    event FundsReceived(address indexed sender, uint256 value);
+
+    // Default payload data length includes the number of bytes of at least one address (20 bytes or 160 bits),
+    // value (12 bytes or 96 bits) and the payload size (4 bytes or 32 bits)
+    uint256 public constant DEFAULT_DATA_LENGTH = 36;
+
+    /// @dev Receives native network token.
+    receive() external payable {
+        emit FundsReceived(msg.sender, msg.value);
+    }
+
+    /// @dev Processes received data.
+    /// @param data Bytes message sent from L2 Relayer contract. The data must be encoded as a set of
+    ///        continuous transactions packed into a single buffer, where each transaction is composed as follows:
+    ///        - target address of 20 bytes (160 bits);
+    ///        - value of 12 bytes (96 bits), as a limit for all of Autonolas ecosystem contracts;
+    ///        - payload length of 4 bytes (32 bits), as 2^32 - 1 characters is more than enough to fill a whole block;
+    ///        - payload as bytes, with the length equal to the specified payload length.
+    function _processData(bytes memory data) internal virtual {
+        // Check for the correct data length
+        uint256 dataLength = data.length;
+        if (dataLength < DEFAULT_DATA_LENGTH) {
+            revert IncorrectDataLength(DEFAULT_DATA_LENGTH, data.length);
+        }
+
+        // Unpack and process the data
+        for (uint256 i = 0; i < dataLength;) {
+            address target;
+            uint96 value;
+            uint32 payloadLength;
+            // solhint-disable-next-line no-inline-assembly
+            assembly {
+                // First 20 bytes is the address (160 bits)
+                i := add(i, 20)
+                target := mload(add(data, i))
+                // Offset the data by 12 bytes of value (96 bits)
+                i := add(i, 12)
+                value := mload(add(data, i))
+                // Offset the data by 4 bytes of payload length (32 bits)
+                i := add(i, 4)
+                payloadLength := mload(add(data, i))
+            }
+
+            // Check for the zero address
+            if (target == address(0)) {
+                revert ZeroAddress();
+            }
+
+            // Check for the value compared to the contract's balance
+            if (value > address(this).balance) {
+                revert InsufficientBalance(value, address(this).balance);
+            }
+
+            // Get the payload
+            bytes memory payload = new bytes(payloadLength);
+            for (uint256 j = 0; j < payloadLength; ++j) {
+                payload[j] = data[i + j];
+            }
+            // Offset the data by the payload number of bytes
+            i += payloadLength;
+
+            // Call the target with the provided payload
+            (bool success, ) = target.call{value: value}(payload);
+            if (!success) {
+                revert TargetExecFailed(target, value, payload);
+            }
+        }
+    }
+}

contracts/bridges/OptimismMessenger.sol

@@ -1,169 +1,82 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.23;
 
+import {BridgeMessenger} from "./BridgeMessenger.sol";
+
 /// @dev Interface to the CrossDomainMessenger (CDM) Contract Proxy.
 interface ICrossDomainMessenger {
     function xDomainMessageSender() external returns (address);
 }
 
-/// @dev Provided zero address.
-error ZeroAddress();
-
-/// @dev Only self contract is allowed to call the function.
-/// @param sender Sender address.
-/// @param instance Required contract instance address.
-error SelfCallOnly(address sender, address instance);
-
-/// @dev Only `CDMContractProxyHome` is allowed to call the function.
-/// @param sender Sender address.
-/// @param CDMContractProxyHome Required CDM Contract Proxy (Home) address.
-error CDMContractProxyHomeOnly(address sender, address CDMContractProxyHome);
-
-/// @dev Only on behalf of `foreignGovernor` the function is allowed to process the data.
-/// @param sender Sender address.
-/// @param foreignGovernor Required Foreign Governor address.
-error ForeignGovernorOnly(address sender, address foreignGovernor);
-
-/// @dev Provided incorrect data length.
-/// @param expected Expected minimum data length.
-/// @param provided Provided data length.
-error IncorrectDataLength(uint256 expected, uint256 provided);
-
-/// @dev Provided value is bigger than the actual balance.
-/// @param value Provided value.
-/// @param balance Actual balance.
-error InsufficientBalance(uint256 value, uint256 balance);
-
-/// @dev Target execution failed.
-/// @param target Target address.
-/// @param value Provided value.
-/// @param payload Provided payload.
-error TargetExecFailed(address target, uint256 value, bytes payload);
-
 /// @title OptimismMessenger - Smart contract for the governor home (Optimism) bridge implementation
 /// @author Aleksandr Kuperman - <[email protected]>
 /// @author Andrey Lebedev - <[email protected]>
 /// @author Mariapia Moscatiello - <[email protected]>
-contract OptimismMessenger {
-    event FundsReceived(address indexed sender, uint256 value);
-    event ForeignGovernorUpdated(address indexed foreignMessageSender);
-    event MessageReceived(address indexed foreignMessageSender, bytes data);
+contract OptimismMessenger is BridgeMessenger {
+    event SourceGovernorUpdated(address indexed sourceGovernor);
+    event MessageReceived(address indexed sourceMessageSender, bytes data);
 
-    // Default payload data length includes the number of bytes of at least one address (20 bytes or 160 bits),
-    // value (12 bytes or 96 bits) and the payload size (4 bytes or 32 bits)
-    uint256 public constant DEFAULT_DATA_LENGTH = 36;
-    // CDM Contract Proxy (Home) address on L2 that receives the message across the bridge from the foreign L1 network
+    // CDM Contract Proxy (Home) address on L2 that receives the message across the bridge from the source L1 network
     address public immutable CDMContractProxyHome;
-    // Foreign governor address on L1 that is authorized to propagate the transaction execution across the bridge
-    address public foreignGovernor;
+    // Source governor address on L1 that is authorized to propagate the transaction execution across the bridge
+    address public sourceGovernor;
 
     /// @dev OptimismMessenger constructor.
     /// @param _CDMContractProxyHome CDM Contract Proxy (Home) address (Optimism).
-    /// @param _foreignGovernor Foreign Governor address (ETH).
-    constructor(address _CDMContractProxyHome, address _foreignGovernor) {
+    /// @param _sourceGovernor Source Governor address (ETH).
+    constructor(address _CDMContractProxyHome, address _sourceGovernor) {
         // Check fo zero addresses
-        if (_CDMContractProxyHome == address(0) || _foreignGovernor == address(0)) {
+        if (_CDMContractProxyHome == address(0) || _sourceGovernor == address(0)) {
             revert ZeroAddress();
         }
 
         CDMContractProxyHome = _CDMContractProxyHome;
-        foreignGovernor = _foreignGovernor;
+        sourceGovernor = _sourceGovernor;
     }
 
-    /// @dev Receives native network token.
-    receive() external payable {
-        emit FundsReceived(msg.sender, msg.value);
-    }
-
-    /// @dev Changes the Foreign Governor address (original Timelock).
-    /// @notice The only way to change the Foreign Governor address is by the Timelock on L1 to request that change.
-    ///         This triggers a self-contract transaction of OptimismMessenger that changes the Foreign Governor address.
-    /// @param newForeignGovernor New Foreign Governor address.
-    function changeForeignGovernor(address newForeignGovernor) external {
+    /// @dev Changes the source governor address (original Timelock).
+    /// @notice The only way to change the source governor address is by the Timelock on L1 to request that change.
+    ///         This triggers a self-contract transaction of BridgeMessenger that changes the source governor address.
+    /// @param newSourceGovernor New source governor address.
+    function changeSourceGovernor(address newSourceGovernor) external {
         // Check if the change is authorized by the previous governor itself
         // This is possible only if all the checks in the message process function pass and the contract calls itself
         if (msg.sender != address(this)) {
             revert SelfCallOnly(msg.sender, address(this));
         }
 
         // Check for the zero address
-        if (newForeignGovernor == address(0)) {
+        if (newSourceGovernor == address(0)) {
             revert ZeroAddress();
         }
 
-        foreignGovernor = newForeignGovernor;
-        emit ForeignGovernorUpdated(newForeignGovernor);
+        sourceGovernor = newSourceGovernor;
+        emit SourceGovernorUpdated(newSourceGovernor);
     }
 
     /// @dev Processes a message received from the CDM Contract Proxy (Home) contract.
-    /// @notice The sender must be the Foreign Governor address (Timelock).
+    /// @notice The sender must be the Source Governor address (Timelock).
     /// @param data Bytes message sent from the CDM Contract Proxy (Home) contract. The data must be encoded as a set of
     ///        continuous transactions packed into a single buffer, where each transaction is composed as follows:
     ///        - target address of 20 bytes (160 bits);
     ///        - value of 12 bytes (96 bits), as a limit for all of Autonolas ecosystem contracts;
     ///        - payload length of 4 bytes (32 bits), as 2^32 - 1 characters is more than enough to fill a whole block;
     ///        - payload as bytes, with the length equal to the specified payload length.
-    function processMessageFromForeign(bytes memory data) external payable {
+    function processMessageFromSource(bytes memory data) external payable {
         // Check for the CDM Contract Proxy (Home) address
         if (msg.sender != CDMContractProxyHome) {
-            revert CDMContractProxyHomeOnly(msg.sender, CDMContractProxyHome);
+            revert TargetRelayerOnly(msg.sender, CDMContractProxyHome);
         }
 
-        // Check for the Foreign Governor address
-        address governor = foreignGovernor;
+        // Check for the Source Governor address
+        address governor = sourceGovernor;
         address bridgeGovernor = ICrossDomainMessenger(CDMContractProxyHome).xDomainMessageSender();
         if (bridgeGovernor != governor) {
-            revert ForeignGovernorOnly(bridgeGovernor, governor);
-        }
-
-        // Check for the correct data length
-        uint256 dataLength = data.length;
-        if (dataLength < DEFAULT_DATA_LENGTH) {
-            revert IncorrectDataLength(DEFAULT_DATA_LENGTH, data.length);
+            revert SourceGovernorOnly(bridgeGovernor, governor);
         }
 
-        // Unpack and process the data
-        for (uint256 i = 0; i < dataLength;) {
-            address target;
-            uint96 value;
-            uint32 payloadLength;
-            // solhint-disable-next-line no-inline-assembly
-            assembly {
-                // First 20 bytes is the address (160 bits)
-                i := add(i, 20)
-                target := mload(add(data, i))
-                // Offset the data by 12 bytes of value (96 bits)
-                i := add(i, 12)
-                value := mload(add(data, i))
-                // Offset the data by 4 bytes of payload length (32 bits)
-                i := add(i, 4)
-                payloadLength := mload(add(data, i))
-            }
-
-            // Check for the zero address
-            if (target == address(0)) {
-                revert ZeroAddress();
-            }
-
-            // Check for the value compared to the contract's balance
-            if (value > address(this).balance) {
-                revert InsufficientBalance(value, address(this).balance);
-            }
-
-            // Get the payload
-            bytes memory payload = new bytes(payloadLength);
-            for (uint256 j = 0; j < payloadLength; ++j) {
-                payload[j] = data[i + j];
-            }
-            // Offset the data by the payload number of bytes
-            i += payloadLength;
-
-            // Call the target with the provided payload
-            (bool success, ) = target.call{value: value}(payload);
-            if (!success) {
-                revert TargetExecFailed(target, value, payload);
-            }
-        }
+        // Process the data
+        _processData(data);
 
         // Emit received message
         emit MessageReceived(governor, data);