Skip to content

Commit

Permalink
fix: guard and module function types
Browse files Browse the repository at this point in the history
  • Loading branch information
0xOneTony committed Nov 21, 2023
1 parent 955bf40 commit a42a466
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 38 deletions.
6 changes: 3 additions & 3 deletions solidity/contracts/GuardCallbackModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
21 changes: 11 additions & 10 deletions solidity/contracts/UpdateStorageMirrorGuard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}

/**
Expand All @@ -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;
}
}
}
6 changes: 4 additions & 2 deletions solidity/interfaces/IGuardCallbackModule.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity =0.8.19;

import {IStorageMirror} from 'interfaces/IStorageMirror.sol';

interface IGuardCallbackModule {
/*///////////////////////////////////////////////////////////////
ERRORS
Expand Down Expand Up @@ -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.
Expand Down
9 changes: 4 additions & 5 deletions solidity/test/unit/GuardCallbackModule.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
35 changes: 17 additions & 18 deletions solidity/test/unit/UpdateStorageMirrorGuard.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

0 comments on commit a42a466

Please sign in to comment.