Skip to content

Commit

Permalink
Only allow installing direct call validation via manifest
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamegyed
adamegyed committed Jul 18, 2024
1 parent 220497a commit 8a1fe73
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 134 deletions.
43 changes: 20 additions & 23 deletions src/account/PluginManagerInternals.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165C

import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} from "../helpers/Constants.sol";
import {KnownSelectors} from "../helpers/KnownSelectors.sol";
import {PluginEntityLib} from "../helpers/PluginEntityLib.sol";
import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {IPlugin, ManifestExecutionHook, ManifestValidation, PluginManifest} from "../interfaces/IPlugin.sol";
import {IPlugin, ManifestExecutionHook, PluginManifest} from "../interfaces/IPlugin.sol";
import {IPluginManager, PluginEntity, ValidationConfig} from "../interfaces/IPluginManager.sol";
import {AccountStorage, SelectorData, ValidationData, getAccountStorage, toSetValue} from "./AccountStorage.sol";

Expand All @@ -18,12 +19,6 @@ abstract contract PluginManagerInternals is IPluginManager {
using PluginEntityLib for PluginEntity;
using ValidationConfigLib for ValidationConfig;

// Index marking the start of the data for the validation function.
uint8 internal constant _RESERVED_VALIDATION_DATA_INDEX = 255;

// Magic value for the Entity ID of direct call validation.
uint32 internal constant _SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max;

error ArrayLengthMismatch();
error Erc4337FunctionNotAllowed(bytes4 selector);
error ExecutionFunctionAlreadySet(bytes4 selector);
Expand Down Expand Up @@ -163,16 +158,19 @@ abstract contract PluginManagerInternals is IPluginManager {
_setExecutionFunction(selector, isPublic, allowGlobalValidation, plugin);
}

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
// Todo: limit this to only "direct runtime call" validation path (old EFP),
// and add a way for the user to specify permission/pre-val hooks here.
ManifestValidation memory mv = manifest.validationFunctions[i];
// Install direct call validation, if any, from the manifest

ValidationConfig validationConfig =
ValidationConfigLib.pack(plugin, mv.entityId, mv.isGlobal, mv.isSignatureValidation);
_addValidationFunction(validationConfig, mv.selectors);
}
// Todo: add a way for the user to specify permission/pre-val hooks here.

ValidationConfig directCallValidation = ValidationConfigLib.pack({
_plugin: plugin,
_entityId: SELF_PERMIT_VALIDATION_FUNCTIONID,
_isGlobal: manifest.globalDirectCallValidation,
// Direct call validation is never a signature validation
_isSignatureValidation: false
});

_addValidationFunction(directCallValidation, manifest.directCallValidationSelectors);

length = manifest.executionHooks.length;
for (uint256 i = 0; i < length; ++i) {
Expand Down Expand Up @@ -212,11 +210,10 @@ abstract contract PluginManagerInternals is IPluginManager {
_removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook);
}

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
PluginEntity validationFunction =
PluginEntityLib.pack(plugin, manifest.validationFunctions[i].entityId);
_removeValidationFunction(validationFunction);
if (manifest.globalDirectCallValidation || manifest.directCallValidationSelectors.length > 0) {
PluginEntity directCallValidation = PluginEntityLib.pack(plugin, SELF_PERMIT_VALIDATION_FUNCTIONID);

_removeValidationFunction(directCallValidation);
}

length = manifest.executionFunctions.length;
Expand Down Expand Up @@ -267,7 +264,7 @@ abstract contract PluginManagerInternals is IPluginManager {
}

// Avoid collision between reserved index and actual indices
if (_validationData.preValidationHooks.length > _RESERVED_VALIDATION_DATA_INDEX) {
if (_validationData.preValidationHooks.length > RESERVED_VALIDATION_DATA_INDEX) {
revert PreValidationHookLimitExceeded();
}
}
Expand Down Expand Up @@ -297,7 +294,7 @@ abstract contract PluginManagerInternals is IPluginManager {
}
}

if (validationConfig.entityId() != _SELF_PERMIT_VALIDATION_FUNCTIONID) {
if (validationConfig.entityId() != SELF_PERMIT_VALIDATION_FUNCTIONID) {
// Only allow global validations and signature validations if they're not direct-call validations.

_validationData.isGlobal = validationConfig.isGlobal();
Expand Down
8 changes: 4 additions & 4 deletions src/account/UpgradeableModularAccount.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"
import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol";

import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} from "../helpers/Constants.sol";
import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {PluginManifest} from "../interfaces/IPlugin.sol";
import {IPluginManager, PluginEntity, ValidationConfig} from "../interfaces/IPluginManager.sol";
Expand All @@ -28,7 +29,6 @@ import {AccountExecutor} from "./AccountExecutor.sol";
import {AccountLoupe} from "./AccountLoupe.sol";
import {AccountStorage, getAccountStorage, toExecutionHook, toSetValue} from "./AccountStorage.sol";
import {AccountStorageInitializable} from "./AccountStorageInitializable.sol";

import {PluginManagerInternals} from "./PluginManagerInternals.sol";

contract UpgradeableModularAccount is
Expand Down Expand Up @@ -441,7 +441,7 @@ contract UpgradeableModularAccount is

// Run the user op validationFunction
{
if (signatureSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) {
if (signatureSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) {
revert ValidationSignatureSegmentMissing();
}

Expand Down Expand Up @@ -497,7 +497,7 @@ contract UpgradeableModularAccount is
_doPreRuntimeValidationHook(preRuntimeValidationHooks[i], callData, currentAuthData);
}

if (authSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) {
if (authSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) {
revert ValidationSignatureSegmentMissing();
}

Expand Down Expand Up @@ -635,7 +635,7 @@ contract UpgradeableModularAccount is
return (new PostExecToRun[](0), new PostExecToRun[](0));
}

PluginEntity directCallValidationKey = PluginEntityLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID);
PluginEntity directCallValidationKey = PluginEntityLib.pack(msg.sender, SELF_PERMIT_VALIDATION_FUNCTIONID);

_checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false);

