Skip to content

Commit

Permalink
Remove upgradeability (#85)
Browse files Browse the repository at this point in the history 
* feat: remove circular dependency

* Fix failing tests

* Make GAC and source chain immutable

---------

Co-authored-by: sujithsomraaj <somraajsujith@gmail.com>
  • Loading branch information
@ermyas @sujithsomraaj
ermyas and sujithsomraaj authored Sep 24, 2023
1 parent af18f6d commit dd0ea1e
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 177 deletions.
59 changes: 25 additions & 34 deletions src/MultiBridgeMessageReceiver.sol
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity >=0.8.9;

/// external modules
import "@openzeppelin/contracts/proxy/utils/Initializable.sol";

/// interfaces
import "./interfaces/controllers/IGAC.sol";
import "./interfaces/adapters/IMessageReceiverAdapter.sol";
import "./interfaces/IMultiBridgeMessageReceiver.sol";
import "./interfaces/EIP5164/ExecutorAware.sol";
Expand All @@ -22,14 +20,16 @@ import "./libraries/Message.sol";
/// governance timelock contract.
/// @dev The contract only accepts messages from trusted bridge receiver adapters, each of which implements the
/// IMessageReceiverAdapter interface.
contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAware, Initializable {
contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAware {
/// @notice the id of the source chain that this contract can receive messages from
uint256 public immutable srcChainId;
/// @notice the global access control contract
IGAC public immutable gac;

/*/////////////////////////////////////////////////////////////////
STATE VARIABLES
////////////////////////////////////////////////////////////////*/

/// @notice the id of the source chain that this contract can receive messages from
uint256 public srcChainId;

/// @notice minimum number of bridges that must deliver a message for it to be considered valid
uint64 public quorum;

Expand Down Expand Up @@ -61,39 +61,21 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
}

/// @notice A modifier used for restricting the caller to just the governance timelock contract
modifier onlyGovernanceTimelock() {
if (msg.sender != governanceTimelock) {
revert Error.CALLER_NOT_GOVERNANCE_TIMELOCK();
modifier onlyOwner() {
if (!gac.isGlobalOwner(msg.sender)) {
revert Error.CALLER_NOT_OWNER();
}
_;
}

/*/////////////////////////////////////////////////////////////////
INITIALIZER
CONSTRUCTOR
////////////////////////////////////////////////////////////////*/

/// @notice sets the initial parameters
function initialize(
uint256 _srcChainId,
address[] calldata _receiverAdapters,
bool[] calldata _operations,
uint64 _quorum,
address _governanceTimelock
) external initializer {
if (_governanceTimelock == address(0)) {
revert Error.ZERO_GOVERNANCE_TIMELOCK();
}
governanceTimelock = _governanceTimelock;

/// @dev adds the new receiver adapters before setting quorum and validations to account for duplicates
_updateReceiverAdapters(_receiverAdapters, _operations);

if (_quorum > trustedExecutorsCount() || _quorum == 0) {
revert Error.INVALID_QUORUM_THRESHOLD();
}
quorum = _quorum;

constructor(uint256 _srcChainId, address _gac) {
srcChainId = _srcChainId;
gac = IGAC(_gac);
}

/*/////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -175,12 +157,21 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
emit MessageExecuted(msgId, _execData.target, _execData.value, _execData.nonce, _execData.callData);
}

/// @notice update the governance timelock contract.
/// @dev called by admin to update the timelock contract
function updateGovernanceTimelock(address _governanceTimelock) external onlyOwner {
if (_governanceTimelock == address(0)) {
revert Error.ZERO_GOVERNANCE_TIMELOCK();
}
governanceTimelock = _governanceTimelock;
}

/// @notice Update bridge receiver adapters.
/// @dev called by admin to update receiver bridge adapters on all other chains
function updateReceiverAdapters(address[] calldata _receiverAdapters, bool[] calldata _operations)
external
override
onlyGovernanceTimelock
onlyOwner
{
_updateReceiverAdapters(_receiverAdapters, _operations);
}
Expand All @@ -191,7 +182,7 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
uint64 _newQuorum,
address[] calldata _receiverAdapters,
bool[] calldata _operations
) external override onlyGovernanceTimelock {
) external override onlyOwner {
/// @dev updates quorum here
_updateQuorum(_newQuorum);

Expand All @@ -200,7 +191,7 @@ contract MultiBridgeMessageReceiver is IMultiBridgeMessageReceiver, ExecutorAwar
}

/// @notice Update power quorum threshold of message execution.
function updateQuorum(uint64 _quorum) external override onlyGovernanceTimelock {
function updateQuorum(uint64 _quorum) external override onlyOwner {
_updateQuorum(_quorum);
}

Expand Down
3 changes: 0 additions & 3 deletions src/libraries/Error.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ library Error {
/// @dev is thrown if message execution fails on the destination chain
error EXECUTION_FAILS_ON_DST();

/// @dev is thrown if caller is not governance timelock contract
error CALLER_NOT_GOVERNANCE_TIMELOCK();

/// @dev is thrown if caller is not admin of timelock
error CALLER_NOT_ADMIN();

Expand Down
49 changes: 39 additions & 10 deletions test/Setup.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ pragma solidity >=0.8.9;
/// library imports
import {Test, Vm} from "forge-std/Test.sol";

import "openzeppelin-contracts/contracts/access/Ownable.sol";

/// @dev imports from Pigeon Helper (Facilitate State Transfer Mocks)
import {WormholeHelper} from "pigeon/wormhole/WormholeHelper.sol";
import {AxelarHelper} from "pigeon/axelar/AxelarHelper.sol";
Expand Down Expand Up @@ -121,6 +123,9 @@ abstract contract Setup is Test {

/// @dev setup adapter contracts
_setupAdapters();

/// @dev gives up owner to timelock
_setupTimelockAsOwner();
}

/*///////////////////////////////////////////////////////////////
Expand All @@ -140,9 +145,10 @@ abstract contract Setup is Test {
senderGAC.setAuthorisedCaller(caller);
gac = address(senderGAC);
} else {
gac = address(new MessageReceiverGAC{salt: _salt}());
MessageReceiverGAC receiverGAC = new MessageReceiverGAC{salt: _salt}();
gac = address(receiverGAC);
}
contractAddress[chainId][bytes("GAC")] = gac;
contractAddress[chainId][bytes("GAC")] = address(gac);

unchecked {
++i;
Expand Down Expand Up @@ -269,7 +275,9 @@ abstract contract Setup is Test {
uint256 chainId = DST_CHAINS[i];

vm.selectFork(fork[chainId]);
address mmaReceiver = address(new MultiBridgeMessageReceiver{salt: _salt}());
address mmaReceiver = address(
new MultiBridgeMessageReceiver{salt: _salt}(SRC_CHAIN_ID, contractAddress[chainId][bytes("GAC")])
);
contractAddress[chainId][bytes("MMA_RECEIVER")] = mmaReceiver;
contractAddress[chainId][bytes("TIMELOCK")] =
address(address(new GovernanceTimelock{salt: _salt}(mmaReceiver, 3 days)));
Expand Down Expand Up @@ -304,18 +312,19 @@ abstract contract Setup is Test {
_operations[0] = true;
_operations[1] = true;

/// setup receiver adapters
vm.selectFork(fork[chainId]);
address dstMMReceiver = contractAddress[chainId][bytes("MMA_RECEIVER")];
MultiBridgeMessageReceiver(dstMMReceiver).initialize(
SRC_CHAIN_ID, _receiverAdapters, _operations, 2, contractAddress[chainId]["TIMELOCK"]
);

MultiBridgeMessageReceiver dstMMReceiver =
MultiBridgeMessageReceiver(contractAddress[chainId][bytes("MMA_RECEIVER")]);
dstMMReceiver.updateReceiverAdapters(_receiverAdapters, _operations);
dstMMReceiver.updateGovernanceTimelock(contractAddress[chainId]["TIMELOCK"]);
dstMMReceiver.updateQuorum(2);

MessageReceiverGAC receiverGAC = MessageReceiverGAC(contractAddress[chainId][bytes("GAC")]);
receiverGAC.setMultiBridgeMessageReceiver(dstMMReceiver);
receiverGAC.setMultiBridgeMessageReceiver(address(dstMMReceiver));

vm.selectFork(fork[SRC_CHAIN_ID]);
senderGAC.setRemoteMultiBridgeMessageReceiver(chainId, dstMMReceiver);
senderGAC.setRemoteMultiBridgeMessageReceiver(chainId, address(dstMMReceiver));

unchecked {
++i;
Expand Down Expand Up @@ -345,6 +354,26 @@ abstract contract Setup is Test {
}
}

/// @dev admin gives up ownership to
function _setupTimelockAsOwner() internal {
/// transfer ownership to timelock finally
for (uint256 i; i < ALL_CHAINS.length;) {
uint256 chainId = ALL_CHAINS[i];

vm.selectFork(fork[chainId]);

if (chainId != SRC_CHAIN_ID) {
GAC(contractAddress[chainId][bytes("GAC")]).transferOwnership(
contractAddress[chainId][bytes("TIMELOCK")]
);
}

unchecked {
++i;
}
}
}

/// @dev helps payload delivery using logs
function _simulatePayloadDelivery(uint256 _srcChainId, uint256 _dstChainId, Vm.Log[] memory _logs) internal {
/// simulate wormhole off-chain infra
Expand Down
14 changes: 8 additions & 6 deletions test/unit-tests/MessageReceiverGAC.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,25 @@ contract MessageReceiverGACTest is Setup {
event MultiBridgeMessageReceiverUpdated(address indexed oldReceiver, address indexed newReceiver);

MessageReceiverGAC receiverGAC;

address gacOwner;
/// @dev initializes the setup

function setUp() public override {
super.setUp();
vm.selectFork(fork[DST_CHAIN_ID]);
receiverGAC = MessageReceiverGAC(contractAddress[DST_CHAIN_ID]["GAC"]);
gacOwner = contractAddress[DST_CHAIN_ID]["TIMELOCK"];
}

/// @dev constructor
function test_constructor() public {
// checks existing setup
assertEq(address(Ownable(address(receiverGAC)).owner()), owner);
assertEq(address(Ownable(address(receiverGAC)).owner()), gacOwner);
}

/// @dev sets multi message receiver
function test_set_multi_message_receiver() public {
vm.startPrank(owner);
vm.startPrank(gacOwner);

vm.expectEmit(true, true, true, true, address(receiverGAC));
emit MultiBridgeMessageReceiverUpdated(address(receiverGAC.getMultiBridgeMessageReceiver()), address(42));
Expand All @@ -54,20 +56,20 @@ contract MessageReceiverGACTest is Setup {

/// @dev cannot set multi message receiver to zero address
function test_set_multi_message_receiver_zero_address() public {
vm.startPrank(owner);
vm.startPrank(gacOwner);

vm.expectRevert(Error.ZERO_ADDRESS_INPUT.selector);
receiverGAC.setMultiBridgeMessageReceiver(address(0));
}

/// @dev checks if address is the global owner
function test_is_global_owner() public {
assertTrue(receiverGAC.isGlobalOwner(owner));
assertTrue(receiverGAC.isGlobalOwner(gacOwner));
assertFalse(receiverGAC.isGlobalOwner(caller));
}

/// @dev gets the global owner
function test_get_global_owner() public {
assertEq(receiverGAC.getGlobalOwner(), owner);
assertEq(receiverGAC.getGlobalOwner(), gacOwner);
}
}
Loading

0 comments on commit dd0ea1e

Please sign in to comment.