Skip to content

Commit

Permalink
feat: change fallback validation magic value to zero (#211)
Browse files Browse the repository at this point in the history
  • Loading branch information
Zer0dot authored Oct 3, 2024
1 parent 7d74707 commit 716d960
Show file tree
Hide file tree
Showing 42 changed files with 394 additions and 289 deletions.
Original file line number Diff line number Diff line change
@@ -1 +1 @@
79399
79113
Original file line number Diff line number Diff line change
@@ -1 +1 @@
448696
448410
Original file line number Diff line number Diff line change
@@ -1 +1 @@
55523
55237
Original file line number Diff line number Diff line change
@@ -1 +1 @@
90077
90071
Original file line number Diff line number Diff line change
@@ -1 +1 @@
123520
123514
Original file line number Diff line number Diff line change
@@ -1 +1 @@
187114
186831
Original file line number Diff line number Diff line change
@@ -1 +1 @@
557483
557200
Original file line number Diff line number Diff line change
@@ -1 +1 @@
163250
162967
Original file line number Diff line number Diff line change
@@ -1 +1 @@
210701
210698
Original file line number Diff line number Diff line change
@@ -1 +1 @@
241879
241876
Original file line number Diff line number Diff line change
@@ -1 +1 @@
265949
265676
6 changes: 0 additions & 6 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ jobs:
- name: Run tests
run: FOUNDRY_PROFILE=optimized-test-deep forge test -vvv

- name: Run SMA tests
run: FOUNDRY_PROFILE=optimized-test-deep SMA_TEST=true forge test -vvv

test-default:
name: Run Forge Tests (default)
runs-on: ubuntu-latest
Expand All @@ -42,6 +39,3 @@ jobs:

- name: Run tests
run: forge test -vvv

- name: Run SMA tests
run: SMA_TEST=true forge test -vvv
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@
"lint:gas": "solhint --max-warnings 0 -c ./config/solhint-gas.json './gas/**/*.sol'",
"lint:script": "solhint --max-warnings 0 -c ./config/solhint-script.json './script/**/*.sol'",
"prep": "pnpm fmt && forge b && pnpm lint && pnpm test && pnpm gas",
"test": "forge test && SMA_TEST=true forge test"
"test": "forge test"
}
}
2 changes: 1 addition & 1 deletion src/helpers/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ uint8 constant MAX_PRE_VALIDATION_HOOKS = type(uint8).max;
uint32 constant DIRECT_CALL_VALIDATION_ENTITYID = type(uint32).max;

