diff --git a/solidity/contracts/GuardCallbackModule.sol b/solidity/contracts/GuardCallbackModule.sol index d84d532..4ec7bcb 100644 --- a/solidity/contracts/GuardCallbackModule.sol +++ b/solidity/contracts/GuardCallbackModule.sol @@ -33,11 +33,11 @@ contract GuardCallbackModule is IGuardCallbackModule { * @notice Saves the updated settings for the safe to the StorageMirror. * @dev Executes a transaction from the module to update the safe settings in the StorageMirror. * @param _safe The address of the safe. - * @param _settingsHash The hash of the new settings for the safe. + * @param _safeSettings The settings of the safe */ - function saveUpdatedSettings(address _safe, bytes32 _settingsHash) external { + function saveUpdatedSettings(address _safe, IStorageMirror.SafeSettings memory _safeSettings) external { if (msg.sender != GUARD) revert OnlyGuard(); - bytes memory _txData = abi.encodeWithSelector(IStorageMirror.update.selector, _settingsHash); + bytes memory _txData = abi.encodeWithSelector(IStorageMirror.update.selector, _safeSettings); ISafe(_safe).execTransactionFromModule(STORAGE_MIRROR, 0, _txData, Enum.Operation.Call); } } diff --git a/solidity/contracts/UpdateStorageMirrorGuard.sol b/solidity/contracts/UpdateStorageMirrorGuard.sol index c232d0a..304ea8c 100644 --- a/solidity/contracts/UpdateStorageMirrorGuard.sol +++ b/solidity/contracts/UpdateStorageMirrorGuard.sol @@ -24,12 +24,12 @@ contract UpdateStorageMirrorGuard is BaseGuard { /** * @notice A boolean that returns true if a tx is changing the safe's settings */ - bool public didSettingsChange; + mapping(address => bool) public didSettingsChangeForSafe; /** - * @notice The hash of the new settings + * @notice The settings of the safe */ - bytes32 public settingsHash; + mapping(address => IStorageMirror.SafeSettings) public safeSettings; constructor(IGuardCallbackModule _guardCallbackModule) { GUARD_CALLBACK_MODULE = _guardCallbackModule; @@ -52,14 +52,16 @@ contract UpdateStorageMirrorGuard is BaseGuard { bytes memory _signatures, address _msgSender ) external { - didSettingsChange = true; + didSettingsChangeForSafe[_msgSender] = true; // TODO: change these data with the decoded ones address[] memory _owners = new address[](1); + _owners[0] = _msgSender; IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1}); + safeSettings[_msgSender] = _safeSettings; - settingsHash = keccak256(abi.encode(_safeSettings)); + bytes32 _settingsHash = keccak256(abi.encode(_safeSettings)); - emit SettingsChanged(msg.sender, settingsHash, _safeSettings); + emit SettingsChanged(msg.sender, _settingsHash, _safeSettings); } /** @@ -68,10 +70,9 @@ contract UpdateStorageMirrorGuard is BaseGuard { * @dev The msg.sender should be the safe */ function checkAfterExecution(bytes32 _txHash, bool _success) external { - if (didSettingsChange && _success) { - GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, settingsHash); - didSettingsChange = false; - settingsHash = keccak256(abi.encodePacked('')); + if (didSettingsChangeForSafe[msg.sender] && _success) { + GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, safeSettings[msg.sender]); + didSettingsChangeForSafe[msg.sender] = false; } } } diff --git a/solidity/interfaces/IGuardCallbackModule.sol b/solidity/interfaces/IGuardCallbackModule.sol index 235bb11..823524e 100644 --- a/solidity/interfaces/IGuardCallbackModule.sol +++ b/solidity/interfaces/IGuardCallbackModule.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity =0.8.19; +import {IStorageMirror} from 'interfaces/IStorageMirror.sol'; + interface IGuardCallbackModule { /*/////////////////////////////////////////////////////////////// ERRORS @@ -33,9 +35,9 @@ interface IGuardCallbackModule { * @notice Saves the updated settings for the safe to the StorageMirror. * @dev Executes a transaction from the module to update the safe settings in the StorageMirror. * @param _safe The address of the safe. - * @param _settingsHash The hash of the new settings for the safe. + * @param _safeSettings The settings of the safe. */ - function saveUpdatedSettings(address _safe, bytes32 _settingsHash) external; + function saveUpdatedSettings(address _safe, IStorageMirror.SafeSettings memory _safeSettings) external; /** * @notice Initates the module by setting the guard. diff --git a/solidity/test/unit/GuardCallbackModule.t.sol b/solidity/test/unit/GuardCallbackModule.t.sol index 220e1d7..68c500f 100644 --- a/solidity/test/unit/GuardCallbackModule.t.sol +++ b/solidity/test/unit/GuardCallbackModule.t.sol @@ -53,20 +53,19 @@ contract UnitGuardCallbackModuel is Base { _storageMirror, 0, abi.encodeWithSelector( - IStorageMirror.update.selector, - keccak256(abi.encode(IStorageMirror.SafeSettings({owners: _owners, threshold: 1}))) + IStorageMirror.update.selector, IStorageMirror.SafeSettings({owners: _owners, threshold: 1}) ), Enum.Operation.Call ); vm.prank(_guard); vm.expectCall(address(_fakeSafe), _txData); _guardCallbackModule.saveUpdatedSettings( - address(_fakeSafe), keccak256(abi.encode(IStorageMirror.SafeSettings({owners: _owners, threshold: 1}))) + address(_fakeSafe), IStorageMirror.SafeSettings({owners: _owners, threshold: 1}) ); } - function testSaveUpdatedSettingsRevertsIfNotCalledFromGuard(bytes32 _fakeData) public { + function testSaveUpdatedSettingsRevertsIfNotCalledFromGuard(IStorageMirror.SafeSettings memory _safeSettings) public { vm.expectRevert(IGuardCallbackModule.OnlyGuard.selector); - _guardCallbackModule.saveUpdatedSettings(address(_fakeSafe), _fakeData); + _guardCallbackModule.saveUpdatedSettings(address(_fakeSafe), _safeSettings); } } diff --git a/solidity/test/unit/UpdateStorageMirrorGuard.t.sol b/solidity/test/unit/UpdateStorageMirrorGuard.t.sol index 70cd31e..2630151 100644 --- a/solidity/test/unit/UpdateStorageMirrorGuard.t.sol +++ b/solidity/test/unit/UpdateStorageMirrorGuard.t.sol @@ -15,74 +15,73 @@ abstract contract Base is Test { UpdateStorageMirrorGuard public updateStorageMirrorGuard; address[] public owners = new address[](1); - IStorageMirror.SafeSettings public safeSettings = IStorageMirror.SafeSettings({owners: owners, threshold: 1}); - bytes32 public settingsHash = keccak256(abi.encode(safeSettings)); + IStorageMirror.SafeSettings public safeSettings; + bytes32 public settingsHash; function setUp() public { safe = makeAddr('safe'); guardCallbackModule = IGuardCallbackModule(makeAddr('guardCallbackModule')); updateStorageMirrorGuard = new UpdateStorageMirrorGuard(guardCallbackModule); + + owners[0] = safe; + safeSettings = IStorageMirror.SafeSettings({owners: owners, threshold: 1}); + settingsHash = keccak256(abi.encode(safeSettings)); } } contract UnitUpdateStorageMirrorGuard is Base { function testCheckTransaction() public { - assertFalse(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), bytes32('')); + assertFalse(updateStorageMirrorGuard.didSettingsChangeForSafe(safe)); vm.expectEmit(true, true, true, true); emit SettingsChanged(safe, settingsHash, safeSettings); vm.prank(safe); updateStorageMirrorGuard.checkTransaction( - address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0) + address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe ); - assertTrue(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), settingsHash, 'Settings hash should be stored'); + assertTrue(updateStorageMirrorGuard.didSettingsChangeForSafe(safe)); } function testCheckAfterExecution(bytes32 _txHash) public { - // Call checkTransaction to change didSettingsChange to true + // Call checkTransaction to change didSettingsChangeForSafe to true vm.prank(safe); updateStorageMirrorGuard.checkTransaction( - address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0) + address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe ); vm.mockCall( address(guardCallbackModule), - abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, settingsHash)), + abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, safeSettings)), abi.encode() ); vm.expectCall( - address(guardCallbackModule), abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, settingsHash)) + address(guardCallbackModule), abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, safeSettings)) ); vm.prank(safe); updateStorageMirrorGuard.checkAfterExecution(_txHash, true); - assertFalse(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), keccak256(abi.encodePacked('')), 'Settings hash should reset'); + assertFalse(updateStorageMirrorGuard.didSettingsChangeForSafe(safe)); } function testCheckAfterExecutionNoSettingsChange(bytes32 _txHash) public { vm.prank(safe); updateStorageMirrorGuard.checkAfterExecution(_txHash, true); - assertFalse(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), bytes32(''), 'Settings hash should stay empty'); + assertFalse(updateStorageMirrorGuard.didSettingsChangeForSafe(safe)); } function testCheckAfterExecutionTxFailed(bytes32 _txHash) public { // Call checkTransaction to change didSettingsChange to true vm.prank(safe); updateStorageMirrorGuard.checkTransaction( - address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', address(0) + address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe ); vm.prank(safe); updateStorageMirrorGuard.checkAfterExecution(_txHash, false); // Should be true since the tx failed to execute and thus didnt make it to reset - assertTrue(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), settingsHash, 'Settings hash should stay the same'); + assertTrue(updateStorageMirrorGuard.didSettingsChangeForSafe(safe)); } }