Expand Down
8 changes: 8 additions & 0 deletions src/helpers/Constants.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

// Index marking the start of the data for the validation function.
uint8 constant RESERVED_VALIDATION_DATA_INDEX = 255;

// Magic value for the Entity ID of direct call validation.
uint32 constant SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max;
11 changes: 2 additions & 9 deletions src/interfaces/IPlugin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,6 @@ struct ManifestExecutionFunction {
bool allowGlobalValidation;
}

// todo: do we need these at all? Or do we fully switch to `installValidation`?
struct ManifestValidation {
uint32 entityId;
bool isGlobal;
bool isSignatureValidation;
bytes4[] selectors;
}

struct ManifestExecutionHook {
// TODO(erc6900 spec): These fields can be packed into a single word
bytes4 executionSelector;
Expand Down Expand Up @@ -54,7 +46,8 @@ struct PluginMetadata {
struct PluginManifest {
// Execution functions defined in this plugin to be installed on the MSCA.
ManifestExecutionFunction[] executionFunctions;
ManifestValidation[] validationFunctions;
bool globalDirectCallValidation;
bytes4[] directCallValidationSelectors;
ManifestExecutionHook[] executionHooks;
// List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
// IPlugin's interface ID.
Expand Down
16 changes: 10 additions & 6 deletions test/account/AccountLoupe.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@ contract AccountLoupeTest is CustomValidationTestBase {

event ReceivedCall(bytes msgData, uint256 msgValue);

PluginEntity public comprehensivePluginValidation;

function setUp() public {
comprehensivePlugin = new ComprehensivePlugin();
comprehensivePluginValidation =
PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION));

_customValidationSetup();

Expand Down Expand Up @@ -61,9 +65,6 @@ contract AccountLoupeTest is CustomValidationTestBase {
}

function test_pluginLoupe_getSelectors() public {
PluginEntity comprehensivePluginValidation =
PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION));

bytes4[] memory selectors = account1.getSelectors(comprehensivePluginValidation);

assertEq(selectors.length, 1);
Expand Down Expand Up @@ -107,7 +108,7 @@ contract AccountLoupeTest is CustomValidationTestBase {
}

function test_pluginLoupe_getValidationHooks() public {
PluginEntity[] memory hooks = account1.getPreValidationHooks(_signerValidation);
PluginEntity[] memory hooks = account1.getPreValidationHooks(comprehensivePluginValidation);

assertEq(hooks.length, 2);
assertEq(
Expand Down Expand Up @@ -144,12 +145,15 @@ contract AccountLoupeTest is CustomValidationTestBase {
address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.PRE_VALIDATION_HOOK_2)
);

bytes4[] memory selectors = new bytes4[](1);
selectors[0] = comprehensivePlugin.foo.selector;

bytes[] memory installDatas = new bytes[](2);
return (
_signerValidation,
comprehensivePluginValidation,
true,
true,
new bytes4[](0),
selectors,
bytes(""),
abi.encode(preValidationHooks, installDatas),
""
Expand Down
12 changes: 9 additions & 3 deletions test/account/SelfCallAuthorization.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,18 @@ contract SelfCallAuthorizationTest is AccountTestBase {

comprehensivePlugin = new ComprehensivePlugin();

comprehensivePluginValidation =
PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION));

bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = ComprehensivePlugin.foo.selector;

vm.startPrank(address(entryPoint));
account1.installPlugin(address(comprehensivePlugin), comprehensivePlugin.pluginManifest(), "");
account1.installValidation(
ValidationConfigLib.pack(comprehensivePluginValidation, false, false), validationSelectors, "", "", ""
);
vm.stopPrank();

comprehensivePluginValidation =
PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION));
}

function test_selfCallFails_userOp() public {
Expand Down
39 changes: 19 additions & 20 deletions test/account/ValidationIntersection.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,21 @@ contract ValidationIntersectionTest is AccountTestBase {
entityId: uint32(MockBaseUserOpValidationPlugin.EntityId.USER_OP_VALIDATION)
});

bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = MockUserOpValidationPlugin.foo.selector;