// Magic value for the ModuleEntity of the fallback validation for SemiModularAccount.
ModuleEntity constant FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(type(uint192).max));
ModuleEntity constant FALLBACK_VALIDATION = ModuleEntity.wrap(bytes24(0));
8 changes: 4 additions & 4 deletions src/modules/permissions/PaymasterGuardModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {BaseModule, IERC165} from "../BaseModule.sol";
/// used.
/// - If this hook is installed, and no paymaster is setup, all requests will revert
contract PaymasterGuardModule is BaseModule, IValidationHookModule {
mapping(uint32 entityId => mapping(address account => address paymaster)) public payamsters;
mapping(uint32 entityId => mapping(address account => address paymaster)) public paymasters;

error BadPaymasterSpecified();

Expand All @@ -23,14 +23,14 @@ contract PaymasterGuardModule is BaseModule, IValidationHookModule {
/// validation
function onInstall(bytes calldata data) external override {
(uint32 entityId, address paymaster) = abi.decode(data, (uint32, address));
payamsters[entityId][msg.sender] = paymaster;
paymasters[entityId][msg.sender] = paymaster;
}

/// @inheritdoc IModule
/// @param data should be encoded with the entityId of the validation
function onUninstall(bytes calldata data) external override {
(uint32 entityId) = abi.decode(data, (uint32));
delete payamsters[entityId][msg.sender];
delete paymasters[entityId][msg.sender];
}

/// @inheritdoc IValidationHookModule
Expand All @@ -41,7 +41,7 @@ contract PaymasterGuardModule is BaseModule, IValidationHookModule {
returns (uint256)
{
address payingPaymaster = address(bytes20(userOp.paymasterAndData[:20]));
if (payingPaymaster == payamsters[entityId][msg.sender]) {
if (payingPaymaster == paymasters[entityId][msg.sender]) {
return 0;
} else {
revert BadPaymasterSpecified();
Expand Down
22 changes: 12 additions & 10 deletions test/account/AccountExecHooks.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ contract AccountExecHooksTest is AccountTestBase {
// emitted by MockModule
event ReceivedCall(bytes msgData, uint256 msgValue);

function setUp() public {
function setUp() public override {
_allowTestDirectCalls();

_m1.executionFunctions.push(
Expand All @@ -39,7 +39,7 @@ contract AccountExecHooksTest is AccountTestBase {
);
}

function test_preExecHook_install() public {
function test_preExecHook_install() public withSMATest {
_installExecution1WithHooks(
ManifestExecutionHook({
executionSelector: _EXEC_SELECTOR,
Expand All @@ -52,7 +52,7 @@ contract AccountExecHooksTest is AccountTestBase {

/// @dev Module 1 hook pair: [1, null]
/// Expected execution: [1, null]
function test_preExecHook_run() public {
function test_preExecHook_run() public withSMATest {
test_preExecHook_install();

vm.expectEmit(true, true, true, true);
Expand All @@ -71,13 +71,13 @@ contract AccountExecHooksTest is AccountTestBase {
assertTrue(success);
}

function test_preExecHook_uninstall() public {
function test_preExecHook_uninstall() public withSMATest {
test_preExecHook_install();

_uninstallExecution(mockModule1);
}

function test_execHookPair_install() public {
function test_execHookPair_install() public withSMATest {
_installExecution1WithHooks(
ManifestExecutionHook({
executionSelector: _EXEC_SELECTOR,
Expand All @@ -90,7 +90,7 @@ contract AccountExecHooksTest is AccountTestBase {

/// @dev Module 1 hook pair: [1, 2]
/// Expected execution: [1, 2]
function test_execHookPair_run() public {
function test_execHookPair_run() public withSMATest {
test_execHookPair_install();

vm.expectEmit(true, true, true, true);
Expand Down Expand Up @@ -119,13 +119,13 @@ contract AccountExecHooksTest is AccountTestBase {
assertTrue(success);
}

function test_execHookPair_uninstall() public {
function test_execHookPair_uninstall() public withSMATest {
test_execHookPair_install();

_uninstallExecution(mockModule1);
}

function test_postOnlyExecHook_install() public {
function test_postOnlyExecHook_install() public withSMATest {
_installExecution1WithHooks(
ManifestExecutionHook({
executionSelector: _EXEC_SELECTOR,
Expand All @@ -138,7 +138,7 @@ contract AccountExecHooksTest is AccountTestBase {

/// @dev Module 1 hook pair: [null, 2]
/// Expected execution: [null, 2]
function test_postOnlyExecHook_run() public {
function test_postOnlyExecHook_run() public withSMATest {
test_postOnlyExecHook_install();

vm.expectEmit(true, true, true, true);
Expand All @@ -151,7 +151,7 @@ contract AccountExecHooksTest is AccountTestBase {
assertTrue(success);
}

function test_postOnlyExecHook_uninstall() public {
function test_postOnlyExecHook_uninstall() public withSMATest {
test_postOnlyExecHook_install();

_uninstallExecution(mockModule1);
Expand All @@ -166,11 +166,13 @@ contract AccountExecHooksTest is AccountTestBase {
vm.expectEmit(true, true, true, true);
emit ExecutionInstalled(address(mockModule1), _m1);

// vm.startPrank(owner1);
account1.installExecution({
module: address(mockModule1),
manifest: mockModule1.executionManifest(),
moduleInstallData: bytes("a")
});
// vm.stopPrank();
}

function _uninstallExecution(MockModule module) internal {
Expand Down
6 changes: 3 additions & 3 deletions test/account/AccountFactory.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,21 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol";
import {TEST_DEFAULT_VALIDATION_ENTITY_ID} from "../utils/TestConstants.sol";

contract AccountFactoryTest is AccountTestBase {
function test_createAccount() public {
function test_createAccount() public withSMATest {
ModularAccount account = factory.createAccount(address(this), 100, TEST_DEFAULT_VALIDATION_ENTITY_ID);

assertEq(address(account.entryPoint()), address(entryPoint));
}

function test_createAccountAndGetAddress() public {
function test_createAccountAndGetAddress() public withSMATest {
ModularAccount account = factory.createAccount(address(this), 100, TEST_DEFAULT_VALIDATION_ENTITY_ID);

assertEq(
address(account), address(factory.createAccount(address(this), 100, TEST_DEFAULT_VALIDATION_ENTITY_ID))
);
}

function test_multipleDeploy() public {
function test_multipleDeploy() public withSMATest {
ModularAccount account = factory.createAccount(address(this), 100, TEST_DEFAULT_VALIDATION_ENTITY_ID);

uint256 startGas = gasleft();
Expand Down
14 changes: 7 additions & 7 deletions test/account/AccountReturnData.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ contract AccountReturnDataTest is AccountTestBase {
ResultCreatorModule public resultCreatorModule;
ResultConsumerModule public resultConsumerModule;

function setUp() public {
function setUp() public override {
_transferOwnershipToTest();

regularResultContract = new RegularResultContract();
Expand Down Expand Up @@ -55,14 +55,14 @@ contract AccountReturnDataTest is AccountTestBase {
}

// Tests the ability to read the result of module execution functions via the account's fallback
function test_returnData_fallback() public view {
function test_returnData_fallback() public withSMATest {
bytes32 result = ResultCreatorModule(address(account1)).foo();

assertEq(result, keccak256("bar"));
}

// Tests the ability to read the results of contracts called via IModularAccount.execute
function test_returnData_singular_execute() public {
function test_returnData_singular_execute() public withSMATest {
bytes memory returnData = account1.executeWithRuntimeValidation(
abi.encodeCall(
account1.execute,
Expand All @@ -77,7 +77,7 @@ contract AccountReturnDataTest is AccountTestBase {
}

// Tests the ability to read the results of multiple contract calls via IModularAccount.executeBatch
function test_returnData_executeBatch() public {
function test_returnData_executeBatch() public withSMATest {
Call[] memory calls = new Call[](2);
calls[0] = Call({
target: address(regularResultContract),
Expand Down Expand Up @@ -105,14 +105,14 @@ contract AccountReturnDataTest is AccountTestBase {
}

// Tests the ability to read data via routing to fallback functions
function test_returnData_execFromModule_fallback() public view {
function test_returnData_execFromModule_fallback() public withSMATest {
bool result = ResultConsumerModule(address(account1)).checkResultFallback(keccak256("bar"));

assertTrue(result);
}

// Tests the ability to read data via executeWithRuntimeValidation
function test_returnData_authorized_exec() public {
// Tests the ability to read data via executeWithAuthorization
function test_returnData_authorized_exec() public withSMATest {
bool result = ResultConsumerModule(address(account1)).checkResultExecuteWithRuntimeValidation(
address(regularResultContract), keccak256("bar")
);
Expand Down
Loading

0 comments on commit 716d960

Please sign in to comment.