diff --git a/solidity/contracts/UpdateStorageMirrorGuard.sol b/solidity/contracts/UpdateStorageMirrorGuard.sol index 4b6547a..3b36475 100644 --- 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 index a45db99..f4b5fc2 100644 --- 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'); } }