Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: wormhole bridging for governance #116

Merged
merged 15 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 102 additions & 54 deletions abis/0.8.23/OptimismMessenger.json

Large diffs are not rendered by default.

370 changes: 370 additions & 0 deletions abis/0.8.23/WormholeMessenger.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions abis/test/WormholeRelayer.json

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions audits/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
124 changes: 124 additions & 0 deletions audits/internal9/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
# 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.
[x] Fixed

#### 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
```
[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>
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
```
[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.
```
// 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);
}
}
```
[x] Fixed








173 changes: 173 additions & 0 deletions audits/internal9/analysis/contracts/OptimismMessenger-flatten.sol
Original file line number Diff line number Diff line change
@@ -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);
}
}
Loading
Loading