doc: internal audit based on Optimism and Wormhole contracts

valory-xyz · Feb 28, 2024 · 9ba2981 · 9ba2981
1 parent 50a336a
commit 9ba2981
Show file tree

Hide file tree

Showing 25 changed files with 1,220 additions and 0 deletions.
diff --git a/audits/README.md b/audits/README.md
@@ -21,6 +21,8 @@ An internal audit with a focus on `ERC20 bridging via fx-tunnel` is located in t
 
 An internal audit with a focus on `Update for Community Multisig (CM)` is located in this folder: [internal audit 8](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal8).
 
+An internal audit with a focus on `OptimismMesseger and WormholeMessenger` is located in this folder: [internal audit 9](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal9).
+
 ### External audit
 Following the initial contracts [audit report](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Valory%20Review%20Final.pdf),
 the recommendations are addressed here: [feedback](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Addressing%20Initial%20ApeWorX%20Recommentations.pdf).

diff --git a/audits/internal9/README.md b/audits/internal9/README.md
@@ -0,0 +1,119 @@
+# autonolas-governance-audit
+The review has been performed based on the contract code in the following repository:<br>
+`https://github.com/valory-xyz/autonolas-governance` <br>
+commit: `50a336a2e31f980399f8c84ba6dad9cb9c673aaa` or `v1.1.10-pre-internal-audi` <br> 
+
+Update: 28-02-2024  <br>
+
+## Objectives
+The audit focused on governance using native bridging to Optimism/Base and Wormhole bridging to EVM-networks supported by Standard Relayer. <BR>
+
+### Flatten version
+Flatten version of contracts. [contracts](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal9/analysis/contracts)
+
+### Coverage
+Hardhat coverage has been performed before the audit and can be found here:
+```sh
+---------------------------|----------|----------|----------|----------|----------------|
+File                       |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
+  OptimismMessenger.sol    |        0 |        0 |        0 |        0 |... 163,164,169 |
+  WormholeMessenger.sol    |        0 |        0 |        0 |        0 |... 201,202,207 |
+```
+Pay attention, please! No tests for contract on scope.
+
+#### Wormhole security list
+https://docs.wormhole.com/wormhole/quick-start/cross-chain-dev/standard-relayer#other-considerations
+```
+Receiving a message from relayer 
+ Check for expected emitter [x]
+ call parseAndVerify on any additionalVAAs [N/A]
+Replay protection [x]
+Message Ordering 
+ no guarantees on order of messages delivered [?] - requires discussion
+Fowarding/Call Chaining - [x], by design
+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
+```
+
+### Security issues
+Details in [slither_full](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal9/analysis/slither_full.txt) <br>
+Notes: <br>
+All is false positive.
+
+### Possible optimization
+##### Avoid casting
+```
+    // Source governor address on L1 that is authorized to propagate the transaction execution across the bridge
+    address public sourceGovernor;
+=>
+set sourceGovernor in wormhole address format as bytes32
+so avoid casting bytes32 to address EVM.
+        // Check for the source governor address
+        bytes32 governor = sourceGovernor;
+        if (governor != sourceAddress) {
+            revert SourceGovernorOnly(bridgeGovernor, governor);
+        }
+ref: https://github.com/wormhole-foundation/wormhole-solidity-sdk/blob/main/src/Base.sol#L30C52-L30C74 SDK of Wormhole
+```
+##### 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.
+```
+// 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);
+            }
+        }
+```
+
+
+
+
+
+
+
+
diff --git a/audits/internal9/analysis/contracts/OptimismMessenger-flatten.sol b/audits/internal9/analysis/contracts/OptimismMessenger-flatten.sol
@@ -0,0 +1,173 @@
+// Sources flattened with hardhat v2.20.1 https://hardhat.org
+
+// Original license: SPDX_License_Identifier: MIT
+pragma solidity ^0.8.23;
+
+/// @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);
+
+    // 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
+    address public immutable CDMContractProxyHome;
+    // Foreign governor address on L1 that is authorized to propagate the transaction execution across the bridge
+    address public foreignGovernor;
+
+    /// @dev OptimismMessenger constructor.
+    /// @param _CDMContractProxyHome CDM Contract Proxy (Home) address (Optimism).
+    /// @param _foreignGovernor Foreign Governor address (ETH).
+    constructor(address _CDMContractProxyHome, address _foreignGovernor) {
+        // Check fo zero addresses
+        if (_CDMContractProxyHome == address(0) || _foreignGovernor == address(0)) {
+            revert ZeroAddress();
+        }
+
+        CDMContractProxyHome = _CDMContractProxyHome;
+        foreignGovernor = _foreignGovernor;
+    }
+
+    /// @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 {
+        // 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)) {
+            revert ZeroAddress();
+        }
+
+        foreignGovernor = newForeignGovernor;
+        emit ForeignGovernorUpdated(newForeignGovernor);
+    }
+
+    /// @dev Processes a message received from the CDM Contract Proxy (Home) contract.
+    /// @notice The sender must be the Foreign 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 {
+        // Check for the CDM Contract Proxy (Home) address
+        if (msg.sender != CDMContractProxyHome) {
+            revert CDMContractProxyHomeOnly(msg.sender, CDMContractProxyHome);
+        }
+
+        // Check for the Foreign Governor address
+        address governor = foreignGovernor;
+        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);
+        }
+
+        // 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);
+            }
+        }
+
+        // Emit received message
+        emit MessageReceived(governor, data);
+    }
+}