Skip to content

Commit

Permalink
refactor and test: accounting for the audit
Browse files Browse the repository at this point in the history
  • Loading branch information
kupermind committed Feb 28, 2024
1 parent 3f0494e commit cb8c79a
Show file tree
Hide file tree
Showing 7 changed files with 625 additions and 31 deletions.
5 changes: 5 additions & 0 deletions audits/internal9/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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>
Expand All @@ -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.
Expand Down Expand Up @@ -109,6 +113,7 @@ It makes sense to separate it into a separate contract and replace magic numbers
}
}
```
[x] Fixed



Expand Down
23 changes: 0 additions & 23 deletions contracts/bridges/BridgeMessenger.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,16 @@ import {IBridgeErrors} from "../interfaces/IBridgeErrors.sol";
/// @author Mariapia Moscatiello - <[email protected]>
abstract contract BridgeMessenger is IBridgeErrors {
event FundsReceived(address indexed sender, uint256 value);
event SourceGovernorUpdated(address indexed sourceGovernor);

// 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;
// Source governor address on L1 that is authorized to propagate the transaction execution across the bridge
address public sourceGovernor;

/// @dev Receives native network token.
receive() external payable {
emit FundsReceived(msg.sender, msg.value);
}

/// @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 virtual {
// 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 (newSourceGovernor == address(0)) {
revert ZeroAddress();
}

sourceGovernor = newSourceGovernor;
emit SourceGovernorUpdated(newSourceGovernor);
}

/// @dev Processes received data.
/// @param data Bytes message sent from L2 Wormhole 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:
Expand Down
23 changes: 23 additions & 0 deletions contracts/bridges/OptimismMessenger.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ interface ICrossDomainMessenger {
/// @author Andrey Lebedev - <[email protected]>
/// @author Mariapia Moscatiello - <[email protected]>
contract OptimismMessenger is BridgeMessenger {
event SourceGovernorUpdated(address indexed sourceGovernor);
event MessageReceived(address indexed sourceMessageSender, bytes data);

// CDM Contract Proxy (Home) address on L2 that receives the message across the bridge from the source L1 network
address public immutable CDMContractProxyHome;
// 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).
Expand All @@ -31,6 +34,26 @@ contract OptimismMessenger is BridgeMessenger {
sourceGovernor = _sourceGovernor;
}

/// @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 virtual {
// 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 (newSourceGovernor == address(0)) {
revert ZeroAddress();
}

sourceGovernor = newSourceGovernor;
emit SourceGovernorUpdated(newSourceGovernor);
}

/// @dev Processes a message received from the CDM Contract Proxy (Home) contract.
/// @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
Expand Down
38 changes: 30 additions & 8 deletions contracts/bridges/WormholeMessenger.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,30 @@ import {BridgeMessenger} from "./BridgeMessenger.sol";
/// @author Andrey Lebedev - <[email protected]>
/// @author Mariapia Moscatiello - <[email protected]>
contract WormholeMessenger is BridgeMessenger {
event MessageReceived(address indexed sourceMessageSender, bytes data, bytes32 deliveryHash, uint256 sourceChain);
event SourceGovernorUpdated(bytes32 indexed sourceGovernor);
event MessageReceived(bytes32 indexed sourceMessageSender, bytes data, bytes32 deliveryHash, uint256 sourceChain);

// L2 Wormhole Relayer address that receives the message across the bridge from the source L1 network
address public immutable wormholeRelayer;
// Source governor chain Id
uint16 public immutable sourceGovernorChainId;
// Source governor address on L1 that is authorized to propagate the transaction execution across the bridge
bytes32 public sourceGovernor;
// Source governor address on L1 that is authorized to propagate the transaction execution across the bridge
mapping(bytes32 => bool) public mapDeliveryHashes;

/// @dev WormholeMessenger constructor.
/// @param _wormholeRelayer L2 Wormhole Relayer address.
/// @param _sourceGovernor Source governor address (ETH).
/// @param _sourceGovernorChainId Source governor wormhole format chain Id.
constructor(address _wormholeRelayer, address _sourceGovernor, uint16 _sourceGovernorChainId) {
constructor(address _wormholeRelayer, bytes32 _sourceGovernor, uint16 _sourceGovernorChainId) {
// Check for zero addresses
if (_wormholeRelayer == address(0) || _sourceGovernor == address(0)) {
if (_wormholeRelayer == address(0)) {
revert ZeroAddress();
}

// Check source governor chain Id
if (_sourceGovernorChainId == 0) {
if (_sourceGovernor == 0 || _sourceGovernorChainId == 0) {
revert ZeroValue();
}

Expand All @@ -37,6 +40,26 @@ contract WormholeMessenger is BridgeMessenger {
sourceGovernorChainId = _sourceGovernorChainId;
}

/// @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(bytes32 newSourceGovernor) external virtual {
// 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 (newSourceGovernor == 0) {
revert ZeroValue();
}

sourceGovernor = newSourceGovernor;
emit SourceGovernorUpdated(newSourceGovernor);
}

/// @dev Processes a message received from L2 Wormhole Relayer contract.
/// @notice The sender must be the source governor address (Timelock).
/// @param data Bytes message sent from L2 Wormhole Relayer contract. The data must be encoded as a set of
Expand Down Expand Up @@ -66,10 +89,9 @@ contract WormholeMessenger is BridgeMessenger {
}

// Check for the source governor address
address governor = sourceGovernor;
address bridgeGovernor = address(uint160(uint256(sourceAddress)));
if (bridgeGovernor != governor) {
revert SourceGovernorOnly(bridgeGovernor, governor);
bytes32 governor = sourceGovernor;
if (governor != sourceAddress) {
revert SourceGovernorOnly32(sourceAddress, governor);
}

// Check the delivery hash uniqueness
Expand Down
5 changes: 5 additions & 0 deletions contracts/interfaces/IBridgeErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ interface IBridgeErrors {
/// @param sourceGovernor Required source governor address.
error SourceGovernorOnly(address sender, address sourceGovernor);

/// @dev Only on behalf of `sourceGovernor` the function is allowed to process the data.
/// @param sender Sender address.
/// @param sourceGovernor Required source governor address.
error SourceGovernorOnly32(bytes32 sender, bytes32 sourceGovernor);

/// @dev The message with a specified hash has already been delivered.
/// @param deliveryHash Delivery hash.
error AlreadyDelivered(bytes32 deliveryHash);
Expand Down
Loading

0 comments on commit cb8c79a

Please sign in to comment.