From c962c7281ccea65dc7494cf8729b43af64e8971a Mon Sep 17 00:00:00 2001 From: excaliborr Date: Tue, 21 Nov 2023 11:16:39 -0500 Subject: [PATCH] fix: update datatype --- solidity/contracts/StorageMirror.sol | 9 +++---- .../contracts/UpdateStorageMirrorGuard.sol | 20 ++++++++------- solidity/interfaces/IStorageMirror.sol | 7 +++--- solidity/test/unit/StorageMirror.t.sol | 13 +++++----- .../test/unit/UpdateStorageMirrorGuard.t.sol | 25 +++++++++---------- 5 files changed, 36 insertions(+), 38 deletions(-) diff --git a/solidity/contracts/StorageMirror.sol b/solidity/contracts/StorageMirror.sol index f34d3ca..ee014ea 100644 --- a/solidity/contracts/StorageMirror.sol +++ b/solidity/contracts/StorageMirror.sol @@ -17,12 +17,11 @@ contract StorageMirror is IStorageMirror { /** * @notice Updates a safe's settings hash * @dev The safe should always be msg.sender - * @param _safeSettings The settings we are going to update to + * @param _hashedSafeSettings The hashed settings we are going to update to */ - function update(SafeSettings memory _safeSettings) external { - bytes32 _settingsHash = keccak256(abi.encode(_safeSettings)); - latestSettingsHash[msg.sender] = _settingsHash; + function update(bytes32 _hashedSafeSettings) external { + latestSettingsHash[msg.sender] = _hashedSafeSettings; - emit SettingsUpdated(msg.sender, _settingsHash, _safeSettings); + emit SettingsUpdated(msg.sender, _hashedSafeSettings); } } diff --git a/solidity/contracts/UpdateStorageMirrorGuard.sol b/solidity/contracts/UpdateStorageMirrorGuard.sol index c232d0a..4b6547a 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 didSettingsChange; /** * @notice The hash of the new settings */ - bytes32 public settingsHash; + mapping(address => bytes32) public settingsHash; constructor(IGuardCallbackModule _guardCallbackModule) { GUARD_CALLBACK_MODULE = _guardCallbackModule; @@ -52,14 +52,16 @@ contract UpdateStorageMirrorGuard is BaseGuard { bytes memory _signatures, address _msgSender ) external { - didSettingsChange = true; + 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}); - settingsHash = keccak256(abi.encode(_safeSettings)); + bytes32 _settingsHash = keccak256(abi.encode(_safeSettings)); + settingsHash[msg.sender] = _settingsHash; + didSettingsChange[msg.sender] = true; - emit SettingsChanged(msg.sender, settingsHash, _safeSettings); + emit SettingsChanged(msg.sender, _settingsHash, _safeSettings); } /** @@ -68,10 +70,10 @@ 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 (didSettingsChange[msg.sender] && _success) { + // 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; } } } diff --git a/solidity/interfaces/IStorageMirror.sol b/solidity/interfaces/IStorageMirror.sol index e75c4c8..c7244b3 100644 --- a/solidity/interfaces/IStorageMirror.sol +++ b/solidity/interfaces/IStorageMirror.sol @@ -11,9 +11,8 @@ interface IStorageMirror { * * @param _safe The address of the safe * @param _settingsHash The hash of the settings - * @param _safeSettings The plaintext of the settings */ - event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash, SafeSettings _safeSettings); + event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash); /*/////////////////////////////////////////////////////////////// STRUCTS @@ -42,7 +41,7 @@ interface IStorageMirror { /** * @notice Updates a safe's settings hash * @dev The safe should always be msg.sender - * @param _safeSettings The settings we are going to update to + * @param _hashedSafeSettings The hashed settings we are going to update to */ - function update(SafeSettings memory _safeSettings) external; + function update(bytes32 _hashedSafeSettings) external; } diff --git a/solidity/test/unit/StorageMirror.t.sol b/solidity/test/unit/StorageMirror.t.sol index ee898be..9464bbc 100644 --- a/solidity/test/unit/StorageMirror.t.sol +++ b/solidity/test/unit/StorageMirror.t.sol @@ -6,9 +6,7 @@ import {StorageMirror} from 'contracts/StorageMirror.sol'; import {IStorageMirror} from 'interfaces/IStorageMirror.sol'; abstract contract Base is Test { - event SettingsUpdated( - address indexed _safe, bytes32 indexed _settingsHash, IStorageMirror.SafeSettings _safeSettings - ); + event SettingsUpdated(address indexed _safe, bytes32 indexed _settingsHash); address public safe; StorageMirror public storageMirror; @@ -27,10 +25,11 @@ contract UnitStorageMirror is Base { IStorageMirror.SafeSettings memory _safeSettings = IStorageMirror.SafeSettings({owners: _owners, threshold: _threshold}); + bytes32 _settingsHash = keccak256(abi.encode(_safeSettings)); + vm.prank(safe); - storageMirror.update(_safeSettings); + storageMirror.update(_settingsHash); - bytes32 _settingsHash = keccak256(abi.encode(_safeSettings)); bytes32 _savedHash = storageMirror.latestSettingsHash(safe); assertEq(_settingsHash, _savedHash, 'Settings hash should be saved'); @@ -47,7 +46,7 @@ contract UnitStorageMirror is Base { vm.prank(safe); vm.expectEmit(true, true, true, true); - emit SettingsUpdated(safe, _expectedSettingsHash, _safeSettings); - storageMirror.update(_safeSettings); + emit SettingsUpdated(safe, _expectedSettingsHash); + storageMirror.update(keccak256(abi.encode(_safeSettings))); } } diff --git a/solidity/test/unit/UpdateStorageMirrorGuard.t.sol b/solidity/test/unit/UpdateStorageMirrorGuard.t.sol index 70cd31e..a45db99 100644 --- a/solidity/test/unit/UpdateStorageMirrorGuard.t.sol +++ b/solidity/test/unit/UpdateStorageMirrorGuard.t.sol @@ -27,25 +27,25 @@ abstract contract Base is Test { contract UnitUpdateStorageMirrorGuard is Base { function testCheckTransaction() public { - assertFalse(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), bytes32('')); + 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), '', 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.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), '', address(0) + address(0), 0, '', Enum.Operation.Call, 0, 0, 0, address(0), payable(0), '', safe ); vm.mockCall( @@ -59,30 +59,29 @@ contract UnitUpdateStorageMirrorGuard is Base { vm.prank(safe); updateStorageMirrorGuard.checkAfterExecution(_txHash, true); - assertFalse(updateStorageMirrorGuard.didSettingsChange()); - assertEq(updateStorageMirrorGuard.settingsHash(), keccak256(abi.encodePacked('')), 'Settings hash should reset'); + assertFalse(updateStorageMirrorGuard.didSettingsChange(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.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), '', 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.didSettingsChange(safe)); + assertEq(updateStorageMirrorGuard.settingsHash(safe), settingsHash, 'Settings hash should stay the same'); } }