From 239830609372ecdb3a6c0a0f996430eb3b6fc7d4 Mon Sep 17 00:00:00 2001 From: DCota <32775237+DaigaroCota@users.noreply.github.com> Date: Tue, 27 Jun 2023 19:35:22 -0700 Subject: [PATCH 1/5] fix(protocol): connext router check gas before trycatch --- packages/protocol/src/routers/ConnextRouter.sol | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/protocol/src/routers/ConnextRouter.sol b/packages/protocol/src/routers/ConnextRouter.sol index 9b04591b5..fa1afd2b0 100644 --- a/packages/protocol/src/routers/ConnextRouter.sol +++ b/packages/protocol/src/routers/ConnextRouter.sol @@ -74,10 +74,11 @@ contract ConnextRouter is BaseRouter, IXReceiver { /// @dev Custom Errors error ConnextRouter__setRouter_invalidInput(); error ConnextRouter__xReceive_notReceivedAssetBalance(); - error ConnextRouter__xReceive_notAllowedCaller(); - error ConnextRouter__xReceiver_noValueTransferUseXbundle(); + error ConnnextRouter__xReceive_notEnoughGasForTryCatch(); error ConnnextRouter__xBundleConnext_notSelfCalled(); + uint256 private constant TRY_CATCH_GAS_ESTIMATE = 740000; + /// @dev The connext contract on the origin domain. IConnext public immutable connext; @@ -163,6 +164,10 @@ contract ConnextRouter is BaseRouter, IXReceiver { (args[0], beforeSlipped) = _accountForSlippage(amount, actions[0], args[0]); } + if (gasleft() < TRY_CATCH_GAS_ESTIMATE) { + revert ConnnextRouter__xReceive_notEnoughGasForTryCatch(); + } + /** * @dev Connext will keep the custody of the bridged amount if the call * to `xReceive` fails. That's why we need to ensure the funds are not stuck at Connext. From ba7089f78f64edba29da9e3c170113dcbc39cd24 Mon Sep 17 00:00:00 2001 From: DCota <32775237+DaigaroCota@users.noreply.github.com> Date: Tue, 27 Jun 2023 19:36:17 -0700 Subject: [PATCH 2/5] chore(protocol): add test for connext router check gas --- .../test/forking/goerli/ConnextRouter.t.sol | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/protocol/test/forking/goerli/ConnextRouter.t.sol b/packages/protocol/test/forking/goerli/ConnextRouter.t.sol index 26f3833df..d4bd6014f 100644 --- a/packages/protocol/test/forking/goerli/ConnextRouter.t.sol +++ b/packages/protocol/test/forking/goerli/ConnextRouter.t.sol @@ -265,6 +265,32 @@ contract ConnextRouterForkingTests is Routines, ForkingSetup { assertEq(IERC20(collateralAsset).balanceOf(address(connextHandler)), amount); } + function testFail_InboundXBundleNotEnoughGas() public { + uint256 amount = 2 ether; + uint256 borrowAmount = 1000e6; + + // make the callData to fail + bytes memory callData = _getDepositAndBorrowCallData( + ALICE, ALICE_PK, amount, borrowAmount, address(0), address(vault) + ); + + // send directly the bridged funds to our router + // thus mocking Connext behavior + deal(collateralAsset, address(connextRouter), amount); + + vm.startPrank(registry[domain].connext); + // call from OPTIMISM_GOERLI where 'originSender' is router that's supposed to have + // the same address as the one on GOERLI + connextRouter.xReceive{gas: 150000}( + "", amount, vault.asset(), address(connextRouter), OPTIMISM_GOERLI_DOMAIN, callData + ); + vm.stopPrank(); + + // No funds were moved + assertEq(vault.balanceOf(ALICE), 0); + assertEq(IERC20(collateralAsset).balanceOf(address(connextHandler)), 0); + } + function test_retryFailedInboundXReceive() public { uint256 amount = 2 ether; uint256 borrowAmount = 1000e6; From 155ec934a6a1cadb61ea4b9d8437c4adac41112e Mon Sep 17 00:00:00 2001 From: DCota <32775237+DaigaroCota@users.noreply.github.com> Date: Wed, 28 Jun 2023 10:41:40 -0700 Subject: [PATCH 3/5] fix(protocol): connext router gas griefing --- packages/protocol/src/routers/ConnextRouter.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/protocol/src/routers/ConnextRouter.sol b/packages/protocol/src/routers/ConnextRouter.sol index 0da9c86b3..9eb599bd2 100644 --- a/packages/protocol/src/routers/ConnextRouter.sol +++ b/packages/protocol/src/routers/ConnextRouter.sol @@ -77,7 +77,7 @@ contract ConnextRouter is BaseRouter, IXReceiver { error ConnnextRouter__xReceive_notEnoughGasForTryCatch(); error ConnnextRouter__xBundleConnext_notSelfCalled(); - uint256 private constant TRY_CATCH_GAS_ESTIMATE = 740000; + uint256 private constant CATCH_EXECUTION_GAS_COST = 645000; /// @dev The connext contract on the origin domain. IConnext public immutable connext; @@ -164,16 +164,13 @@ contract ConnextRouter is BaseRouter, IXReceiver { (args[0], beforeSlipped) = _accountForSlippage(amount, actions[0], args[0]); } - if (gasleft() < TRY_CATCH_GAS_ESTIMATE) { - revert ConnnextRouter__xReceive_notEnoughGasForTryCatch(); - } - /** * @dev Connext will keep the custody of the bridged amount if the call * to `xReceive` fails. That's why we need to ensure the funds are not stuck at Connext. * Therefore we try/catch instead of directly calling _bundleInternal(...). */ - try this.xBundleConnext(actions, args, beforeSlipped) { + try this.xBundleConnext{gas: gasleft() - CATCH_EXECUTION_GAS_COST}(actions, args, beforeSlipped) + { emit XReceived(transferId, originDomain, true, asset, amount, callData); } catch { if (balance > 0) { From 8f3de5e2b8cd4c20f1c7f8d1f37467125a436b2b Mon Sep 17 00:00:00 2001 From: DCota <32775237+DaigaroCota@users.noreply.github.com> Date: Wed, 28 Jun 2023 10:42:01 -0700 Subject: [PATCH 4/5] chore(protocol): update gas griefing testing --- .../test/forking/goerli/ConnextRouter.t.sol | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/protocol/test/forking/goerli/ConnextRouter.t.sol b/packages/protocol/test/forking/goerli/ConnextRouter.t.sol index d4bd6014f..5296f8eab 100644 --- a/packages/protocol/test/forking/goerli/ConnextRouter.t.sol +++ b/packages/protocol/test/forking/goerli/ConnextRouter.t.sol @@ -58,6 +58,12 @@ contract MockTestFlasher is Routines, IFlasher { } } +contract MaliciousReceiver { + receive() external payable { + while (true) {} + } +} + contract ConnextRouterForkingTests is Routines, ForkingSetup { event Deposit(address indexed sender, address indexed owner, uint256 assets, uint256 shares); @@ -265,30 +271,33 @@ contract ConnextRouterForkingTests is Routines, ForkingSetup { assertEq(IERC20(collateralAsset).balanceOf(address(connextHandler)), amount); } - function testFail_InboundXBundleNotEnoughGas() public { - uint256 amount = 2 ether; - uint256 borrowAmount = 1000e6; + function test_gasGriefingInboundXBundle() public { + MaliciousReceiver maliciousReceiver = new MaliciousReceiver(); - // make the callData to fail - bytes memory callData = _getDepositAndBorrowCallData( - ALICE, ALICE_PK, amount, borrowAmount, address(0), address(vault) - ); + uint256 amount = 1 ether; + + // make the callData with malicious receiver + IRouter.Action[] memory actions_ = new IRouter.Action[](1); + actions_[0] = IRouter.Action.WithdrawETH; + + bytes[] memory args_ = new bytes[](1); + args_[0] = abi.encode(amount, address(maliciousReceiver)); + + bytes memory callData = abi.encode(actions_, args_); // send directly the bridged funds to our router // thus mocking Connext behavior deal(collateralAsset, address(connextRouter), amount); vm.startPrank(registry[domain].connext); - // call from OPTIMISM_GOERLI where 'originSender' is router that's supposed to have - // the same address as the one on GOERLI - connextRouter.xReceive{gas: 150000}( + // Limit the amount of gas being passed + connextRouter.xReceive{gas: 750000}( "", amount, vault.asset(), address(connextRouter), OPTIMISM_GOERLI_DOMAIN, callData ); vm.stopPrank(); - // No funds were moved - assertEq(vault.balanceOf(ALICE), 0); - assertEq(IERC20(collateralAsset).balanceOf(address(connextHandler)), 0); + // Funds were moved to ConnextHandler regardles of maliciousReceiver + assertEq(IERC20(collateralAsset).balanceOf(address(connextHandler)), amount); } function test_retryFailedInboundXReceive() public { From 86848e07e80aa3ca0ad4952f1f7964a80664ef0d Mon Sep 17 00:00:00 2001 From: DCota <32775237+DaigaroCota@users.noreply.github.com> Date: Wed, 28 Jun 2023 12:08:24 -0700 Subject: [PATCH 5/5] refactor(protocol): connext router add setter for catch gas cost --- .../protocol/src/routers/ConnextRouter.sol | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/protocol/src/routers/ConnextRouter.sol b/packages/protocol/src/routers/ConnextRouter.sol index 9eb599bd2..c044d572a 100644 --- a/packages/protocol/src/routers/ConnextRouter.sol +++ b/packages/protocol/src/routers/ConnextRouter.sol @@ -31,6 +31,13 @@ contract ConnextRouter is BaseRouter, IXReceiver { */ event NewRouterAdded(address indexed router, uint256 indexed domain); + /** + * @dev Emitted when a new _catchGasCost value is set. + * + * @param newCatchGasCost value + */ + event CatchGasCostChanged(uint256 newCatchGasCost); + /** * @dev Emitted when Connext `xCall` is invoked. * @@ -76,8 +83,9 @@ contract ConnextRouter is BaseRouter, IXReceiver { error ConnextRouter__xReceive_notReceivedAssetBalance(); error ConnnextRouter__xReceive_notEnoughGasForTryCatch(); error ConnnextRouter__xBundleConnext_notSelfCalled(); + error ConnextRouter__setCatchExecutionGasCost_zeroValue(); - uint256 private constant CATCH_EXECUTION_GAS_COST = 645000; + uint256 private _catchGasCost; /// @dev The connext contract on the origin domain. IConnext public immutable connext; @@ -103,6 +111,7 @@ contract ConnextRouter is BaseRouter, IXReceiver { connext = connext_; handler = new ConnextHandler(address(this)); _allowCaller(msg.sender, true); + _catchGasCost = 670000; } /*//////////////////////////////////// @@ -169,8 +178,7 @@ contract ConnextRouter is BaseRouter, IXReceiver { * to `xReceive` fails. That's why we need to ensure the funds are not stuck at Connext. * Therefore we try/catch instead of directly calling _bundleInternal(...). */ - try this.xBundleConnext{gas: gasleft() - CATCH_EXECUTION_GAS_COST}(actions, args, beforeSlipped) - { + try this.xBundleConnext{gas: gasleft() - _catchGasCost}(actions, args, beforeSlipped) { emit XReceived(transferId, originDomain, true, asset, amount, callData); } catch { if (balance > 0) { @@ -467,4 +475,23 @@ contract ConnextRouter is BaseRouter, IXReceiver { emit NewRouterAdded(router, domain); } + + /** + * @notice Sets a new value for `_catchGasCost`. + * + * @param newCatchGasCost value + * + * @dev Requirements: + * - Must be restricted to timelock. + * - Must ensure through testing that `newCatchGasCost` > than most + * gas heavy call in {ConnextHandler-recordFailed(...)}. + */ + function setCatchGasExecutionCost(uint256 newCatchGasCost) external onlyTimelock { + if (newCatchGasCost == 0) { + revert ConnextRouter__setCatchExecutionGasCost_zeroValue(); + } + _catchGasCost = newCatchGasCost; + + emit CatchGasCostChanged(newCatchGasCost); + } }