Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: guard and module function arguments #13

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions solidity/contracts/GuardCallbackModule.sol
Original file line number Diff line number Diff line change
@@ -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
@@ -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;
}
}
}
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
@@ -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.
9 changes: 4 additions & 5 deletions solidity/test/unit/GuardCallbackModule.t.sol
Original file line number Diff line number Diff line change
@@ -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
@@ -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));
}
}