From b212c9916a08a73c2cf76b2ac4d0fee442160aee Mon Sep 17 00:00:00 2001 From: jj1980a Date: Mon, 18 Nov 2024 12:31:18 +0400 Subject: [PATCH 1/7] set gas refund beneficiary as metacall parameter --- src/contracts/atlas/Atlas.sol | 8 ++++++-- src/contracts/atlas/GasAccounting.sol | 10 ++++++++-- .../fastlane-online/FastLaneOnlineOuter.sol | 2 +- .../GeneralizedBackrunUserBundler.sol | 2 +- src/contracts/helpers/Simulator.sol | 2 +- src/contracts/interfaces/IAtlas.sol | 3 ++- test/Accounting.t.sol | 4 ++-- test/Escrow.t.sol | 10 +++++----- test/ExPost.t.sol | 2 +- test/FlashLoan.t.sol | 6 +++--- test/GasAccounting.t.sol | 2 +- test/MainTest.t.sol | 2 +- test/OEV.t.sol | 4 ++-- test/OEValt.t.sol | 2 +- test/SwapIntent.t.sol | 19 +++++++------------ test/SwapIntentInvertBid.t.sol | 2 +- test/TrebleSwap.t.sol | 2 +- test/V2RewardDAppControl.t.sol | 2 +- 18 files changed, 45 insertions(+), 39 deletions(-) diff --git a/src/contracts/atlas/Atlas.sol b/src/contracts/atlas/Atlas.sol index 5635d17ab..694d3fe3f 100644 --- a/src/contracts/atlas/Atlas.sol +++ b/src/contracts/atlas/Atlas.sol @@ -54,11 +54,13 @@ contract Atlas is Escrow, Factory { /// @param userOp The UserOperation struct containing the user's transaction data. /// @param solverOps The SolverOperation array containing the solvers' transaction data. /// @param dAppOp The DAppOperation struct containing the DApp's transaction data. + /// @param gasRefundBeneficiary The address to receive the gas refund. /// @return auctionWon A boolean indicating whether there was a successful, winning solver. function metacall( UserOperation calldata userOp, // set by user SolverOperation[] calldata solverOps, // supplied by ops relay - DAppOperation calldata dAppOp // supplied by front end via atlas SDK + DAppOperation calldata dAppOp, // supplied by front end via atlas SDK + address gasRefundBeneficiary // address(0) = msg.sender ) external payable @@ -97,7 +99,9 @@ contract Atlas is Escrow, Factory { try this.execute(_dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, _isSimulation) returns (Context memory ctx) { // Gas Refund to sender only if execution is successful - (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = _settle(ctx, _dConfig.solverGasLimit); + (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = _settle( + ctx, _dConfig.solverGasLimit, gasRefundBeneficiary != address(0) ? gasRefundBeneficiary : msg.sender + ); auctionWon = ctx.solverSuccessful; emit MetacallResult( diff --git a/src/contracts/atlas/GasAccounting.sol b/src/contracts/atlas/GasAccounting.sol index 43e0c7ece..5a642b243 100644 --- a/src/contracts/atlas/GasAccounting.sol +++ b/src/contracts/atlas/GasAccounting.sol @@ -414,11 +414,13 @@ abstract contract GasAccounting is SafetyLocks { /// refund for gas spent, and Atlas' gas surcharge is updated. /// @param ctx Context struct containing relevant context information for the Atlas auction. /// @param solverGasLimit The dApp's maximum gas limit for a solver, as set in the DAppConfig. + /// @param gasRefundBeneficiary The address to receive the gas refund. /// @return claimsPaidToBundler The amount of ETH paid to the bundler in this function. /// @return netAtlasGasSurcharge The net gas surcharge of the metacall, taken by Atlas. function _settle( Context memory ctx, - uint256 solverGasLimit + uint256 solverGasLimit, + address gasRefundBeneficiary ) internal returns (uint256 claimsPaidToBundler, uint256 netAtlasGasSurcharge) @@ -474,13 +476,17 @@ abstract contract GasAccounting is SafetyLocks { } claimsPaidToBundler -= _currentDeficit; } else { + if (_winningSolver == ctx.bundler) { + _winningSolver = gasRefundBeneficiary; + } + _credit(_winningSolver, _amountSolverReceives - _amountSolverPays, _adjustedClaims); } // Set lock to FullyLocked to prevent any reentrancy possibility _setLockPhase(uint8(ExecutionPhase.FullyLocked)); - if (claimsPaidToBundler != 0) SafeTransferLib.safeTransferETH(ctx.bundler, claimsPaidToBundler); + if (claimsPaidToBundler != 0) SafeTransferLib.safeTransferETH(gasRefundBeneficiary, claimsPaidToBundler); return (claimsPaidToBundler, netAtlasGasSurcharge); } diff --git a/src/contracts/examples/fastlane-online/FastLaneOnlineOuter.sol b/src/contracts/examples/fastlane-online/FastLaneOnlineOuter.sol index fb4f10f65..77a0e41d9 100644 --- a/src/contracts/examples/fastlane-online/FastLaneOnlineOuter.sol +++ b/src/contracts/examples/fastlane-online/FastLaneOnlineOuter.sol @@ -52,7 +52,7 @@ contract FastLaneOnlineOuter is SolverGateway { // Atlas call (bool _success,) = ATLAS.call{ value: msg.value, gas: _metacallGasLimit(_gasReserved, userOp.gas, gasleft()) }( - abi.encodeCall(IAtlas.metacall, (userOp, _solverOps, _dAppOp)) + abi.encodeCall(IAtlas.metacall, (userOp, _solverOps, _dAppOp, address(0))) ); // Revert if the metacall failed - neither solvers nor baseline call fulfilled swap intent diff --git a/src/contracts/examples/generalized-backrun/GeneralizedBackrunUserBundler.sol b/src/contracts/examples/generalized-backrun/GeneralizedBackrunUserBundler.sol index 643fe0bea..ffbc5a058 100644 --- a/src/contracts/examples/generalized-backrun/GeneralizedBackrunUserBundler.sol +++ b/src/contracts/examples/generalized-backrun/GeneralizedBackrunUserBundler.sol @@ -182,7 +182,7 @@ contract GeneralizedBackrunUserBundler is DAppControl { SolverOperation[] memory _solverOps = _getSolverOps(solverOpHashes); (bool _success, bytes memory _data) = - ATLAS.call{ value: msg.value }(abi.encodeCall(IAtlas.metacall, (userOp, _solverOps, _dAppOp))); + ATLAS.call{ value: msg.value }(abi.encodeCall(IAtlas.metacall, (userOp, _solverOps, _dAppOp, address(0)))); if (!_success) { assembly { revert(add(_data, 32), mload(_data)) diff --git a/src/contracts/helpers/Simulator.sol b/src/contracts/helpers/Simulator.sol index 5ac2653d7..f08d59854 100644 --- a/src/contracts/helpers/Simulator.sol +++ b/src/contracts/helpers/Simulator.sol @@ -144,7 +144,7 @@ contract Simulator is AtlasErrors { payable { if (msg.sender != address(this)) revert InvalidEntryFunction(); - if (!IAtlas(atlas).metacall{ value: msg.value }(userOp, solverOps, dAppOp)) { + if (!IAtlas(atlas).metacall{ value: msg.value }(userOp, solverOps, dAppOp, address(0))) { revert NoAuctionWinner(); // should be unreachable } revert SimulationPassed(); diff --git a/src/contracts/interfaces/IAtlas.sol b/src/contracts/interfaces/IAtlas.sol index d66ce2adb..578da2b36 100644 --- a/src/contracts/interfaces/IAtlas.sol +++ b/src/contracts/interfaces/IAtlas.sol @@ -11,7 +11,8 @@ interface IAtlas { function metacall( UserOperation calldata userOp, SolverOperation[] calldata solverOps, - DAppOperation calldata dAppOp + DAppOperation calldata dAppOp, + address gasRefundBeneficiary ) external payable diff --git a/test/Accounting.t.sol b/test/Accounting.t.sol index 1006d6047..c87ac15ac 100644 --- a/test/Accounting.t.sol +++ b/test/Accounting.t.sol @@ -63,7 +63,7 @@ contract AccountingTest is BaseTest { SolverOperation[] memory solverOps = _setupBorrowRepayTestUsingBasicSwapIntent(address(honestSolver)); vm.startPrank(userEOA); - atlas.metacall{ value: 0 }({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall{ value: 0 }({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); // console.log("\nAFTER METACALL"); @@ -90,7 +90,7 @@ contract AccountingTest is BaseTest { SolverOperation[] memory solverOps = _setupBorrowRepayTestUsingBasicSwapIntent(address(evilSolver)); vm.startPrank(userEOA); - atlas.metacall{ value: 0 }({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall{ value: 0 }({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); } diff --git a/test/Escrow.t.sol b/test/Escrow.t.sol index 2eb667754..12822743a 100644 --- a/test/Escrow.t.sol +++ b/test/Escrow.t.sol @@ -210,7 +210,7 @@ contract EscrowTest is BaseTest { DAppOperation memory dappOp = validDAppOperation(userOp, solverOps).build(); vm.prank(userEOA); - bool auctionWon = atlas.metacall(userOp, solverOps, dappOp); + bool auctionWon = atlas.metacall(userOp, solverOps, dappOp, address(0)); assertLe(dAppControl.userOpGasLeft(), userGasLim, "userOpGasLeft should be <= userGasLim"); assertTrue(auctionWon, "2nd auction should have been won"); @@ -239,7 +239,7 @@ contract EscrowTest is BaseTest { // Send msg.value so it must be sent back, testing the upper bound of remaining gas for graceful return deal(userEOA, 1 ether); vm.prank(userEOA); - bool auctionWon = atlas.metacall{gas: metacallGasLim, value: 1 ether}(userOp, solverOps, dappOp); + bool auctionWon = atlas.metacall{gas: metacallGasLim, value: 1 ether}(userOp, solverOps, dappOp, address(0)); assertEq(auctionWon, false, "call should not revert but auction should not be won either"); } @@ -332,7 +332,7 @@ contract EscrowTest is BaseTest { } vm.prank(userEOA); - bool auctionWon = atlas.metacall(userOp, solverOps, dappOp); + bool auctionWon = atlas.metacall(userOp, solverOps, dappOp, address(0)); if (!revertExpected) { assertTrue(auctionWon, "auction should have been won"); @@ -558,7 +558,7 @@ contract EscrowTest is BaseTest { DAppOperation memory dappOp = validDAppOperation(userOp, solverOps).build(); vm.prank(userEOA); - (bool success,) = address(atlas).call(abi.encodeCall(atlas.metacall, (userOp, solverOps, dappOp))); + (bool success,) = address(atlas).call(abi.encodeCall(atlas.metacall, (userOp, solverOps, dappOp, address(0)))); assertTrue(success, "metacall should have succeeded"); } @@ -670,7 +670,7 @@ contract EscrowTest is BaseTest { vm.prank(userEOA); if (metacallShouldRevert) vm.expectRevert(); // Metacall should revert, the reason isn't important, we're only checking the event - atlas.metacall(userOp, solverOps, dappOp); + atlas.metacall(userOp, solverOps, dappOp, address(0)); } } diff --git a/test/ExPost.t.sol b/test/ExPost.t.sol index 78f5add99..da7226e94 100644 --- a/test/ExPost.t.sol +++ b/test/ExPost.t.sol @@ -189,7 +189,7 @@ contract ExPostTest is BaseTest { // uint256 solverTwoAtlEthBalance = atlas.balanceOf(solverTwoEOA); (bool success,) = - address(atlas).call(abi.encodeCall(atlas.metacall, (userOp, solverOps, dAppOp))); + address(atlas).call(abi.encodeCall(atlas.metacall, (userOp, solverOps, dAppOp, address(0)))); if (success) { console.log("success!"); diff --git a/test/FlashLoan.t.sol b/test/FlashLoan.t.sol index bc7716521..8b7007898 100644 --- a/test/FlashLoan.t.sol +++ b/test/FlashLoan.t.sol @@ -129,7 +129,7 @@ contract FlashLoanTest is BaseTest { result ); vm.expectRevert(); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); // now try it again with a valid solverOp - but dont fully pay back @@ -181,7 +181,7 @@ contract FlashLoanTest is BaseTest { result ); vm.expectRevert(); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); // final try, should be successful with full payback @@ -246,7 +246,7 @@ contract FlashLoanTest is BaseTest { true, result ); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); // atlas 2e beginning bal + 1e from solver +100e eth from user = 103e atlas total diff --git a/test/GasAccounting.t.sol b/test/GasAccounting.t.sol index 430471dcf..f6179be8e 100644 --- a/test/GasAccounting.t.sol +++ b/test/GasAccounting.t.sol @@ -64,7 +64,7 @@ contract MockGasAccounting is TestAtlas, BaseTest { } function settle(Context memory ctx) external returns (uint256, uint256) { - return _settle(ctx, MOCK_SOLVER_GAS_LIMIT); + return _settle(ctx, MOCK_SOLVER_GAS_LIMIT, msg.sender); } function adjustAccountingForFees(Context memory ctx) diff --git a/test/MainTest.t.sol b/test/MainTest.t.sol index 788b8ab9b..1bc0a4339 100644 --- a/test/MainTest.t.sol +++ b/test/MainTest.t.sol @@ -404,7 +404,7 @@ contract MainTest is BaseTest { vm.startPrank(userEOA); IERC20(TOKEN_ONE).approve(address(atlas), type(uint256).max); - atlas.metacall(userOp, solverOps, dAppOp); + atlas.metacall(userOp, solverOps, dAppOp, address(0)); vm.stopPrank(); // Execution environment should exist now diff --git a/test/OEV.t.sol b/test/OEV.t.sol index 267d966e4..4cca75db4 100644 --- a/test/OEV.t.sol +++ b/test/OEV.t.sol @@ -165,7 +165,7 @@ contract OEVTest is BaseTest { // Should Fail vm.prank(userEOA); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); assertEq(uint(chainlinkAtlasWrapper.latestAnswer()), uint(AggregatorV2V3Interface(chainlinkETHUSD).latestAnswer()), "Metacall unexpectedly succeeded"); @@ -176,7 +176,7 @@ contract OEVTest is BaseTest { // Should Succeed vm.prank(userEOA); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); console.log("Metacall Gas Cost:", gasLeftBefore - gasleft()); diff --git a/test/OEValt.t.sol b/test/OEValt.t.sol index a836e5c1d..ebe295e28 100644 --- a/test/OEValt.t.sol +++ b/test/OEValt.t.sol @@ -164,7 +164,7 @@ contract OEVTest is BaseTest { // Should Succeed vm.prank(transmitter); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); assertEq(uint(chainlinkAtlasWrapper.latestAnswer()), targetOracleAnswer, "Wrapper did not update as expected"); assertTrue(uint(chainlinkAtlasWrapper.latestAnswer()) != uint(IChainlinkFeed(chainlinkETHUSD).latestAnswer()), "Wrapper and base feed should report different answers"); diff --git a/test/SwapIntent.t.sol b/test/SwapIntent.t.sol index 5ae6b8ce2..6072a62c3 100644 --- a/test/SwapIntent.t.sol +++ b/test/SwapIntent.t.sol @@ -60,14 +60,10 @@ contract SwapIntentTest is BaseTest { UserCondition userCondition = new UserCondition(); Condition[] memory conditions = new Condition[](2); - conditions[0] = Condition({ - antecedent: address(userCondition), - context: abi.encodeCall(UserCondition.isLessThanFive, 3) - }); - conditions[1] = Condition({ - antecedent: address(userCondition), - context: abi.encodeCall(UserCondition.isLessThanFive, 4) - }); + conditions[0] = + Condition({ antecedent: address(userCondition), context: abi.encodeCall(UserCondition.isLessThanFive, 3) }); + conditions[1] = + Condition({ antecedent: address(userCondition), context: abi.encodeCall(UserCondition.isLessThanFive, 4) }); SwapIntent memory swapIntent = SwapIntent({ tokenUserBuys: DAI_ADDRESS, @@ -124,8 +120,7 @@ contract SwapIntentTest is BaseTest { // userOp.signature = abi.encodePacked(sig.r, sig.s, sig.v); // Build solver calldata (function selector on solver contract and its params) - bytes memory solverOpData = - abi.encodeCall(SimpleRFQSolver.fulfillRFQ, (swapIntent, executionEnvironment)); + bytes memory solverOpData = abi.encodeCall(SimpleRFQSolver.fulfillRFQ, (swapIntent, executionEnvironment)); // Builds the SolverOperation solverOps[0] = txBuilder.buildSolverOperation({ @@ -176,7 +171,7 @@ contract SwapIntentTest is BaseTest { uint256 gasLeftBefore = gasleft(); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); console.log("Metacall Gas Cost:", gasLeftBefore - gasleft()); vm.stopPrank(); @@ -304,7 +299,7 @@ contract SwapIntentTest is BaseTest { assertEq(DAI.balanceOf(address(uniswapSolver)), 0, "Solver has DAI before metacall"); // NOTE: Should metacall return something? Feels like a lot of data you might want to know about the tx - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); vm.stopPrank(); console.log("\nAFTER METACALL"); diff --git a/test/SwapIntentInvertBid.t.sol b/test/SwapIntentInvertBid.t.sol index 4af3f86fd..652724e70 100644 --- a/test/SwapIntentInvertBid.t.sol +++ b/test/SwapIntentInvertBid.t.sol @@ -245,7 +245,7 @@ contract SwapIntentTest is BaseTest { uint256 gasLeftBefore = gasleft(); vm.startPrank(userEOA); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); console.log("Metacall Gas Cost:", gasLeftBefore - gasleft()); vm.stopPrank(); diff --git a/test/TrebleSwap.t.sol b/test/TrebleSwap.t.sol index e118e588b..88005b7a9 100644 --- a/test/TrebleSwap.t.sol +++ b/test/TrebleSwap.t.sol @@ -295,7 +295,7 @@ contract TrebleSwapTest is BaseTest { // Do the actual metacall vm.prank(userEOA); - bool auctionWon = atlas.metacall{ value: msgValue }(args.userOp, args.solverOps, args.dAppOp); + bool auctionWon = atlas.metacall{ value: msgValue }(args.userOp, args.solverOps, args.dAppOp, address(0)); // Estimate gas surcharge Atlas should have taken txGasUsed = estAtlasGasSurcharge - gasleft(); diff --git a/test/V2RewardDAppControl.t.sol b/test/V2RewardDAppControl.t.sol index d20e697ef..c4300ce96 100644 --- a/test/V2RewardDAppControl.t.sol +++ b/test/V2RewardDAppControl.t.sol @@ -132,7 +132,7 @@ contract V2RewardDAppControlTest is BaseTest { console.log("User DAI balance", DAI.balanceOf(userEOA)); vm.prank(governanceEOA); - atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp }); + atlas.metacall({ userOp: userOp, solverOps: solverOps, dAppOp: dAppOp, gasRefundBeneficiary: address(0) }); console.log("\nAFTER METACALL"); console.log("User WETH balance", WETH.balanceOf(userEOA)); From da38ccb026845edba9fa2e554b93b7273ca924aa Mon Sep 17 00:00:00 2001 From: jj1980a Date: Mon, 18 Nov 2024 15:27:57 +0400 Subject: [PATCH 2/7] remove isSimulator local variable to be under stack too deep limit --- src/contracts/atlas/Atlas.sol | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/contracts/atlas/Atlas.sol b/src/contracts/atlas/Atlas.sol index 694d3fe3f..60c177af7 100644 --- a/src/contracts/atlas/Atlas.sol +++ b/src/contracts/atlas/Atlas.sol @@ -70,15 +70,14 @@ contract Atlas is Escrow, Factory { ? gasleft() + _BASE_TRANSACTION_GAS_USED + (msg.data.length * _CALLDATA_LENGTH_PREMIUM) : gasleft() + IL2GasCalculator(L2_GAS_CALCULATOR).initialGasUsed(msg.data.length); - bool _isSimulation = msg.sender == SIMULATOR; - address _bundler = _isSimulation ? dAppOp.bundler : msg.sender; - + address _bundler = msg.sender == SIMULATOR ? dAppOp.bundler : msg.sender; (address _executionEnvironment, DAppConfig memory _dConfig) = _getOrCreateExecutionEnvironment(userOp); - ValidCallsResult _validCallsResult = - VERIFICATION.validateCalls(_dConfig, userOp, solverOps, dAppOp, msg.value, _bundler, _isSimulation); + ValidCallsResult _validCallsResult = VERIFICATION.validateCalls( + _dConfig, userOp, solverOps, dAppOp, msg.value, _bundler, msg.sender == SIMULATOR + ); if (_validCallsResult != ValidCallsResult.Valid) { - if (_isSimulation) revert VerificationSimFail(_validCallsResult); + if (msg.sender == SIMULATOR) revert VerificationSimFail(_validCallsResult); // Gracefully return for results that need nonces to be stored and prevent replay attacks if (uint8(_validCallsResult) >= _GRACEFUL_RETURN_THRESHOLD && !_dConfig.callConfig.allowsReuseUserOps()) { @@ -96,8 +95,9 @@ contract Atlas is Escrow, Factory { // userOpHash has already been calculated and verified in validateCalls at this point, so rather // than re-calculate it, we can simply take it from the dAppOp here. It's worth noting that this will // be either a TRUSTED or DEFAULT hash, depending on the allowsTrustedOpHash setting. - try this.execute(_dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, _isSimulation) - returns (Context memory ctx) { + try this.execute( + _dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, msg.sender == SIMULATOR + ) returns (Context memory ctx) { // Gas Refund to sender only if execution is successful (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = _settle( ctx, _dConfig.solverGasLimit, gasRefundBeneficiary != address(0) ? gasRefundBeneficiary : msg.sender From 01d62fa9d61425e0394a811ebd735ffb21624d4b Mon Sep 17 00:00:00 2001 From: jj1980a Date: Mon, 18 Nov 2024 16:02:13 +0400 Subject: [PATCH 3/7] fix unit test --- test/GasAccounting.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/GasAccounting.t.sol b/test/GasAccounting.t.sol index f6179be8e..9b9a7198b 100644 --- a/test/GasAccounting.t.sol +++ b/test/GasAccounting.t.sol @@ -64,7 +64,7 @@ contract MockGasAccounting is TestAtlas, BaseTest { } function settle(Context memory ctx) external returns (uint256, uint256) { - return _settle(ctx, MOCK_SOLVER_GAS_LIMIT, msg.sender); + return _settle(ctx, MOCK_SOLVER_GAS_LIMIT, makeAddr("bundler")); } function adjustAccountingForFees(Context memory ctx) From 63250c32af1a59cb5726a809893d46c99b59b9b9 Mon Sep 17 00:00:00 2001 From: jj1980a Date: Mon, 18 Nov 2024 16:29:10 +0400 Subject: [PATCH 4/7] add gas refund beneficiary unit tests --- test/MainTest.t.sol | 89 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/test/MainTest.t.sol b/test/MainTest.t.sol index 1bc0a4339..aa38629da 100644 --- a/test/MainTest.t.sol +++ b/test/MainTest.t.sol @@ -494,4 +494,93 @@ contract MainTest is BaseTest { assertFalse(abi.decode(data, (bool)), "Failure case did not return false"); vm.stopPrank(); } + + function testgasRefundBeneficiarySolverSucceeds() public { + uint8 v; + bytes32 r; + bytes32 s; + + address beneficiary = makeAddr("beneficiary"); + + vm.prank(solverOneEOA); + atlas.bond(1 ether); + + UserOperation memory userOp = helper.buildUserOperation(POOL_ONE, POOL_TWO, userEOA, TOKEN_ONE); + // User does not sign their own operation when bundling + + SolverOperation[] memory solverOps = new SolverOperation[](1); + bytes memory solverOpData = helper.buildV2SolverOperationData(POOL_TWO, POOL_ONE); + solverOps[0] = helper.buildSolverOperation(userOp, solverOpData, solverOneEOA, address(solverOne), 2e17, 0); + (v, r, s) = vm.sign(solverOnePK, atlasVerification.getSolverPayload(solverOps[0])); + solverOps[0].signature = abi.encodePacked(r, s, v); + + DAppOperation memory dAppOp = helper.buildDAppOperation(governanceEOA, userOp, solverOps); + (v, r, s) = vm.sign(governancePK, atlasVerification.getDAppOperationPayload(dAppOp)); + dAppOp.signature = abi.encodePacked(r, s, v); + + uint256 bondedBalanceBefore = atlas.balanceOfBonded(beneficiary); + + console.log("Beneficiary's balance before metacall", beneficiary.balance); + console.log("Beneficiary's AtlETH bonded balance before metacall", bondedBalanceBefore); + + assertEq(beneficiary.balance, 0, "Beneficiary should start with 0 balance"); + assertEq(bondedBalanceBefore, 0, "Beneficiary's AtlETH bonded balance should start with 0"); + + vm.startPrank(userEOA); + IERC20(TOKEN_ONE).approve(address(atlas), type(uint256).max); + atlas.metacall(userOp, solverOps, dAppOp, beneficiary); + vm.stopPrank(); + + uint256 bondedBalanceAfter = atlas.balanceOfBonded(beneficiary); + + console.log("Beneficiary's balance after metacall", beneficiary.balance); + console.log("Beneficiary's AtlETH bonded balance after metacall", bondedBalanceAfter); + + assertGt(beneficiary.balance, 0, "Beneficiary should have received some ETH"); + assertEq(bondedBalanceAfter, 0, "Beneficiary's AtlETH bonded balance should still be 0"); + } + + function testgasRefundBeneficiarySolverFails() public { + uint8 v; + bytes32 r; + bytes32 s; + + address beneficiary = makeAddr("beneficiary"); + + vm.prank(solverOneEOA); + atlas.bond(1 ether); + + UserOperation memory userOp = helper.buildUserOperation(POOL_ONE, POOL_TWO, userEOA, TOKEN_ONE); + // User does not sign their own operation when bundling + + SolverOperation[] memory solverOps = new SolverOperation[](1); + bytes memory solverOpData = helper.buildV2SolverOperationData(POOL_ONE, POOL_ONE); // will fail + solverOps[0] = helper.buildSolverOperation(userOp, solverOpData, solverOneEOA, address(solverOne), 2e17, 0); + (v, r, s) = vm.sign(solverOnePK, atlasVerification.getSolverPayload(solverOps[0])); + solverOps[0].signature = abi.encodePacked(r, s, v); + + DAppOperation memory dAppOp = helper.buildDAppOperation(governanceEOA, userOp, solverOps); + (v, r, s) = vm.sign(governancePK, atlasVerification.getDAppOperationPayload(dAppOp)); + dAppOp.signature = abi.encodePacked(r, s, v); + + uint256 bondedBalanceBefore = atlas.balanceOfBonded(beneficiary); + + console.log("Beneficiary's balance before metacall", beneficiary.balance); + console.log("Beneficiary's AtlETH bonded balance before metacall", bondedBalanceBefore); + + assertEq(beneficiary.balance, 0, "Beneficiary should start with 0 balance"); + assertEq(bondedBalanceBefore, 0, "Beneficiary's AtlETH bonded balance should start with 0"); + + vm.startPrank(userEOA); + IERC20(TOKEN_ONE).approve(address(atlas), type(uint256).max); + atlas.metacall(userOp, solverOps, dAppOp, beneficiary); + vm.stopPrank(); + + uint256 bondedBalanceAfter = atlas.balanceOfBonded(beneficiary); + console.log("Beneficiary's balance after metacall", beneficiary.balance); + console.log("Beneficiary's AtlETH bonded balance after metacall", bondedBalanceAfter); + + assertEq(beneficiary.balance, 0, "Beneficiary should still have 0 balance"); + assertGt(bondedBalanceAfter, 0, "Beneficiary's AtlETH bonded balance should be greater than 0"); + } } From 67388e76b80831e39f681db9b40f7bc25a6d6f98 Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Mon, 18 Nov 2024 17:34:01 +0200 Subject: [PATCH 5/7] chore: make test names camelCase --- test/MainTest.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/MainTest.t.sol b/test/MainTest.t.sol index aa38629da..d628ce615 100644 --- a/test/MainTest.t.sol +++ b/test/MainTest.t.sol @@ -412,7 +412,7 @@ contract MainTest is BaseTest { assertTrue(exists, "ExecutionEnvironment wasn't created"); } - function testTestUserOperation_SkipCoverage() public { + function testUserOperation_SkipCoverage() public { uint8 v; bytes32 r; bytes32 s; @@ -495,7 +495,7 @@ contract MainTest is BaseTest { vm.stopPrank(); } - function testgasRefundBeneficiarySolverSucceeds() public { + function testGasRefundBeneficiarySolverSucceeds() public { uint8 v; bytes32 r; bytes32 s; @@ -540,7 +540,7 @@ contract MainTest is BaseTest { assertEq(bondedBalanceAfter, 0, "Beneficiary's AtlETH bonded balance should still be 0"); } - function testgasRefundBeneficiarySolverFails() public { + function testGasRefundBeneficiarySolverFails() public { uint8 v; bytes32 r; bytes32 s; From fcb02f3314ba3085f1d3efbf606b988d32613435 Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Mon, 18 Nov 2024 17:41:51 +0200 Subject: [PATCH 6/7] refactor: resolve stack too deep, add back `_isSim` var --- src/contracts/atlas/Atlas.sol | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/contracts/atlas/Atlas.sol b/src/contracts/atlas/Atlas.sol index ecbc35eeb..2fe2b340e 100644 --- a/src/contracts/atlas/Atlas.sol +++ b/src/contracts/atlas/Atlas.sol @@ -70,22 +70,25 @@ contract Atlas is Escrow, Factory { ? gasleft() + _BASE_TRANSACTION_GAS_USED + (msg.data.length * _CALLDATA_LENGTH_PREMIUM) : gasleft() + IL2GasCalculator(L2_GAS_CALCULATOR).initialGasUsed(msg.data.length); - address _bundler = msg.sender == SIMULATOR ? dAppOp.bundler : msg.sender; + bool _isSimulation = msg.sender == SIMULATOR; + address _bundler = _isSimulation ? dAppOp.bundler : msg.sender; (address _executionEnvironment, DAppConfig memory _dConfig) = _getOrCreateExecutionEnvironment(userOp); - ValidCallsResult _validCallsResult = VERIFICATION.validateCalls( - _dConfig, userOp, solverOps, dAppOp, msg.value, _bundler, msg.sender == SIMULATOR - ); - if (_validCallsResult != ValidCallsResult.Valid) { - if (msg.sender == SIMULATOR) revert VerificationSimFail(_validCallsResult); + { + ValidCallsResult _validCallsResult = + VERIFICATION.validateCalls(_dConfig, userOp, solverOps, dAppOp, msg.value, _bundler, _isSimulation); + if (_validCallsResult != ValidCallsResult.Valid) { + if (_isSimulation) revert VerificationSimFail(_validCallsResult); - // Gracefully return for results that need nonces to be stored and prevent replay attacks - if (uint8(_validCallsResult) >= _GRACEFUL_RETURN_THRESHOLD && !_dConfig.callConfig.allowsReuseUserOps()) { - return false; - } + // Gracefully return for results that need nonces to be stored and prevent replay attacks + if (uint8(_validCallsResult) >= _GRACEFUL_RETURN_THRESHOLD && !_dConfig.callConfig.allowsReuseUserOps()) + { + return false; + } - // Revert for all other results - revert ValidCalls(_validCallsResult); + // Revert for all other results + revert ValidCalls(_validCallsResult); + } } // Initialize the environment lock and accounting values @@ -95,9 +98,8 @@ contract Atlas is Escrow, Factory { // userOpHash has already been calculated and verified in validateCalls at this point, so rather // than re-calculate it, we can simply take it from the dAppOp here. It's worth noting that this will // be either a TRUSTED or DEFAULT hash, depending on the allowsTrustedOpHash setting. - try this.execute( - _dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, msg.sender == SIMULATOR - ) returns (Context memory ctx) { + try this.execute(_dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, _isSimulation) + returns (Context memory ctx) { // Gas Refund to sender only if execution is successful (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = _settle( ctx, _dConfig.solverGasLimit, gasRefundBeneficiary != address(0) ? gasRefundBeneficiary : msg.sender From fe2fb3d44e49083c2c19516ba42d0f038172491f Mon Sep 17 00:00:00 2001 From: Ben Sparks <52714090+BenSparksCode@users.noreply.github.com> Date: Mon, 18 Nov 2024 18:08:20 +0200 Subject: [PATCH 7/7] refactor: simplify gas beneficiary logic --- src/contracts/atlas/Atlas.sol | 5 ++--- src/contracts/atlas/GasAccounting.sol | 9 ++++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/contracts/atlas/Atlas.sol b/src/contracts/atlas/Atlas.sol index 2fe2b340e..efc246db3 100644 --- a/src/contracts/atlas/Atlas.sol +++ b/src/contracts/atlas/Atlas.sol @@ -101,9 +101,8 @@ contract Atlas is Escrow, Factory { try this.execute(_dConfig, userOp, solverOps, _executionEnvironment, _bundler, dAppOp.userOpHash, _isSimulation) returns (Context memory ctx) { // Gas Refund to sender only if execution is successful - (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = _settle( - ctx, _dConfig.solverGasLimit, gasRefundBeneficiary != address(0) ? gasRefundBeneficiary : msg.sender - ); + (uint256 _ethPaidToBundler, uint256 _netGasSurcharge) = + _settle(ctx, _dConfig.solverGasLimit, gasRefundBeneficiary); auctionWon = ctx.solverSuccessful; emit MetacallResult( diff --git a/src/contracts/atlas/GasAccounting.sol b/src/contracts/atlas/GasAccounting.sol index 6d2c70b6e..3e8562a40 100644 --- a/src/contracts/atlas/GasAccounting.sol +++ b/src/contracts/atlas/GasAccounting.sol @@ -428,6 +428,8 @@ abstract contract GasAccounting is SafetyLocks { // If a solver won, their address is still in the _solverLock (address _winningSolver,,) = _solverLockData(); + if (gasRefundBeneficiary == address(0)) gasRefundBeneficiary = ctx.bundler; + // Load what we can from storage so that it shows up in the gasleft() calc uint256 _claims; @@ -457,8 +459,9 @@ abstract contract GasAccounting is SafetyLocks { } else if (_winningSolver == ctx.bundler) { claimsPaidToBundler = 0; } else { + // this else block is only executed if there is no successful solver claimsPaidToBundler = 0; - _winningSolver = ctx.bundler; + _winningSolver = gasRefundBeneficiary; } if (_amountSolverPays > _amountSolverReceives) { @@ -473,10 +476,6 @@ abstract contract GasAccounting is SafetyLocks { } claimsPaidToBundler -= _currentDeficit; } else { - if (_winningSolver == ctx.bundler) { - _winningSolver = gasRefundBeneficiary; - } - _credit(_winningSolver, _amountSolverReceives - _amountSolverPays, _adjustedClaims); }