From 7e25a04d4657472b66ebfd40f50d7af6663d691e Mon Sep 17 00:00:00 2001 From: Dominator008 Date: Tue, 12 Sep 2023 02:31:38 -0700 Subject: [PATCH] Add unit tests for GAC (#56) * Add unit tests for GAC Part of #45. * Cleanup caller / global owner confusion Add a sensible minimum value for dst gas limit --- src/adapters/BaseSenderAdapter.sol | 6 +- src/adapters/Celer/CelerReceiverAdapter.sol | 8 +- .../Wormhole/WormholeReceiverAdapter.sol | 12 +- .../Wormhole/WormholeSenderAdapter.sol | 2 +- src/adapters/axelar/AxelarReceiverAdapter.sol | 8 +- src/adapters/axelar/AxelarSenderAdapter.sol | 2 +- src/controllers/GAC.sol | 11 +- src/interfaces/IGAC.sol | 2 +- src/libraries/Error.sol | 6 + test/unit-tests/GAC.t.sol | 205 ++++++++++++++++++ 10 files changed, 241 insertions(+), 21 deletions(-) create mode 100644 test/unit-tests/GAC.t.sol diff --git a/src/adapters/BaseSenderAdapter.sol b/src/adapters/BaseSenderAdapter.sol index f49f3ed..40424c2 100644 --- a/src/adapters/BaseSenderAdapter.sol +++ b/src/adapters/BaseSenderAdapter.sol @@ -25,8 +25,8 @@ abstract contract BaseSenderAdapter is IBridgeSenderAdapter { _; } - modifier onlyCaller() { - if (!gac.isPrivilegedCaller(msg.sender)) { + modifier onlyGlobalOwner() { + if (!gac.isGlobalOwner(msg.sender)) { revert Error.INVALID_PRIVILEGED_CALLER(); } _; @@ -53,7 +53,7 @@ abstract contract BaseSenderAdapter is IBridgeSenderAdapter { function updateReceiverAdapter(uint256[] calldata _dstChainIds, address[] calldata _receiverAdapters) external override - onlyCaller + onlyGlobalOwner { uint256 arrLength = _dstChainIds.length; diff --git a/src/adapters/Celer/CelerReceiverAdapter.sol b/src/adapters/Celer/CelerReceiverAdapter.sol index ea20787..f98ac2a 100644 --- a/src/adapters/Celer/CelerReceiverAdapter.sol +++ b/src/adapters/Celer/CelerReceiverAdapter.sol @@ -46,9 +46,9 @@ contract CelerReceiverAdapter is IBridgeReceiverAdapter, IMessageReceiverApp { /*///////////////////////////////////////////////////////////////// MODIFIER ////////////////////////////////////////////////////////////////*/ - modifier onlyCaller() { - if (!gac.isPrivilegedCaller(msg.sender)) { - revert Error.INVALID_PRIVILEGED_CALLER(); + modifier onlyGlobalOwner() { + if (!gac.isGlobalOwner(msg.sender)) { + revert Error.CALLER_NOT_OWNER(); } _; } @@ -73,7 +73,7 @@ contract CelerReceiverAdapter is IBridgeReceiverAdapter, IMessageReceiverApp { ////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBridgeReceiverAdapter - function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyCaller { + function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyGlobalOwner { uint64 _senderChainDecoded = abi.decode(_senderChain, (uint64)); if (_senderChainDecoded == 0) { diff --git a/src/adapters/Wormhole/WormholeReceiverAdapter.sol b/src/adapters/Wormhole/WormholeReceiverAdapter.sol index 8d1cdee..f26a384 100644 --- a/src/adapters/Wormhole/WormholeReceiverAdapter.sol +++ b/src/adapters/Wormhole/WormholeReceiverAdapter.sol @@ -1,4 +1,4 @@ -// SPDX-License-Identifier: GPL-3.0-only +/// SPDX-License-Identifier: GPL-3.0-only pragma solidity >=0.8.9; /// library imports @@ -36,9 +36,9 @@ contract WormholeReceiverAdapter is IBridgeReceiverAdapter, IWormholeReceiver { /*///////////////////////////////////////////////////////////////// MODIFIER ////////////////////////////////////////////////////////////////*/ - modifier onlyCaller() { - if (!gac.isPrivilegedCaller(msg.sender)) { - revert Error.INVALID_PRIVILEGED_CALLER(); + modifier onlyGlobalOwner() { + if (!gac.isGlobalOwner(msg.sender)) { + revert Error.CALLER_NOT_OWNER(); } _; } @@ -67,7 +67,7 @@ contract WormholeReceiverAdapter is IBridgeReceiverAdapter, IWormholeReceiver { ////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBridgeReceiverAdapter - function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyCaller { + function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyGlobalOwner { uint16 _senderChainDecoded = abi.decode(_senderChain, (uint16)); if (_senderChainDecoded == 0) { @@ -88,7 +88,7 @@ contract WormholeReceiverAdapter is IBridgeReceiverAdapter, IWormholeReceiver { /// @dev maps the MMA chain id to bridge specific chain id /// @dev _origIds is the chain's native chain id /// @dev _whIds are the bridge allocated chain id - function setChainIdMap(uint256[] calldata _origIds, uint16[] calldata _whIds) external onlyCaller { + function setChainIdMap(uint256[] calldata _origIds, uint16[] calldata _whIds) external onlyGlobalOwner { uint256 arrLength = _origIds.length; if (arrLength != _whIds.length) { diff --git a/src/adapters/Wormhole/WormholeSenderAdapter.sol b/src/adapters/Wormhole/WormholeSenderAdapter.sol index 280176f..4433c8f 100644 --- a/src/adapters/Wormhole/WormholeSenderAdapter.sol +++ b/src/adapters/Wormhole/WormholeSenderAdapter.sol @@ -69,7 +69,7 @@ contract WormholeSenderAdapter is BaseSenderAdapter { /// @dev maps the MMA chain id to bridge specific chain id /// @dev _origIds is the chain's native chain id /// @dev _whIds are the bridge allocated chain id - function setChainIdMap(uint256[] calldata _origIds, uint16[] calldata _whIds) external onlyCaller { + function setChainIdMap(uint256[] calldata _origIds, uint16[] calldata _whIds) external onlyGlobalOwner { uint256 arrLength = _origIds.length; if (arrLength != _whIds.length) { diff --git a/src/adapters/axelar/AxelarReceiverAdapter.sol b/src/adapters/axelar/AxelarReceiverAdapter.sol index 57a7589..9e753fe 100644 --- a/src/adapters/axelar/AxelarReceiverAdapter.sol +++ b/src/adapters/axelar/AxelarReceiverAdapter.sol @@ -36,9 +36,9 @@ contract AxelarReceiverAdapter is IAxelarExecutable, IBridgeReceiverAdapter { /*///////////////////////////////////////////////////////////////// MODIFIER ////////////////////////////////////////////////////////////////*/ - modifier onlyCaller() { - if (!gac.isPrivilegedCaller(msg.sender)) { - revert Error.INVALID_PRIVILEGED_CALLER(); + modifier onlyGlobalOwner() { + if (!gac.isGlobalOwner(msg.sender)) { + revert Error.CALLER_NOT_OWNER(); } _; } @@ -56,7 +56,7 @@ contract AxelarReceiverAdapter is IAxelarExecutable, IBridgeReceiverAdapter { ////////////////////////////////////////////////////////////////*/ /// @inheritdoc IBridgeReceiverAdapter - function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyCaller { + function updateSenderAdapter(bytes memory _senderChain, address _senderAdapter) external override onlyGlobalOwner { string memory _senderChainDecoded = abi.decode(_senderChain, (string)); if (keccak256(abi.encode(_senderChainDecoded)) == keccak256(abi.encode(""))) { diff --git a/src/adapters/axelar/AxelarSenderAdapter.sol b/src/adapters/axelar/AxelarSenderAdapter.sol index 222efa1..560d663 100644 --- a/src/adapters/axelar/AxelarSenderAdapter.sol +++ b/src/adapters/axelar/AxelarSenderAdapter.sol @@ -72,7 +72,7 @@ contract AxelarSenderAdapter is BaseSenderAdapter { /// @dev maps the MMA chain id to bridge specific chain id /// @dev _origIds is the chain's native chain id /// @dev _axlIds are the bridge allocated chain id - function setChainIdMap(uint256[] calldata _origIds, string[] calldata _axlIds) external onlyCaller { + function setChainIdMap(uint256[] calldata _origIds, string[] calldata _axlIds) external onlyGlobalOwner { uint256 arrLength = _origIds.length; if (arrLength != _axlIds.length) { diff --git a/src/controllers/GAC.sol b/src/controllers/GAC.sol index e8b3705..8bcfaa3 100644 --- a/src/controllers/GAC.sol +++ b/src/controllers/GAC.sol @@ -11,6 +11,11 @@ import {Error} from "../libraries/Error.sol"; /// @dev is the global access control contract for bridge adapters contract GAC is IGAC, Ownable { + /*/////////////////////////////////////////////////////////////// + CONSTANTS + //////////////////////////////////////////////////////////////*/ + uint256 public constant MINIMUM_DST_GAS_LIMIT = 50000; + /*/////////////////////////////////////////////////////////////// STATE VARIABLES //////////////////////////////////////////////////////////////*/ @@ -76,6 +81,10 @@ contract GAC is IGAC, Ownable { /// @inheritdoc IGAC function setGlobalMsgDeliveryGasLimit(uint256 _gasLimit) external override onlyOwner { + if (_gasLimit < MINIMUM_DST_GAS_LIMIT) { + revert Error.INVALID_DST_GAS_LIMIT_MIN(); + } + uint256 oldLimit = dstGasLimit; dstGasLimit = _gasLimit; @@ -96,7 +105,7 @@ contract GAC is IGAC, Ownable { //////////////////////////////////////////////////////////////*/ /// @inheritdoc IGAC - function isPrivilegedCaller(address _caller) external view override returns (bool) { + function isGlobalOwner(address _caller) external view override returns (bool) { if (_caller == owner()) { return true; } diff --git a/src/interfaces/IGAC.sol b/src/interfaces/IGAC.sol index d8b1b57..dbb785d 100644 --- a/src/interfaces/IGAC.sol +++ b/src/interfaces/IGAC.sol @@ -47,7 +47,7 @@ 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 isPrivilegedCaller(address _caller) external view returns (bool); + function isGlobalOwner(address _caller) external view returns (bool); /// @dev returns the global owner address /// @return _owner is the global owner address diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index 94ba3b6..03bba80 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -165,4 +165,10 @@ library Error { /// @dev is thrown if transaction is expired error TX_EXPIRED(); + + /*///////////////////////////////////////////////////////////////// + GAC ERRORS + ////////////////////////////////////////////////////////////////*/ + /// @dev is thrown if the gas limit is less than minimum + error INVALID_DST_GAS_LIMIT_MIN(); } diff --git a/test/unit-tests/GAC.t.sol b/test/unit-tests/GAC.t.sol new file mode 100644 index 0000000..e0288ae --- /dev/null +++ b/test/unit-tests/GAC.t.sol @@ -0,0 +1,205 @@ +// SPDX-License-Identifier: GPL-3.0-only +pragma solidity >=0.8.9; + +/// library imports +import {Vm} from "forge-std/Test.sol"; + +import "openzeppelin-contracts/contracts/access/Ownable.sol"; + +/// local imports +import "../Setup.t.sol"; +import "../contracts-mock/FailingSenderAdapter.sol"; +import "../contracts-mock/ZeroAddressReceiverGac.sol"; +import "src/interfaces/IBridgeSenderAdapter.sol"; +import "src/interfaces/IMultiMessageReceiver.sol"; +import "src/interfaces/IGAC.sol"; +import "src/libraries/Error.sol"; +import "src/libraries/Message.sol"; +import {MultiMessageSender} from "src/MultiMessageSender.sol"; + +contract GACTest is Setup { + event DstGasLimitUpdated(uint256 oldLimit, uint256 newLimit); + event MultiMessageCallerUpdated(address indexed mmaCaller); + event MultiMessageSenderUpdated(address indexed mmaSender); + event MultiMessageReceiverUpdated(uint256 chainId, address indexed mmaReceiver); + + uint256 constant SRC_CHAIN_ID = 1; + uint256 constant DST_CHAIN_ID = 137; + + address senderAddr; + address receiverAddr; + IGAC gac; + + /// @dev initializes the setup + function setUp() public override { + super.setUp(); + + vm.selectFork(fork[SRC_CHAIN_ID]); + senderAddr = contractAddress[SRC_CHAIN_ID]["MMA_SENDER"]; + receiverAddr = contractAddress[DST_CHAIN_ID]["MMA_RECEIVER"]; + gac = IGAC(contractAddress[SRC_CHAIN_ID]["GAC"]); + } + + /// @dev constructor + function test_constructor() public { + // checks existing setup + assertEq(address(Ownable(address(gac)).owner()), owner); + } + + /// @dev sets multi message sender + function test_set_multi_message_sender() public { + vm.startPrank(owner); + + vm.expectEmit(true, true, true, true, address(gac)); + emit MultiMessageSenderUpdated(address(42)); + + gac.setMultiMessageSender(address(42)); + + assertEq(gac.getMultiMessageSender(), address(42)); + } + + /// @dev only owner can set multi message sender + function test_set_multi_message_sender_only_owner() public { + vm.startPrank(caller); + + vm.expectRevert("Ownable: caller is not the owner"); + gac.setMultiMessageSender(address(42)); + } + + /// @dev cannot set multi message sender to zero address + function test_set_multi_message_sender_zero_address() public { + vm.startPrank(owner); + + vm.expectRevert(Error.ZERO_ADDRESS_INPUT.selector); + gac.setMultiMessageSender(address(0)); + } + + /// @dev sets multi message caller + function test_set_multi_message_caller() public { + vm.startPrank(owner); + + vm.expectEmit(true, true, true, true, address(gac)); + emit MultiMessageCallerUpdated(address(42)); + + gac.setMultiMessageCaller(address(42)); + + assertEq(gac.getMultiMessageCaller(), address(42)); + } + + /// @dev only owner can set multi message caller + function test_set_multi_message_caller_only_owner() public { + vm.startPrank(caller); + + vm.expectRevert("Ownable: caller is not the owner"); + gac.setMultiMessageCaller(address(42)); + } + + /// @dev cannot set multi message caller to zero address + function test_set_multi_message_caller_zero_address() public { + vm.startPrank(owner); + + vm.expectRevert(Error.ZERO_ADDRESS_INPUT.selector); + gac.setMultiMessageCaller(address(0)); + } + + /// @dev sets multi message receiver + function test_set_multi_message_receiver() public { + vm.startPrank(owner); + + vm.expectEmit(true, true, true, true, address(gac)); + emit MultiMessageReceiverUpdated(DST_CHAIN_ID, address(42)); + + gac.setMultiMessageReceiver(DST_CHAIN_ID, address(42)); + + assertEq(gac.getMultiMessageReceiver(DST_CHAIN_ID), address(42)); + } + + /// @dev only owner can set multi message receiver + function test_set_multi_message_receiver_only_owner() public { + vm.startPrank(caller); + + vm.expectRevert("Ownable: caller is not the owner"); + gac.setMultiMessageReceiver(DST_CHAIN_ID, address(42)); + } + + /// @dev cannot set multi message receiver to zero address + function test_set_multi_message_receiver_zero_address() public { + vm.startPrank(owner); + + vm.expectRevert(Error.ZERO_ADDRESS_INPUT.selector); + gac.setMultiMessageReceiver(DST_CHAIN_ID, address(0)); + } + + /// @dev cannot set multi message receiver on zero chain ID + function test_set_multi_message_receiver_zero_chain_id() public { + vm.startPrank(owner); + + vm.expectRevert(Error.ZERO_CHAIN_ID.selector); + gac.setMultiMessageReceiver(0, address(42)); + } + + /// @dev sets global message delivery gas limit + function test_set_global_msg_delivery_gas_limit() public { + vm.startPrank(owner); + + uint256 oldLimit = gac.getGlobalMsgDeliveryGasLimit(); + vm.expectEmit(true, true, true, true, address(gac)); + emit DstGasLimitUpdated(oldLimit, 420000); + + gac.setGlobalMsgDeliveryGasLimit(420000); + + assertEq(gac.getGlobalMsgDeliveryGasLimit(), 420000); + } + + /// @dev only owner can set global message delivery gas limit + function test_set_global_msg_delivery_gas_limit_only_owner() public { + vm.startPrank(caller); + + vm.expectRevert("Ownable: caller is not the owner"); + gac.setGlobalMsgDeliveryGasLimit(420000); + } + + /// @dev cannot set a gas limit lower than the minimum + function test_set_global_msg_delivery_gas_limit_lower_than_min() public { + vm.startPrank(owner); + + vm.expectRevert(Error.INVALID_DST_GAS_LIMIT_MIN.selector); + gac.setGlobalMsgDeliveryGasLimit(30000); + } + + /// @dev sets refund address + function test_set_refund_address() public { + vm.startPrank(owner); + + gac.setRefundAddress(address(42)); + + assertEq(gac.getRefundAddress(), address(42)); + } + + /// @dev only owner can set refund address + function test_set_refund_address_only_owner() public { + vm.startPrank(caller); + + vm.expectRevert("Ownable: caller is not the owner"); + gac.setRefundAddress(address(42)); + } + + /// @dev cannot set refund address to zero + function test_set_refund_address_zero() public { + vm.startPrank(owner); + + vm.expectRevert(Error.ZERO_ADDRESS_INPUT.selector); + gac.setRefundAddress(address(0)); + } + + /// @dev checks if address is the global owner + function test_is_global_owner() public { + assertTrue(gac.isGlobalOwner(owner)); + assertFalse(gac.isGlobalOwner(caller)); + } + + /// @dev gets the global owner + function test_get_global_owner() public { + assertEq(gac.getGlobalOwner(), owner); + } +}