From 9340561c79ef482eee9ee4d15c1cac6610b710aa Mon Sep 17 00:00:00 2001 From: dodger213 Date: Wed, 26 Jun 2024 12:21:22 +0200 Subject: [PATCH 1/4] [#775] Create a post call hook onAfterExecTransaction --- contracts/Safe.sol | 23 +++++++++++++++++++++++ contracts/SafeL2.sol | 12 ++++++------ test/core/Safe.Execution.spec.ts | 11 +++++++---- test/l2/Safe.Execution.spec.ts | 2 +- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index cc3cdbd..86db150 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -188,6 +188,7 @@ contract Safe is ITransactionGuard(guard).checkAfterExecution(txHash, success); } } + onAfterExecTransaction(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures, success); } /** @@ -437,4 +438,26 @@ contract Safe is ) public view override returns (bytes32) { return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce)); } + + /** + * @notice A hook that gets called after execution of {execTransaction} method. + * @param to Destination address of module transaction. + * @param value Ether value of module transaction. + * @param data Data payload of module transaction. + * @param operation Operation type of module transaction. + * @param success Boolean flag indicating if the call succeeded. + */ + function onAfterExecTransaction( + address to, + uint256 value, + bytes calldata data, + Enum.Operation operation, + uint256 safeTxGas, + uint256 baseGas, + uint256 gasPrice, + address gasToken, + address payable refundReceiver, + bytes memory signatures, + bool success + ) internal virtual {} } diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index 1dfcc3b..e28a634 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -36,9 +36,9 @@ contract SafeL2 is Safe { event SafeModuleTransaction(address module, address to, uint256 value, bytes data, Enum.Operation operation); /** - * @inheritdoc ISafe + * @inheritdoc Safe */ - function execTransaction( + function onAfterExecTransaction( address to, uint256 value, bytes calldata data, @@ -48,11 +48,12 @@ contract SafeL2 is Safe { uint256 gasPrice, address gasToken, address payable refundReceiver, - bytes memory signatures - ) public payable override returns (bool) { + bytes memory signatures, + bool /*success*/ + ) internal override { bytes memory additionalInfo; { - additionalInfo = abi.encode(nonce, msg.sender, threshold); + additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); } emit SafeMultiSigTransaction( to, @@ -67,7 +68,6 @@ contract SafeL2 is Safe { signatures, additionalInfo ); - return super.execTransaction(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures); } /** diff --git a/test/core/Safe.Execution.spec.ts b/test/core/Safe.Execution.spec.ts index 0334c06..e4304df 100644 --- a/test/core/Safe.Execution.spec.ts +++ b/test/core/Safe.Execution.spec.ts @@ -203,7 +203,7 @@ describe("Safe", () => { await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); const userBalance = await hre.ethers.provider.getBalance(user2.address); - await expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.eq(ethers.parseEther("1")); + expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.eq(ethers.parseEther("1")); let executedTx: any; await expect( @@ -212,9 +212,12 @@ describe("Safe", () => { return tx; }), ).to.emit(safe, "ExecutionSuccess"); + const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!.hash); const receiptLogs = receipt?.logs ?? []; - const logIndex = receiptLogs.length - 1; + + const logIndex = 0; + const successEvent = safe.interface.decodeEventLog( "ExecutionSuccess", receiptLogs[logIndex].data, @@ -223,7 +226,7 @@ describe("Safe", () => { expect(successEvent.txHash).to.be.eq(calculateSafeTransactionHash(safeAddress, tx, await chainId())); // Gas costs are around 3000, so even if we specified a safeTxGas from 100000 we should not use more expect(successEvent.payment).to.be.lte(5000n); - await expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment); + expect(await hre.ethers.provider.getBalance(user2.address)).to.eq(userBalance + successEvent.payment); }); it("should emit payment in failure event", async () => { @@ -255,7 +258,7 @@ describe("Safe", () => { ).to.emit(safe, "ExecutionFailure"); const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!.hash); const receiptLogs = receipt?.logs ?? []; - const logIndex = receiptLogs.length - 1; + const logIndex = 0; const successEvent = safe.interface.decodeEventLog( "ExecutionFailure", receiptLogs[logIndex].data, diff --git a/test/l2/Safe.Execution.spec.ts b/test/l2/Safe.Execution.spec.ts index 28e209a..e042f0b 100644 --- a/test/l2/Safe.Execution.spec.ts +++ b/test/l2/Safe.Execution.spec.ts @@ -46,7 +46,7 @@ describe("SafeL2", () => { }); await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); - await expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.deep.eq(ethers.parseEther("1")); + expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.deep.eq(ethers.parseEther("1")); const additionalInfo = ethers.AbiCoder.defaultAbiCoder().encode( ["uint256", "address", "uint256"], From 62b9f3ae23192676f46df7fb3ab64449af8c153b Mon Sep 17 00:00:00 2001 From: dodger213 Date: Thu, 27 Jun 2024 10:32:53 +0200 Subject: [PATCH 2/4] [#775] Update test should emit SafeMultiSigTransaction event --- test/l2/Safe.Execution.spec.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/l2/Safe.Execution.spec.ts b/test/l2/Safe.Execution.spec.ts index e042f0b..62bfd07 100644 --- a/test/l2/Safe.Execution.spec.ts +++ b/test/l2/Safe.Execution.spec.ts @@ -37,16 +37,19 @@ describe("SafeL2", () => { } = await setupTests(); const safeAddress = await safe.getAddress(); const tx = buildSafeTransaction({ - to: user1.address, + to: safeAddress, nonce: await safe.nonce(), operation: 0, gasPrice: 1, - safeTxGas: 100000, + safeTxGas: 1000000, + // addOwnerWithThreshold is specifically chosen here in this test because additionalInfo in the SafeMultiSigTransaction event has threshold value. + // The test here also checks if the threshold value prior to Safe state change is emitted in the event. + data: safe.interface.encodeFunctionData("addOwnerWithThreshold", [user2.address, 2]), refundReceiver: user2.address, }); await user1.sendTransaction({ to: safeAddress, value: ethers.parseEther("1") }); - expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.deep.eq(ethers.parseEther("1")); + await expect(await hre.ethers.provider.getBalance(safeAddress)).to.be.deep.eq(ethers.parseEther("1")); const additionalInfo = ethers.AbiCoder.defaultAbiCoder().encode( ["uint256", "address", "uint256"], From 90808dc0f7f23693e5fa6d4b4cfd2df7e3f87362 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Thu, 27 Jun 2024 11:29:38 +0200 Subject: [PATCH 3/4] [#775] Refactor: Create onBeforeExecTransaction hook --- contracts/Safe.sol | 6 +++--- contracts/SafeL2.sol | 4 ++-- test/core/Safe.Execution.spec.ts | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 86db150..6cbce9b 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -120,6 +120,7 @@ contract Safe is address payable refundReceiver, bytes memory signatures ) public payable virtual override returns (bool success) { + onBeforeExecTransaction(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures, success); bytes32 txHash; // Use scope here to limit variable lifetime and prevent `stack too deep` errors { @@ -188,7 +189,6 @@ contract Safe is ITransactionGuard(guard).checkAfterExecution(txHash, success); } } - onAfterExecTransaction(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, signatures, success); } /** @@ -440,14 +440,14 @@ contract Safe is } /** - * @notice A hook that gets called after execution of {execTransaction} method. + * @notice A hook that gets called before execution of {execTransaction} method. * @param to Destination address of module transaction. * @param value Ether value of module transaction. * @param data Data payload of module transaction. * @param operation Operation type of module transaction. * @param success Boolean flag indicating if the call succeeded. */ - function onAfterExecTransaction( + function onBeforeExecTransaction( address to, uint256 value, bytes calldata data, diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index e28a634..8ab87aa 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -38,7 +38,7 @@ contract SafeL2 is Safe { /** * @inheritdoc Safe */ - function onAfterExecTransaction( + function onBeforeExecTransaction( address to, uint256 value, bytes calldata data, @@ -53,7 +53,7 @@ contract SafeL2 is Safe { ) internal override { bytes memory additionalInfo; { - additionalInfo = abi.encode(nonce - 1, msg.sender, threshold); + additionalInfo = abi.encode(nonce, msg.sender, threshold); } emit SafeMultiSigTransaction( to, diff --git a/test/core/Safe.Execution.spec.ts b/test/core/Safe.Execution.spec.ts index e4304df..6432254 100644 --- a/test/core/Safe.Execution.spec.ts +++ b/test/core/Safe.Execution.spec.ts @@ -216,7 +216,7 @@ describe("Safe", () => { const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!.hash); const receiptLogs = receipt?.logs ?? []; - const logIndex = 0; + const logIndex = receiptLogs.length - 1; const successEvent = safe.interface.decodeEventLog( "ExecutionSuccess", @@ -258,7 +258,7 @@ describe("Safe", () => { ).to.emit(safe, "ExecutionFailure"); const receipt = await hre.ethers.provider.getTransactionReceipt(executedTx!.hash); const receiptLogs = receipt?.logs ?? []; - const logIndex = 0; + const logIndex = receiptLogs.length - 1; const successEvent = safe.interface.decodeEventLog( "ExecutionFailure", receiptLogs[logIndex].data, From c725dd20d7bbb78c0bcad951167d0d8de41b01f6 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Thu, 27 Jun 2024 11:46:58 +0200 Subject: [PATCH 4/4] [#735] Create pre-hook for execTransactionFromModule* methods --- contracts/SafeL2.sol | 8 +------- contracts/base/ModuleManager.sol | 15 ++++----------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index 8ab87aa..3fb63ea 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -73,13 +73,7 @@ contract SafeL2 is Safe { /** * @inheritdoc ModuleManager */ - function onAfterExecTransactionFromModule( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation, - bool /*success*/ - ) internal override { + function onBeforeExecTransactionFromModule(address to, uint256 value, bytes memory data, Enum.Operation operation) internal override { emit SafeModuleTransaction(msg.sender, to, value, data, operation); } } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 50986ba..56d7058 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -153,10 +153,10 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { bytes memory data, Enum.Operation operation ) public override returns (bool success) { + onBeforeExecTransactionFromModule(to, value, data, operation); (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); success = execute(to, value, data, operation, type(uint256).max); postModuleExecution(guard, guardHash, success); - onAfterExecTransactionFromModule(to, value, data, operation, success); } /** @@ -168,6 +168,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { bytes memory data, Enum.Operation operation ) public override returns (bool success, bytes memory returnData) { + onBeforeExecTransactionFromModule(to, value, data, operation); (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); success = execute(to, value, data, operation, type(uint256).max); /* solhint-disable no-inline-assembly */ @@ -185,7 +186,6 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { } /* solhint-enable no-inline-assembly */ postModuleExecution(guard, guardHash, success); - onAfterExecTransactionFromModule(to, value, data, operation, success); } /** @@ -278,18 +278,11 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { } /** - * @notice A hook that gets called after execution of {execTransactionFromModule*} methods. + * @notice A hook that gets called before execution of {execTransactionFromModule*} methods. * @param to Destination address of module transaction. * @param value Ether value of module transaction. * @param data Data payload of module transaction. * @param operation Operation type of module transaction. - * @param success Boolean flag indicating if the call succeeded. */ - function onAfterExecTransactionFromModule( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation, - bool success - ) internal virtual {} + function onBeforeExecTransactionFromModule(address to, uint256 value, bytes memory data, Enum.Operation operation) internal virtual {} }