From 0575e021b8725c5aaaea64c6a50a4268f6e3e6b4 Mon Sep 17 00:00:00 2001 From: Sujith Somraaj <35634175+sujithsomraaj@users.noreply.github.com> Date: Sun, 8 Oct 2023 21:29:09 -0400 Subject: [PATCH] feat: revert if all message bridges fail (#97) * feat: revert if all message bridges fail * chore: rename error message * chore: add ways for caller to specify successful senders * chore: remove old functions * fix: pr comments * chore: add more tests * chore: add linter --- src/MultiBridgeMessageSender.sol | 42 ++- src/libraries/Error.sol | 3 + test/Setup.t.sol | 1 + .../integration-tests/GracePeriodExpiry.t.sol | 1 + .../MultiMessageAggregation.t.sol | 1 + test/integration-tests/RemoteAdapterAdd.t.sol | 1 + .../RemoteAdapterRemove.t.sol | 1 + test/integration-tests/RemoteSetQuorum.t.sol | 1 + .../RemoteTimelockUpdate.t.sol | 1 + test/integration-tests/TimelockCheck.t.sol | 1 + .../unit-tests/MultiBridgeMessageSender.t.sol | 248 ++++++++++++++++-- 11 files changed, 271 insertions(+), 30 deletions(-) diff --git a/src/MultiBridgeMessageSender.sol b/src/MultiBridgeMessageSender.sol index fd69a83..6a3351c 100644 --- a/src/MultiBridgeMessageSender.sol +++ b/src/MultiBridgeMessageSender.sol @@ -28,6 +28,7 @@ contract MultiBridgeMessageSender { uint256 expiration; address refundAddress; uint256[] fees; + uint256 successThreshold; address[] excludedAdapters; } @@ -145,6 +146,7 @@ contract MultiBridgeMessageSender { /// @param _refundAddress refers to the refund address for any extra native tokens paid /// @param _fees refers to the fees to pay to each sender adapter that is not in the exclusion list specified by _excludedAdapters. /// The fees are in the same order as the sender adapters in the senderAdapters list, after the exclusion list is applied. + /// @param _successThreshold specifies the minimum number of bridges that must successfully dispatch the message for this call to succeed. /// @param _excludedAdapters are the sender adapters to be excluded from relaying the message, in ascending order by address function remoteCall( uint256 _dstChainId, @@ -154,11 +156,20 @@ contract MultiBridgeMessageSender { uint256 _expiration, address _refundAddress, uint256[] calldata _fees, - address[] calldata _excludedAdapters + uint256 _successThreshold, + address[] memory _excludedAdapters ) external payable onlyCaller validateExpiration(_expiration) { _remoteCall( RemoteCallArgs( - _dstChainId, _target, _callData, _nativeValue, _expiration, _refundAddress, _fees, _excludedAdapters + _dstChainId, + _target, + _callData, + _nativeValue, + _expiration, + _refundAddress, + _fees, + _successThreshold, + _excludedAdapters ) ); } @@ -237,7 +248,12 @@ contract MultiBridgeMessageSender { function _remoteCall(RemoteCallArgs memory _args) private { (address mmaReceiver, address[] memory adapters) = _checkAndProcessArgs( - _args.dstChainId, _args.target, _args.refundAddress, _args.fees, _args.excludedAdapters + _args.dstChainId, + _args.target, + _args.refundAddress, + _args.successThreshold, + _args.fees, + _args.excludedAdapters ); /// @dev increments nonce @@ -253,7 +269,12 @@ contract MultiBridgeMessageSender { block.timestamp + _args.expiration ); bytes32 msgId = MessageLibrary.computeMsgId(message); - bool[] memory adapterSuccess = _dispatchMessages(adapters, mmaReceiver, _args.dstChainId, message, _args.fees); + (bool[] memory adapterSuccess, uint256 successCount) = + _dispatchMessages(adapters, mmaReceiver, _args.dstChainId, message, _args.fees); + + if (successCount < _args.successThreshold) { + revert Error.MULTI_MESSAGE_SEND_FAILED(); + } emit MultiBridgeMessageSent( msgId, @@ -277,6 +298,7 @@ contract MultiBridgeMessageSender { uint256 _dstChainId, address _target, address _refundAddress, + uint256 _successThreshold, uint256[] memory _fees, address[] memory _excludedAdapters ) private view returns (address mmaReceiver, address[] memory adapters) { @@ -307,6 +329,11 @@ contract MultiBridgeMessageSender { if (adapters.length == 0) { revert Error.NO_SENDER_ADAPTER_FOUND(); } + + if (_successThreshold > adapters.length) { + revert Error.MULTI_MESSAGE_SEND_FAILED(); + } + if (adapters.length != _fees.length) { revert Error.INVALID_SENDER_ADAPTER_FEES(); } @@ -328,15 +355,18 @@ contract MultiBridgeMessageSender { uint256 _dstChainId, MessageLibrary.Message memory _message, uint256[] memory _fees - ) private returns (bool[] memory) { + ) private returns (bool[] memory, uint256) { uint256 len = _adapters.length; bool[] memory successes = new bool[](len); + uint256 successCount; + for (uint256 i; i < len;) { /// @dev if one bridge is paused, the flow shouldn't be broken try IMessageSenderAdapter(_adapters[i]).dispatchMessage{value: _fees[i]}( _dstChainId, _mmaReceiver, abi.encode(_message) ) { successes[i] = true; + ++successCount; } catch { successes[i] = false; emit MessageSendFailed(_adapters[i], _message); @@ -346,7 +376,7 @@ contract MultiBridgeMessageSender { ++i; } } - return successes; + return (successes, successCount); } function _filterAdapters(address[] memory _existings, address[] memory _removals) diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index 0cdbbdc..dc73ba6 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -106,6 +106,9 @@ library Error { /// @dev is thrown if contract call is invalid (for axelar) error NOT_APPROVED_BY_GATEWAY(); + /// @dev is thrown if a message could not be sent through a sufficient number of bridges + error MULTI_MESSAGE_SEND_FAILED(); + /*///////////////////////////////////////////////////////////////// TIMELOCK ERRORS ////////////////////////////////////////////////////////////////*/ diff --git a/test/Setup.t.sol b/test/Setup.t.sol index 88c7c39..870810b 100644 --- a/test/Setup.t.sol +++ b/test/Setup.t.sol @@ -46,6 +46,7 @@ abstract contract Setup is Test { address constant owner = address(420); address constant refundAddress = address(420420); uint256 constant EXPIRATION_CONSTANT = 5 days; + uint256 constant DEFAULT_SUCCESS_THRESHOLD = 2; /// @dev constants for axelar address constant ETH_GATEWAY = 0x4F4495243837681061C4743b74B3eEdf548D56A5; diff --git a/test/integration-tests/GracePeriodExpiry.t.sol b/test/integration-tests/GracePeriodExpiry.t.sol index c7a3c95..a783ff9 100644 --- a/test/integration-tests/GracePeriodExpiry.t.sol +++ b/test/integration-tests/GracePeriodExpiry.t.sol @@ -48,6 +48,7 @@ contract GracePeriodExpiryTest is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/MultiMessageAggregation.t.sol b/test/integration-tests/MultiMessageAggregation.t.sol index 10c8612..f09b6da 100644 --- a/test/integration-tests/MultiMessageAggregation.t.sol +++ b/test/integration-tests/MultiMessageAggregation.t.sol @@ -48,6 +48,7 @@ contract MultiBridgeMessageAggregationTest is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/RemoteAdapterAdd.t.sol b/test/integration-tests/RemoteAdapterAdd.t.sol index 88296ab..573e16e 100644 --- a/test/integration-tests/RemoteAdapterAdd.t.sol +++ b/test/integration-tests/RemoteAdapterAdd.t.sol @@ -73,6 +73,7 @@ contract RemoteAdapterAdd is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/RemoteAdapterRemove.t.sol b/test/integration-tests/RemoteAdapterRemove.t.sol index e4831ad..d5f2011 100644 --- a/test/integration-tests/RemoteAdapterRemove.t.sol +++ b/test/integration-tests/RemoteAdapterRemove.t.sol @@ -83,6 +83,7 @@ contract RemoteAdapterRemove is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/RemoteSetQuorum.t.sol b/test/integration-tests/RemoteSetQuorum.t.sol index 5cf9817..ac10f04 100644 --- a/test/integration-tests/RemoteSetQuorum.t.sol +++ b/test/integration-tests/RemoteSetQuorum.t.sol @@ -45,6 +45,7 @@ contract RemoteQuorumUpdate is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/RemoteTimelockUpdate.t.sol b/test/integration-tests/RemoteTimelockUpdate.t.sol index 487ba92..4c1bb28 100644 --- a/test/integration-tests/RemoteTimelockUpdate.t.sol +++ b/test/integration-tests/RemoteTimelockUpdate.t.sol @@ -45,6 +45,7 @@ contract RemoteTimelockUpdate is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/integration-tests/TimelockCheck.t.sol b/test/integration-tests/TimelockCheck.t.sol index a3dbe55..1a4891d 100644 --- a/test/integration-tests/TimelockCheck.t.sol +++ b/test/integration-tests/TimelockCheck.t.sol @@ -50,6 +50,7 @@ contract TimelockCheckTest is Setup { EXPIRATION_CONSTANT, refundAddress, fees, + DEFAULT_SUCCESS_THRESHOLD, new address[](0) ); diff --git a/test/unit-tests/MultiBridgeMessageSender.t.sol b/test/unit-tests/MultiBridgeMessageSender.t.sol index b76da8b..5e4fa4e 100644 --- a/test/unit-tests/MultiBridgeMessageSender.t.sol +++ b/test/unit-tests/MultiBridgeMessageSender.t.sol @@ -94,7 +94,15 @@ contract MultiBridgeMessageSenderTest is Setup { ); sender.remoteCall{value: fees[0] + fees[1]}( - DST_CHAIN_ID, address(42), bytes("42"), 0, expiration, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + expiration, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); assertEq(sender.nonce(), 1); @@ -116,7 +124,15 @@ contract MultiBridgeMessageSenderTest is Setup { (, uint256[] memory fees) = _sortTwoAdaptersWithFees(axelarAdapterAddr, wormholeAdapterAddr, 0.01 ether, wormholeFee); sender.remoteCall{value: nativeValue}( - DST_CHAIN_ID, address(42), bytes("42"), 0, expiration, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + expiration, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); uint256 balanceAfter = refundAddress.balance; @@ -136,8 +152,6 @@ contract MultiBridgeMessageSenderTest is Setup { address[] memory excludedAdapters = new address[](1); excludedAdapters[0] = axelarAdapterAddr; - uint256 expiration = EXPIRATION_CONSTANT; - MessageLibrary.Message memory message = MessageLibrary.Message({ srcChainId: SRC_CHAIN_ID, dstChainId: DST_CHAIN_ID, @@ -145,7 +159,7 @@ contract MultiBridgeMessageSenderTest is Setup { nonce: 1, callData: bytes("42"), nativeValue: 0, - expiration: block.timestamp + expiration + expiration: block.timestamp + EXPIRATION_CONSTANT }); bytes32 msgId = MessageLibrary.computeMsgId(message); @@ -157,11 +171,19 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectEmit(true, true, true, true, address(sender)); emit MultiBridgeMessageSent( - msgId, 1, DST_CHAIN_ID, address(42), bytes("42"), 0, expiration, senderAdapters, adapterSuccess + msgId, 1, DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, senderAdapters, adapterSuccess ); sender.remoteCall{value: 0.01 ether}( - DST_CHAIN_ID, address(42), bytes("42"), 0, expiration, refundAddress, fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD - excludedAdapters.length, + excludedAdapters ); } @@ -178,14 +200,30 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(); sender.remoteCall{value: 1 ether}( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, duplicateExclusions + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + duplicateExclusions ); address[] memory nonExistentAdapter = new address[](1); nonExistentAdapter[0] = address(42); vm.expectRevert(); sender.remoteCall{value: 1 ether}( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, duplicateExclusions + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + duplicateExclusions ); } @@ -199,7 +237,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.INVALID_PRIVILEGED_CALLER.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -217,24 +263,56 @@ contract MultiBridgeMessageSenderTest is Setup { vm.startPrank(caller); vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMin, refundAddress, fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + invalidExpMin, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMax, refundAddress, fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + invalidExpMax, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); // test expiration validation in remoteCall() which accepts excluded adapters vm.startPrank(caller); vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMin, refundAddress, fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + invalidExpMin, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); vm.expectRevert(Error.INVALID_EXPIRATION_DURATION.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, invalidExpMax, refundAddress, fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + invalidExpMax, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); } @@ -249,7 +327,15 @@ contract MultiBridgeMessageSenderTest is Setup { fees[1] = 0.01 ether; vm.expectRevert(Error.INVALID_REFUND_ADDRESS.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, address(0), fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + address(0), + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); vm.expectRevert(Error.INVALID_REFUND_ADDRESS.selector); @@ -261,6 +347,7 @@ contract MultiBridgeMessageSenderTest is Setup { EXPIRATION_CONSTANT, contractAddress[SRC_CHAIN_ID]["MMA_SENDER"], fees, + DEFAULT_SUCCESS_THRESHOLD, excludedAdapters ); @@ -268,7 +355,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.startPrank(caller); vm.expectRevert(Error.INVALID_REFUND_ADDRESS.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, address(0), fees, excludedAdapters + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + address(0), + fees, + DEFAULT_SUCCESS_THRESHOLD, + excludedAdapters ); vm.expectRevert(Error.INVALID_REFUND_ADDRESS.selector); @@ -280,6 +375,7 @@ contract MultiBridgeMessageSenderTest is Setup { EXPIRATION_CONSTANT, contractAddress[SRC_CHAIN_ID]["MMA_SENDER"], fees, + DEFAULT_SUCCESS_THRESHOLD, excludedAdapters ); } @@ -294,7 +390,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.INVALID_DST_CHAIN.selector); sender.remoteCall( - block.chainid, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + block.chainid, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -307,7 +411,17 @@ contract MultiBridgeMessageSenderTest is Setup { fees[1] = 0.01 ether; vm.expectRevert(Error.ZERO_CHAIN_ID.selector); - sender.remoteCall(0, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0)); + sender.remoteCall( + 0, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) + ); } /// @dev cannot call with target address of 0 @@ -320,7 +434,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.INVALID_TARGET.selector); sender.remoteCall( - DST_CHAIN_ID, address(0), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(0), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -336,7 +458,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.ZERO_RECEIVER_ADAPTER.selector); dummySender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -357,7 +487,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.NO_SENDER_ADAPTER_FOUND.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -412,7 +550,15 @@ contract MultiBridgeMessageSenderTest is Setup { ); sender.remoteCall{value: 0.1 ether}( - DST_CHAIN_ID, address(42), bytes("42"), 0, expiration, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + expiration, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -425,7 +571,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.INVALID_SENDER_ADAPTER_FEES.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -439,7 +593,15 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.INVALID_MSG_VALUE.selector); sender.remoteCall( - DST_CHAIN_ID, address(42), bytes("42"), 0, EXPIRATION_CONSTANT, refundAddress, fees, new address[](0) + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + fees, + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) ); } @@ -632,4 +794,42 @@ contract MultiBridgeMessageSenderTest is Setup { vm.expectRevert(Error.SENDER_ADAPTER_NOT_EXIST.selector); sender.removeSenderAdapters(adapters); } + + /// @dev if the message could not be sent through a sufficient number of bridge + function test_revert_for_insufficient_number_of_bridges() public { + vm.startPrank(caller); + + vm.expectRevert(Error.MULTI_MESSAGE_SEND_FAILED.selector); + sender.remoteCall( + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + new uint256[](2), + DEFAULT_SUCCESS_THRESHOLD, + new address[](0) + ); + } + + function test_revert_for_invalid_success_threshold() public { + vm.startPrank(caller); + + uint256 nativeValue = 2 ether; + uint256 invalidSuccessThrehsold = 10; + + vm.expectRevert(Error.MULTI_MESSAGE_SEND_FAILED.selector); + sender.remoteCall{value: nativeValue}( + DST_CHAIN_ID, + address(42), + bytes("42"), + 0, + EXPIRATION_CONSTANT, + refundAddress, + new uint256[](2), + invalidSuccessThrehsold, + new address[](0) + ); + } }