fix: update mirror guard (#16)

# 🤖 Linear Closes SAF-XXX
defi-wonderland · Nov 22, 2023 · 3a0f90e · 3a0f90e
1 parent a644c07
commit 3a0f90e
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 71 deletions.
diff --git a/solidity/contracts/UpdateStorageMirrorGuard.sol b/solidity/contracts/UpdateStorageMirrorGuard.sol
@@ -21,16 +21,6 @@ contract UpdateStorageMirrorGuard is BaseGuard {
 
   IGuardCallbackModule public immutable GUARD_CALLBACK_MODULE;
 
-  /**
-   * @notice A boolean that returns true if a tx is changing the safe's settings
-   */
-  mapping(address => bool) public didSettingsChange;
-
-  /**
-   * @notice The hash of the new settings
-   */
-  mapping(address => bytes32) public settingsHash;
-
   constructor(IGuardCallbackModule _guardCallbackModule) {
     GUARD_CALLBACK_MODULE = _guardCallbackModule;
   }
@@ -52,16 +42,7 @@ contract UpdateStorageMirrorGuard is BaseGuard {
     bytes memory _signatures,
     address _msgSender
   ) external {
-    didSettingsChange[msg.sender] = true;
-    // TODO: change these data with the decoded ones
-    address[] memory _owners = new address[](1);
-    IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1});
-
-    bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));
-    settingsHash[msg.sender] = _settingsHash;
-    didSettingsChange[msg.sender] = true;
-
-    emit SettingsChanged(msg.sender, _settingsHash, _safeSettings);
+    // TODO: This can be improved with the decoding of the data to accurate catch a change of the safe's settings
   }
 
   /**
@@ -70,10 +51,16 @@ contract UpdateStorageMirrorGuard is BaseGuard {
    * @dev The msg.sender should be the safe
    */
   function checkAfterExecution(bytes32 _txHash, bool _success) external {
-    if (didSettingsChange[msg.sender] && _success) {
+    if (_success) {
+      address[] memory _owners = new address[](1);
+      _owners[0] = msg.sender;
+      IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: 1});
+      bytes32 _settingsHash = keccak256(abi.encode(_safeSettings));
+
       // NOTE: No need to reset settings as this function will only be called when the settings change
-      GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, settingsHash[msg.sender]);
-      didSettingsChange[msg.sender] = false;
+      GUARD_CALLBACK_MODULE.saveUpdatedSettings(msg.sender, _settingsHash);
+
+      emit SettingsChanged(msg.sender, _settingsHash, _safeSettings);
     }
   }
 }
diff --git a/solidity/test/unit/UpdateStorageMirrorGuard.t.sol b/solidity/test/unit/UpdateStorageMirrorGuard.t.sol
@@ -2,7 +2,6 @@
 pragma solidity >=0.8.4 <0.9.0;
 
 import {Test} from 'forge-std/Test.sol';
-import {Enum} from 'safe-contracts/common/Enum.sol';
 import {UpdateStorageMirrorGuard} from 'contracts/UpdateStorageMirrorGuard.sol';
 import {IGuardCallbackModule} from 'interfaces/IGuardCallbackModule.sol';
 import {IStorageMirror} from 'interfaces/IStorageMirror.sol';
@@ -15,39 +14,22 @@ 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(safe));
-    assertEq(updateStorageMirrorGuard.settingsHash(safe), bytes32(''));
-
-    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), '', safe
-    );
-
-    assertTrue(updateStorageMirrorGuard.didSettingsChange(safe));
-    assertEq(updateStorageMirrorGuard.settingsHash(safe), settingsHash, 'Settings hash should be stored');
-  }
-
   function testCheckAfterExecution(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), '', safe
-    );
-
     vm.mockCall(
       address(guardCallbackModule),
       abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, settingsHash)),
@@ -56,32 +38,10 @@ contract UnitUpdateStorageMirrorGuard is Base {
     vm.expectCall(
       address(guardCallbackModule), abi.encodeCall(IGuardCallbackModule.saveUpdatedSettings, (safe, settingsHash))
     );
-    vm.prank(safe);
-    updateStorageMirrorGuard.checkAfterExecution(_txHash, true);
-
-    assertFalse(updateStorageMirrorGuard.didSettingsChange(safe));
-  }
 
-  function testCheckAfterExecutionNoSettingsChange(bytes32 _txHash) public {
+    vm.expectEmit(true, true, true, true);
+    emit SettingsChanged(safe, settingsHash, safeSettings);
     vm.prank(safe);
     updateStorageMirrorGuard.checkAfterExecution(_txHash, true);
-
-    assertFalse(updateStorageMirrorGuard.didSettingsChange(safe));
-    assertEq(updateStorageMirrorGuard.settingsHash(safe), bytes32(''), 'Settings hash should stay empty');
-  }
-
-  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), '', 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(safe));
-    assertEq(updateStorageMirrorGuard.settingsHash(safe), settingsHash, 'Settings hash should stay the same');
   }
 }