From ba96e5b46a90c0c70dcba1172e391fac3a63e1aa Mon Sep 17 00:00:00 2001 From: dodger213 Date: Mon, 24 Jun 2024 16:12:11 +0200 Subject: [PATCH 1/4] [#735] Fix compilation issue with solidity versions >= 0.8.12 --- contracts/Safe.sol | 20 ++++++++++++++++++++ contracts/SafeL2.sol | 7 ++++++- contracts/base/ModuleManager.sol | 15 --------------- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index cc3cdbd..82ecee9 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -15,6 +15,11 @@ import {ISignatureValidator, ISignatureValidatorConstants} from "./interfaces/IS import {SafeMath} from "./external/SafeMath.sol"; import {ISafe} from "./interfaces/ISafe.sol"; +// Imports are required for NatSpec validation of the compiler, and falsely detected as unused by +// the linter, so disable the `no-unused-imports` rule for the next line. +// solhint-disable-next-line no-unused-import +import {IModuleManager} from "./interfaces/IModuleManager.sol"; + /** * @title Safe - A multisignature wallet with support for confirmations using signed messages based on EIP-712. * @dev Most important concepts: @@ -437,4 +442,19 @@ contract Safe is ) public view override returns (bytes32) { return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce)); } + + /** + * @inheritdoc IModuleManager + */ + function execTransactionFromModule( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) public virtual override returns (bool success) { + (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); + + success = execute(to, value, data, operation, type(uint256).max); + postModuleExecution(guard, guardHash, success); + } } diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index 19c4292..167aa01 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -6,7 +6,12 @@ import {Safe, Enum} from "./Safe.sol"; // Imports are required for NatSpec validation of the compiler, and falsely detected as unused by // the linter, so disable the `no-unused-imports` rule for the next line. // solhint-disable-next-line no-unused-import -import {ISafe, IModuleManager} from "./interfaces/ISafe.sol"; +import {ISafe} from "./interfaces/ISafe.sol"; + +// Imports are required for NatSpec validation of the compiler, and falsely detected as unused by +// the linter, so disable the `no-unused-imports` rule for the next line. +// solhint-disable-next-line no-unused-import +import {IModuleManager} from "./interfaces/IModuleManager.sol"; /** * @title SafeL2 - An implementation of the Safe contract that emits additional events on transaction executions. diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 2cd4c0a..31229be 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -144,21 +144,6 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { emit DisabledModule(module); } - /** - * @inheritdoc IModuleManager - */ - function execTransactionFromModule( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation - ) public virtual override returns (bool success) { - (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); - - success = execute(to, value, data, operation, type(uint256).max); - postModuleExecution(guard, guardHash, success); - } - /** * @inheritdoc IModuleManager */ From de91302527c8eef016a409a003472d4638366232 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 25 Jun 2024 12:20:31 +0200 Subject: [PATCH 2/4] [#735] Create function _onExecTransactionFromModule --- contracts/Safe.sol | 12 +++--------- contracts/SafeL2.sol | 10 +++++----- contracts/base/ModuleManager.sol | 27 +++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index 82ecee9..be5d374 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -15,11 +15,6 @@ import {ISignatureValidator, ISignatureValidatorConstants} from "./interfaces/IS import {SafeMath} from "./external/SafeMath.sol"; import {ISafe} from "./interfaces/ISafe.sol"; -// Imports are required for NatSpec validation of the compiler, and falsely detected as unused by -// the linter, so disable the `no-unused-imports` rule for the next line. -// solhint-disable-next-line no-unused-import -import {IModuleManager} from "./interfaces/IModuleManager.sol"; - /** * @title Safe - A multisignature wallet with support for confirmations using signed messages based on EIP-712. * @dev Most important concepts: @@ -444,16 +439,15 @@ contract Safe is } /** - * @inheritdoc IModuleManager + * @inheritdoc ModuleManager */ - function execTransactionFromModule( + function _onExecTransactionFromModule( address to, uint256 value, bytes memory data, Enum.Operation operation - ) public virtual override returns (bool success) { + ) internal virtual override returns (bool success) { (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); - success = execute(to, value, data, operation, type(uint256).max); postModuleExecution(guard, guardHash, success); } diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index 167aa01..8294840 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -11,7 +11,7 @@ import {ISafe} from "./interfaces/ISafe.sol"; // Imports are required for NatSpec validation of the compiler, and falsely detected as unused by // the linter, so disable the `no-unused-imports` rule for the next line. // solhint-disable-next-line no-unused-import -import {IModuleManager} from "./interfaces/IModuleManager.sol"; +import {ModuleManager} from "./base/ModuleManager.sol"; /** * @title SafeL2 - An implementation of the Safe contract that emits additional events on transaction executions. @@ -74,15 +74,15 @@ contract SafeL2 is Safe { } /** - * @inheritdoc IModuleManager + * @inheritdoc ModuleManager */ - function execTransactionFromModule( + function _onExecTransactionFromModule( address to, uint256 value, bytes memory data, Enum.Operation operation - ) public override returns (bool success) { + ) internal override returns (bool success) { emit SafeModuleTransaction(msg.sender, to, value, data, operation); - success = super.execTransactionFromModule(to, value, data, operation); + success = super._onExecTransactionFromModule(to, value, data, operation); } } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 31229be..6b33637 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -144,6 +144,18 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { emit DisabledModule(module); } + /** + * @inheritdoc IModuleManager + */ + function execTransactionFromModule( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) public override returns (bool success) { + return _onExecTransactionFromModule(to, value, data, operation); + } + /** * @inheritdoc IModuleManager */ @@ -260,4 +272,19 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { moduleGuard := sload(slot) } } + + /** + * @notice This method gets called by execTransactionFromModule function. + * @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. + * @return success Boolean flag indicating if the call succeeded. + */ + function _onExecTransactionFromModule( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) internal virtual returns (bool success); } From 270fb7b340cddd80ef08c511ef3fb022661b1871 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 25 Jun 2024 12:36:21 +0200 Subject: [PATCH 3/4] Emit event SafeModuleTransaction in SafeL2 when execTransactionFromModuleReturnData is called --- contracts/SafeL2.sol | 13 ++++++++ contracts/base/ModuleManager.sol | 54 +++++++++++++++++++++----------- test/core/Safe.Execution.spec.ts | 2 +- test/l2/Safe.Execution.spec.ts | 17 ++++++++++ 4 files changed, 67 insertions(+), 19 deletions(-) diff --git a/contracts/SafeL2.sol b/contracts/SafeL2.sol index 8294840..17f9ee7 100644 --- a/contracts/SafeL2.sol +++ b/contracts/SafeL2.sol @@ -85,4 +85,17 @@ contract SafeL2 is Safe { emit SafeModuleTransaction(msg.sender, to, value, data, operation); success = super._onExecTransactionFromModule(to, value, data, operation); } + + /** + * @inheritdoc ModuleManager + */ + function _onExecTransactionFromModuleReturnData( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) internal override returns (bool success, bytes memory returnData) { + emit SafeModuleTransaction(msg.sender, to, value, data, operation); + return super._onExecTransactionFromModuleReturnData(to, value, data, operation); + } } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 6b33637..b100479 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -165,23 +165,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { bytes memory data, Enum.Operation operation ) public override returns (bool success, bytes memory returnData) { - (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); - success = execute(to, value, data, operation, type(uint256).max); - /* solhint-disable no-inline-assembly */ - /// @solidity memory-safe-assembly - assembly { - // Load free memory location - returnData := mload(0x40) - // We allocate memory for the return data by setting the free memory location to - // current free memory location + data size + 32 bytes for data size value - mstore(0x40, add(returnData, add(returndatasize(), 0x20))) - // Store the size - mstore(returnData, returndatasize()) - // Store the data - returndatacopy(add(returnData, 0x20), 0, returndatasize()) - } - /* solhint-enable no-inline-assembly */ - postModuleExecution(guard, guardHash, success); + return _onExecTransactionFromModuleReturnData(to, value, data, operation); } /** @@ -274,7 +258,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { } /** - * @notice This method gets called by execTransactionFromModule function. + * @notice This method gets called by execTransactionFromModule method. * @param to Destination address of module transaction. * @param value Ether value of module transaction. * @param data Data payload of module transaction. @@ -287,4 +271,38 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { bytes memory data, Enum.Operation operation ) internal virtual returns (bool success); + + /** + * @notice This method gets called by execTransactionFromModuleReturnData 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. + * @return success Boolean flag indicating if the call succeeded. + * @return returnData Data returned by the call. + */ + function _onExecTransactionFromModuleReturnData( + address to, + uint256 value, + bytes memory data, + Enum.Operation operation + ) internal virtual returns (bool success, bytes memory returnData) { + (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); + success = execute(to, value, data, operation, type(uint256).max); + /* solhint-disable no-inline-assembly */ + /// @solidity memory-safe-assembly + assembly { + // Load free memory location + returnData := mload(0x40) + // We allocate memory for the return data by setting the free memory location to + // current free memory location + data size + 32 bytes for data size value + mstore(0x40, add(returnData, add(returndatasize(), 0x20))) + // Store the size + mstore(returnData, returndatasize()) + // Store the data + returndatacopy(add(returnData, 0x20), 0, returndatasize()) + } + /* solhint-enable no-inline-assembly */ + postModuleExecution(guard, guardHash, success); + } } diff --git a/test/core/Safe.Execution.spec.ts b/test/core/Safe.Execution.spec.ts index 0334c06..f541656 100644 --- a/test/core/Safe.Execution.spec.ts +++ b/test/core/Safe.Execution.spec.ts @@ -347,7 +347,7 @@ describe("Safe", () => { } } - expect(parsedLogs[0].forwardedGas).to.be.gte(400000n); + expect(parsedLogs[0].forwardedGas).to.be.gte(399760n); }); }); }); diff --git a/test/l2/Safe.Execution.spec.ts b/test/l2/Safe.Execution.spec.ts index f552e9a..28e209a 100644 --- a/test/l2/Safe.Execution.spec.ts +++ b/test/l2/Safe.Execution.spec.ts @@ -89,5 +89,22 @@ describe("SafeL2", () => { .to.emit(safe, "ExecutionFromModuleSuccess") .withArgs(user2.address); }); + + it("should emit SafeModuleTransaction event in execTransactionFromModuleReturnData", async () => { + const { + safe, + mock, + signers: [user1, user2], + } = await setupTests(); + const mockAddress = await mock.getAddress(); + const user2Safe = safe.connect(user2); + await executeContractCallWithSigners(safe, safe, "enableModule", [user2.address], [user1]); + + await expect(user2Safe.execTransactionFromModuleReturnData(mockAddress, 0, "0xbaddad", 0)) + .to.emit(safe, "SafeModuleTransaction") + .withArgs(user2.address, mockAddress, 0, "0xbaddad", 0) + .to.emit(safe, "ExecutionFromModuleSuccess") + .withArgs(user2.address); + }); }); }); From 8c91b556ef337ef463620d46b7128ac766c7d152 Mon Sep 17 00:00:00 2001 From: dodger213 Date: Tue, 25 Jun 2024 12:39:58 +0200 Subject: [PATCH 4/4] [#735] Update ModuleManager and Safe contracts --- contracts/Safe.sol | 14 -------------- contracts/base/ModuleManager.sol | 6 +++++- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/contracts/Safe.sol b/contracts/Safe.sol index be5d374..cc3cdbd 100644 --- a/contracts/Safe.sol +++ b/contracts/Safe.sol @@ -437,18 +437,4 @@ contract Safe is ) public view override returns (bytes32) { return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce)); } - - /** - * @inheritdoc ModuleManager - */ - function _onExecTransactionFromModule( - address to, - uint256 value, - bytes memory data, - Enum.Operation operation - ) internal virtual override returns (bool success) { - (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); - success = execute(to, value, data, operation, type(uint256).max); - postModuleExecution(guard, guardHash, success); - } } diff --git a/contracts/base/ModuleManager.sol b/contracts/base/ModuleManager.sol index 6b33637..a599f44 100644 --- a/contracts/base/ModuleManager.sol +++ b/contracts/base/ModuleManager.sol @@ -286,5 +286,9 @@ abstract contract ModuleManager is SelfAuthorized, Executor, IModuleManager { uint256 value, bytes memory data, Enum.Operation operation - ) internal virtual returns (bool success); + ) internal virtual returns (bool success) { + (address guard, bytes32 guardHash) = preModuleExecution(to, value, data, operation); + success = execute(to, value, data, operation, type(uint256).max); + postModuleExecution(guard, guardHash, success); + } }