diff --git a/src/MultiBridgeMessageReceiver.sol b/src/MultiBridgeMessageReceiver.sol index 52d95c9..fa1f82b 100644 --- a/src/MultiBridgeMessageReceiver.sol +++ b/src/MultiBridgeMessageReceiver.sol @@ -15,9 +15,9 @@ import "./libraries/Error.sol"; import "./libraries/Message.sol"; /// @title Multi-bridge message receiver. -/// @notice This contract is deployed on each destination chain, and receives messages sent by the MultiBridgeMessageSender.sol +/// @notice This contract is deployed on each destination chain, and receives messages sent by the MultiBridgeMessageSender /// contract on the source chain, through multiple bridge adapters. A message is considered valid and can be executed -/// if it has been delivered by enough trusted bridge receiver adapters (i.e. has achieved a quorum threshold), +/// if it has been delivered by enough trusted bridge receiver adapters (i.e. has achieved a configured quorum threshold), /// before the message's expiration. If a message is successfully validated in time, it is queued for execution on the /// governance timelock contract. /// @dev The contract only accepts messages from trusted bridge receiver adapters, each of which implements the diff --git a/src/MultiBridgeMessageSender.sol b/src/MultiBridgeMessageSender.sol index a3f9f49..92da406 100644 --- a/src/MultiBridgeMessageSender.sol +++ b/src/MultiBridgeMessageSender.sol @@ -25,8 +25,8 @@ contract MultiBridgeMessageSender { MessageSenderGAC public immutable senderGAC; /// @dev the minimum and maximum duration that a message's expiration parameter can be set to - uint256 public constant MIN_EXPIRATION = 2 days; - uint256 public constant MAX_EXPIRATION = 30 days; + uint256 public constant MIN_MESSAGE_EXPIRATION = 2 days; + uint256 public constant MAX_MESSAGE_EXPIRATION = 30 days; /*///////////////////////////////////////////////////////////////// STATE VARIABLES @@ -98,7 +98,7 @@ contract MultiBridgeMessageSender { /// @dev validates the expiration provided by the user modifier validateExpiration(uint256 _expiration) { - if (_expiration < MIN_EXPIRATION || _expiration > MAX_EXPIRATION) { + if (_expiration < MIN_MESSAGE_EXPIRATION || _expiration > MAX_MESSAGE_EXPIRATION) { revert Error.INVALID_EXPIRATION_DURATION(); } diff --git a/src/controllers/GovernanceTimelock.sol b/src/controllers/GovernanceTimelock.sol index b75c007..283698a 100644 --- a/src/controllers/GovernanceTimelock.sol +++ b/src/controllers/GovernanceTimelock.sol @@ -21,7 +21,7 @@ contract GovernanceTimelock is IGovernanceTimelock { STATE VARIABLES ////////////////////////////////////////////////////////////////*/ uint256 public txCounter; - uint256 public delay = MINIMUM_DELAY; + uint256 public delay; /// @notice the admin is the one allowed to schedule events address public admin; diff --git a/src/interfaces/IMultiBridgeMessageReceiver.sol b/src/interfaces/IMultiBridgeMessageReceiver.sol index bbff20e..42af4f9 100644 --- a/src/interfaces/IMultiBridgeMessageReceiver.sol +++ b/src/interfaces/IMultiBridgeMessageReceiver.sol @@ -3,9 +3,9 @@ pragma solidity >=0.8.9; import "../libraries/Message.sol"; -/// @dev interface for the multi-bridge message receiver +/// @notice interface for the multi-bridge message receiver interface IMultiBridgeMessageReceiver { - /// @notice encapsulates data that is relevant to a message's intended transaction execution + /// @notice encapsulates data that is relevant to a message's intended transaction execution. struct ExecutionData { // target contract address on the destination chain address target; @@ -19,7 +19,7 @@ interface IMultiBridgeMessageReceiver { uint256 expiration; } - /// @notice emitted when a message has been received from a single bridge + /// @notice emitted when a message has been received from a single bridge. /// @param msgId is the unique identifier of the message /// @param bridgeName is the name of the bridge from which the message was received /// @param nonce is the nonce of the message @@ -28,7 +28,7 @@ interface IMultiBridgeMessageReceiver { bytes32 indexed msgId, string indexed bridgeName, uint256 nonce, address receiverAdapter ); - /// @notice emitted when a message has been queued for execution in the destination timelock contract + /// @notice emitted when a message has been queued for execution in the destination timelock contract. /// @param msgId is the unique identifier of the message /// @param target is the address of the final target address that will be called once the timelock matures /// @param nativeValue is the value that will be passed to the target address through low-level call @@ -38,12 +38,12 @@ interface IMultiBridgeMessageReceiver { bytes32 indexed msgId, address indexed target, uint256 nativeValue, uint256 nonce, bytes callData ); - /// @notice emitted when receiver adapter of a specific bridge is updated + /// @notice emitted when receiver adapter of a specific bridge is updated. /// @param receiverAdapter is the new receiver adapter address /// @param add is true if the receiver adapter was added, false if removed event BridgeReceiverAdapterUpdated(address indexed receiverAdapter, bool add); - /// @notice emitted when the quorum for message validity is updated + /// @notice emitted when the quorum for message validity is updated. /// @param oldQuorum is the old quorum value /// @param newQuorum is the new quorum value event QuorumUpdated(uint64 oldQuorum, uint64 newQuorum); @@ -53,23 +53,23 @@ interface IMultiBridgeMessageReceiver { /// @param _message is the message to be received function receiveMessage(MessageLibrary.Message calldata _message) external; - /// @notice Sends a message, that has achieved quorum and has not yet expired, to the governance timelock for eventual execution + /// @notice Sends a message, that has achieved quorum and has not yet expired, to the governance timelock for eventual execution. /// @param _msgId is the unique identifier of the message function executeMessage(bytes32 _msgId) external; - /// @notice adds or removes bridge receiver adapters + /// @notice adds or removes bridge receiver adapters. /// @param _receiverAdapters the list of receiver adapters to add or remove - /// @param _operations the list of operations to perform for corresponding receiver adapters. true for add, false for remove + /// @param _operations the list of operations to perform for corresponding receiver adapters, true for add, false for remove function updateReceiverAdapters(address[] calldata _receiverAdapters, bool[] calldata _operations) external; /// @notice updates the quorum for message validity, which is the number of bridges that must deliver the message for it to be considered valid. /// @param _quorum is the new quorum value function updateQuorum(uint64 _quorum) external; - /// @notice updates the the list of receiver adapters and the quorum for message validity + /// @notice updates the the list of receiver adapters and the quorum for message validity. /// @param _newQuorum is the new quorum value /// @param _receiverAdapters the list of receiver adapters to add or remove - /// @param _operations the list of operations to perform for corresponding receiver adapters. true for add, false for remove + /// @param _operations the list of operations to perform for corresponding receiver adapters, true for add, false for remove function updateQuorumAndReceiverAdapter( uint64 _newQuorum, address[] calldata _receiverAdapters, diff --git a/src/interfaces/adapters/IMessageReceiverAdapter.sol b/src/interfaces/adapters/IMessageReceiverAdapter.sol index 2796faa..e64c371 100644 --- a/src/interfaces/adapters/IMessageReceiverAdapter.sol +++ b/src/interfaces/adapters/IMessageReceiverAdapter.sol @@ -3,21 +3,21 @@ pragma solidity >=0.8.9; import "../EIP5164/MessageExecutor.sol"; -//// @dev interface for message receiver adapters +/** + * @notice A common interface for AMB message receiver adapters, that builds on EIP-5164 MessageExecutor. + * A message receiver adapter receives messages through the AMB that are sent by a specific sender adapter on the source chain. + * It validates the message and forwards it to the IMultiBridgeMessageReceiver contract on the destination chain. + */ interface IMessageReceiverAdapter is MessageExecutor { - /*///////////////////////////////////////////////////////////////// - EVENTS - ////////////////////////////////////////////////////////////////*/ + /// @notice emitted when the sender adapter address, which resides on the source chain, is updated. + /// @param oldSenderAdapter is the old sender adapter address + /// @param newSenderAdapter is the new sender adapter address event SenderAdapterUpdated(address indexed oldSenderAdapter, address indexed newSenderAdapter); - /*///////////////////////////////////////////////////////////////// - EXTERNAL FUNCTIONS - ////////////////////////////////////////////////////////////////*/ - - /// @dev returns name of the message bridge wrapped by the adapter + /// @notice returns name of the message bridge wrapped by the adapter function name() external view returns (string memory); - /// @dev allows the admin to update the sender adapter + /// @dev Changes the address of the sender adapter on the source chain, that is authorised to send this receiver messages. /// @param _senderAdapter is the bridge's sender adapter deployed on the source chain (i.e. Ethereum) /// note: access controlled to be called by the global admin contract function updateSenderAdapter(address _senderAdapter) external; diff --git a/src/interfaces/adapters/IMessageSenderAdapter.sol b/src/interfaces/adapters/IMessageSenderAdapter.sol index 23d8e27..f4fe6f4 100644 --- a/src/interfaces/adapters/IMessageSenderAdapter.sol +++ b/src/interfaces/adapters/IMessageSenderAdapter.sol @@ -4,17 +4,22 @@ pragma solidity >=0.8.9; import "../EIP5164/SingleMessageDispatcher.sol"; -/// @dev common interface for all message sender adapters of messaging bridges +/** + * @notice A common interface for AMB message sender adapters, that builds on EIP-5164 SingleMessageDispatcher. + * A message sender adapter for a specific AMB is responsible for receiving messages from an MultiBridgeMessageSender + * on the source chain and sending the message to corresponding receiver adapters on the intended destination chain. + * The sender adapter keeps a list of the remote receiver adapters on each destination chain that it forwards messages to. + */ interface IMessageSenderAdapter is SingleMessageDispatcher { - /// @notice emitted when a the sender's corresponding receiver adapter on a remote chain is changed + /// @notice emitted when a the sender's corresponding receiver adapter on a destination chain is changed /// @param dstChainId is the destination chain for which the receiver adapter is updated /// @param oldReceiver is the old receiver adapter address /// @param newReceiver is the new receiver adapter address event ReceiverAdapterUpdated(uint256 indexed dstChainId, address indexed oldReceiver, address indexed newReceiver); - /// @notice allows owner to update the receiver adapters on different destination chains + /// @notice Updates the corresponding message receiver adapters for different destination chains /// @param _dstChainIds are the destination chain IDs for which the receiver adapters are to be updated - /// @param _receiverAdapters receiver adapter addresses for the corresponding destination chain ids in _dstChainIds + /// @param _receiverAdapters new receiver adapter addresses for the corresponding destination chain ids in _dstChainIds function updateReceiverAdapter(uint256[] calldata _dstChainIds, address[] calldata _receiverAdapters) external; /// @notice returns name of the message bridge wrapped by the adapter diff --git a/src/interfaces/controllers/IGAC.sol b/src/interfaces/controllers/IGAC.sol index f25bca7..3baeda8 100644 --- a/src/interfaces/controllers/IGAC.sol +++ b/src/interfaces/controllers/IGAC.sol @@ -4,12 +4,12 @@ pragma solidity >=0.8.9; /// @notice interface for GAC (Global Access Controller) interface IGAC { - /// @dev returns `true` if the caller is global access controller - /// @param _caller is the msg.sender to validate - /// @return boolean indicating the validity of the caller - function isGlobalOwner(address _caller) external view returns (bool); + /// @notice Checks whether a given address is the global owner + /// @param _addr is the address to compare with the global owner + /// @return boolean true if _caller is the global owner, false otherwise + function isGlobalOwner(address _addr) external view returns (bool); - /// @dev returns the global owner address + /// @notice returns the global owner address. /// @return _owner is the global owner address function getGlobalOwner() external view returns (address _owner); } diff --git a/src/interfaces/controllers/IGovernanceTimelock.sol b/src/interfaces/controllers/IGovernanceTimelock.sol index 7168c2a..93972f4 100644 --- a/src/interfaces/controllers/IGovernanceTimelock.sol +++ b/src/interfaces/controllers/IGovernanceTimelock.sol @@ -1,44 +1,65 @@ // SPDX-License-Identifier: GPL-3.0-only pragma solidity >=0.8.9; -/// @dev interface for governance timelock before execution events on dst chain +/** + * @notice interface for governance timelock on the destination chain. + * @dev this contract is responsible for executing all cross-chain messages on the destination chain + * after they have been queued for a configurable period of time. + * This contract is also typically, the GAC (Global Access Controller) on the destination chain, ensuring that all + * admin actions on the destination chain go through the same validation process and time delay as other governance actions. + */ interface IGovernanceTimelock { - /*///////////////////////////////////////////////////////////////// - EVENTS - ////////////////////////////////////////////////////////////////*/ + /// @notice emitted when a transaction is scheduled for execution. + /// @param txId is the generated unique identifier of the scheduled transaction + /// @param target is the address to call by low-level call + /// @param value is the value to pass to target by low-level call + /// @param data is the abieencoded function selector and arguments data, to execute on target + /// @param eta is the timestamp after which the transaction should be executed event TransactionScheduled(uint256 indexed txId, address indexed target, uint256 value, bytes data, uint256 eta); + + /// @notice emitted when a transaction is executed. + /// @param txId is the unique identifier of the executed transaction, generated when the transaction was scheduled + /// @param target is the address called as part of the execution + /// @param value is the value passed to target by low-level call + /// @param data is the abieencoded function selector and arguments data, executed on target + /// @param eta is the timestamp after which the transaction should be executed event TransactionExecuted(uint256 indexed txId, address indexed target, uint256 value, bytes data, uint256 eta); - event ExecutionPeriodUpdated(uint256 oldPeriod, uint256 newPeriod); + /// @notice emitted when the time delay parameter is changed. + /// @param oldDelay is the previous value of the time delay + /// @param newDelay is the new value of the time delay event DelayUpdated(uint256 oldDelay, uint256 newDelay); - event AdminUpdated(address oldAdmin, address newAdmin); - /*///////////////////////////////////////////////////////////////// - EXTERNAL FUNCTIONS - ////////////////////////////////////////////////////////////////*/ + /// @notice emitted when the admin of the time lock contract is changed + /// @param oldAdmin is the previous admin of the time lock contract + /// @param newAdmin is the new admin of the time lock contract + event AdminUpdated(address oldAdmin, address newAdmin); - /// @notice Schedules the provided transaction for execution after a specified ETA. - /// @dev this function can only be called by an authorised bridge adapter on the remote chain - /// @param _target the contract to call - /// @param _value the amount to pass when calling target - /// @param _data the abieencoded function selector and arguments data, to execute on target + /// @notice Schedules the provided transaction for execution after a pre-configured delay period. + /// @dev this function can only be called by the admin of the timelock contract. + /// @param _target is the address to call by low-level call + /// @param _value is the value to pass to target by low-level call + /// @param _data is the abieencoded function selector and arguments data, to execute on target function scheduleTransaction(address _target, uint256 _value, bytes memory _data) external; - /// @notice Executes a previously scheduled transaction if it has reached its ETA. - /// @param _txId is the unqiue identifier of the scheduled transaction - /// @param _target the contract to call - /// @param _value the amount to pass when calling target - /// @param _data the abieencoded function selector and arguments data, to execute on target - /// @param _eta the time after which the transaction should be executed + /// @notice Executes a previously scheduled transaction if it has reached its ETA, but has not exceeded a grace period beyond that. + /// @param _txId is the unique identifier of the executed transaction, generated when the transaction was scheduled + /// @param _target is the address called as part of the execution + /// @param _value is the value passed to target by low-level call + /// @param _data is the abieencoded function selector and arguments data, executed on target + /// @param _eta is the timestamp after which the transaction should be executed function executeTransaction(uint256 _txId, address _target, uint256 _value, bytes memory _data, uint256 _eta) external payable; - /// @notice Updates the minimum delay for a transaction before it can be executed. - /// @dev This can only be invoked by the timelock contract itself, thus requiring that this change go through the same time delay as other governance actions. + /// @notice Changes the time period that transactions must be queued for before they can be executed. + /// The new delay must still be within an allowed range. + /// This function can only be invoked by the timelock contract itself, thus requiring that this change go + /// through the process and time delays as other governance actions. function setDelay(uint256 _delay) external; - /// @notice Updates the admin. - /// @dev This can only be invoked by the timelock contract itself, thus requiring that this change go through the same time delay as other governance actions. + /// @notice Changes the admin. + /// This function can only be invoked by the timelock contract itself, thus requiring that this change go + /// through the process and time delays as other governance actions. function setAdmin(address newAdmin) external; } diff --git a/test/unit-tests/GovernanceTimelock.t.sol b/test/unit-tests/GovernanceTimelock.t.sol index 9717d94..c3ad31b 100644 --- a/test/unit-tests/GovernanceTimelock.t.sol +++ b/test/unit-tests/GovernanceTimelock.t.sol @@ -13,7 +13,6 @@ contract GovernanceTimelockTest is Setup { event TransactionScheduled(uint256 indexed txId, address indexed target, uint256 value, bytes data, uint256 eta); event TransactionExecuted(uint256 indexed txId, address indexed target, uint256 value, bytes data, uint256 eta); - event ExecutionPeriodUpdated(uint256 oldPeriod, uint256 newPeriod); event DelayUpdated(uint256 oldDelay, uint256 newDelay); event AdminUpdated(address oldAdmin, address newAdmin); diff --git a/test/unit-tests/MultiBridgeMessageSender.t.sol b/test/unit-tests/MultiBridgeMessageSender.t.sol index fc963d8..9f8227d 100644 --- a/test/unit-tests/MultiBridgeMessageSender.t.sol +++ b/test/unit-tests/MultiBridgeMessageSender.t.sol @@ -183,8 +183,8 @@ contract MultiBridgeMessageSenderTest is Setup { /// @dev message expiration has to be within allowed range function test_remote_call_invalid_expiration() public { - uint256 invalidExpMin = sender.MIN_EXPIRATION() - 1 days; - uint256 invalidExpMax = sender.MAX_EXPIRATION() + 1 days; + uint256 invalidExpMin = sender.MIN_MESSAGE_EXPIRATION() - 1 days; + uint256 invalidExpMax = sender.MAX_MESSAGE_EXPIRATION() + 1 days; // test expiration validation in remoteCall() which does not accept excluded adapters vm.startPrank(caller);