vm.startPrank(address(entryPoint));
account1.installPlugin({
plugin: address(noHookPlugin),
manifest: noHookPlugin.pluginManifest(),
pluginInstallData: ""
});
account1.installPlugin({
plugin: address(oneHookPlugin),
manifest: oneHookPlugin.pluginManifest(),
pluginInstallData: ""
});
// TODO: change with new install flow
// temporary fix to add the pre-validation hook
// Install noHookValidation
account1.installValidation(
ValidationConfigLib.pack(noHookValidation, true, true),
validationSelectors,
bytes(""),
bytes(""),
bytes("")
);

// Install oneHookValidation
validationSelectors[0] = MockUserOpValidation1HookPlugin.bar.selector;
PluginEntity[] memory preValidationHooks = new PluginEntity[](1);
preValidationHooks[0] = PluginEntityLib.pack({
addr: address(oneHookPlugin),
Expand All @@ -67,17 +69,14 @@ contract ValidationIntersectionTest is AccountTestBase {
bytes[] memory installDatas = new bytes[](1);
account1.installValidation(
ValidationConfigLib.pack(oneHookValidation, true, true),
new bytes4[](0),
validationSelectors,
bytes(""),
abi.encode(preValidationHooks, installDatas),
bytes("")
);
account1.installPlugin({
plugin: address(twoHookPlugin),
manifest: twoHookPlugin.pluginManifest(),
pluginInstallData: ""
});
// temporary fix to add the pre-validation hook

// Install twoHookValidation
validationSelectors[0] = MockUserOpValidation2HookPlugin.baz.selector;
preValidationHooks = new PluginEntity[](2);
preValidationHooks[0] = PluginEntityLib.pack({
addr: address(twoHookPlugin),
Expand All @@ -90,7 +89,7 @@ contract ValidationIntersectionTest is AccountTestBase {
installDatas = new bytes[](2);
account1.installValidation(
ValidationConfigLib.pack(twoHookValidation, true, true),
new bytes4[](0),
validationSelectors,
bytes(""),
abi.encode(preValidationHooks, installDatas),
bytes("")
Expand Down
12 changes: 0 additions & 12 deletions test/mocks/plugins/ComprehensivePlugin.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol";
import {
ManifestExecutionFunction,
ManifestExecutionHook,
ManifestValidation,
PluginManifest,
PluginMetadata
} from "../../../src/interfaces/IPlugin.sol";
Expand Down Expand Up @@ -140,17 +139,6 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba
allowGlobalValidation: false
});

bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = this.foo.selector;

manifest.validationFunctions = new ManifestValidation[](1);
manifest.validationFunctions[0] = ManifestValidation({
entityId: uint32(EntityId.VALIDATION),
isGlobal: true,
isSignatureValidation: false,
selectors: validationSelectors
});

manifest.executionHooks = new ManifestExecutionHook[](3);
manifest.executionHooks[0] = ManifestExecutionHook({
executionSelector: this.foo.selector,
Expand Down
25 changes: 7 additions & 18 deletions test/mocks/plugins/ReturnDataPluginMocks.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,9 @@ pragma solidity ^0.8.19;

import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol";

import {
ManifestExecutionFunction,
ManifestValidation,
PluginManifest,
PluginMetadata
} from "../../../src/interfaces/IPlugin.sol";
import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol";

import {SELF_PERMIT_VALIDATION_FUNCTIONID} from "../../../src/helpers/Constants.sol";

import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol";
import {IValidation} from "../../../src/interfaces/IValidation.sol";
Expand Down Expand Up @@ -103,7 +100,8 @@ contract ResultConsumerPlugin is BasePlugin, IValidation {
// This result should be allowed based on the manifest permission request
bytes memory returnData = IStandardExecutor(msg.sender).executeWithAuthorization(
abi.encodeCall(IStandardExecutor.execute, (target, 0, abi.encodeCall(RegularResultContract.foo, ()))),
abi.encodePacked(this, uint32(0), uint8(0), uint32(1), uint8(255)) // Validation function of self,
abi.encodePacked(this, SELF_PERMIT_VALIDATION_FUNCTIONID, uint8(0), uint32(1), uint8(255)) // Validation
// function of self,
// selector-associated, with no auth data
);

Expand All @@ -119,17 +117,8 @@ contract ResultConsumerPlugin is BasePlugin, IValidation {
function pluginManifest() external pure override returns (PluginManifest memory) {
PluginManifest memory manifest;

// todo: this is the exact workflow that would benefit from a "permiteed call" setup in the manifest.
bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = IStandardExecutor.execute.selector;

manifest.validationFunctions = new ManifestValidation[](1);
manifest.validationFunctions[0] = ManifestValidation({
entityId: 0,
isGlobal: true,
isSignatureValidation: false,
selectors: validationSelectors
});
manifest.directCallValidationSelectors = new bytes4[](1);
manifest.directCallValidationSelectors[0] = IStandardExecutor.execute.selector;

manifest.executionFunctions = new ManifestExecutionFunction[](2);
manifest.executionFunctions[0] = ManifestExecutionFunction({
Expand Down
Loading

0 comments on commit 8a1fe73

Please sign in to comment.