-
Notifications
You must be signed in to change notification settings - Fork 4
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge remote-tracking branch 'origin/v1.1.10-internal-audit' into bri…
…dge_testing
- Loading branch information
Showing
25 changed files
with
1,220 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} | ||
``` | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
173 changes: 173 additions & 0 deletions
173
audits/internal9/analysis/contracts/OptimismMessenger-flatten.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
} |
Oops, something went wrong.