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

[3/n] Enable validation composability and add composable single singer validation and tests #96

Merged
merged 7 commits into from
Jul 17, 2024
10 changes: 7 additions & 3 deletions src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,10 @@ contract UpgradeableModularAccount is
revert SignatureValidationInvalid(plugin, entityId);
}

if (IValidation(plugin).validateSignature(entityId, msg.sender, hash, signature[24:]) == _1271_MAGIC_VALUE)
{
if (
IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this should be addressed in this PR, but if the return values are _1271_MAGIC_VALUE or _1271_INVALID, would prefer to just remove the if/else part and do return IValidation(plugin).validateSignature(address(this), entityId, msg.sender, hash, signature[24:])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's preferable to do the equivalence check before returning, because then we can absorb any bad values that are valid bytes4 types but aren't _1271_MAGIC_VALUE or _1271_INVALID.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm from the spec, returning anything thats not _1271_MAGIC_VALUE should be considered invalid. Don't feel super strongly about this, just a LOC reduction maxi

== _1271_MAGIC_VALUE
) {
return _1271_MAGIC_VALUE;
}
return _1271_INVALID;
Expand Down Expand Up @@ -517,7 +519,9 @@ contract UpgradeableModularAccount is

(address plugin, uint32 entityId) = runtimeValidationFunction.unpack();

try IValidation(plugin).validateRuntime(entityId, msg.sender, msg.value, callData, authSegment.getBody())
try IValidation(plugin).validateRuntime(
address(this), entityId, msg.sender, msg.value, callData, authSegment.getBody()
)
// forgefmt: disable-start
// solhint-disable-next-line no-empty-blocks
{} catch (bytes memory revertReason) {
Expand Down
14 changes: 10 additions & 4 deletions src/interfaces/IValidation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ interface IValidation is IPlugin {

/// @notice Run the runtime validationFunction specified by the `entityId`.
/// @dev To indicate the entire call should revert, the function MUST revert.
/// @param account the account to validate for.
/// @param entityId An identifier that routes the call to different internal implementations, should there
/// be more than one.
/// @param sender The caller address.
/// @param value The call value.
/// @param data The calldata sent.
/// @param authorization Additional data for the validation function to use.
function validateRuntime(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating these other two validation functions - with this, we should have full composability across all of the different types of validation functions.

address account,
uint32 entityId,
address sender,
uint256 value,
Expand All @@ -34,14 +36,18 @@ interface IValidation is IPlugin {

/// @notice Validates a signature using ERC-1271.
/// @dev To indicate the entire call should revert, the function MUST revert.
/// @param account the account to validate for.
/// @param entityId An identifier that routes the call to different internal implementations, should there
/// be more than one.
/// @param sender the address that sent the ERC-1271 request to the smart account
/// @param hash the hash of the ERC-1271 request
/// @param signature the signature of the ERC-1271 request
/// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid.
function validateSignature(uint32 entityId, address sender, bytes32 hash, bytes calldata signature)
external
view
returns (bytes4);
function validateSignature(
address account,
uint32 entityId,
address sender,
bytes32 hash,
bytes calldata signature
) external view returns (bytes4);
}
19 changes: 11 additions & 8 deletions src/plugins/owner/SingleOwnerPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,16 @@ contract SingleOwnerPlugin is IValidation, BasePlugin {
}

/// @inheritdoc IValidation
function validateRuntime(uint32 entityId, address sender, uint256, bytes calldata, bytes calldata)
external
view
override
{
function validateRuntime(
address account,
uint32 entityId,
address sender,
uint256,
bytes calldata,
bytes calldata
) external view override {
// Validate that the sender is the owner of the account or self.
if (sender != owners[entityId][msg.sender]) {
if (sender != owners[entityId][account]) {
revert NotAuthorized();
}
return;
fangting-alchemy marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -123,13 +126,13 @@ contract SingleOwnerPlugin is IValidation, BasePlugin {
/// validation used in `validateUserOp`, this does///*not** wrap the digest in
/// an "Ethereum Signed Message" envelope before checking the signature in
/// the EOA-owner case.
function validateSignature(uint32 entityId, address, bytes32 digest, bytes calldata signature)
function validateSignature(address account, uint32 entityId, address, bytes32 digest, bytes calldata signature)
external
view
override
returns (bytes4)
{
if (SignatureChecker.isValidSignatureNow(owners[entityId][msg.sender], digest, signature)) {
if (SignatureChecker.isValidSignatureNow(owners[entityId][account], digest, signature)) {
return _1271_MAGIC_VALUE;
}
return _1271_INVALID;
Expand Down
28 changes: 28 additions & 0 deletions src/plugins/validation/ISingleSignerValidation.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.25;

import {IValidation} from "../../interfaces/IValidation.sol";

interface ISingleSignerValidation is IValidation {
/// @notice This event is emitted when Signer of the account's validation changes.
/// @param account The account whose validation Signer changed.
/// @param validationId The validationId for the account and the signer.
/// @param previousSigner The address of the previous signer.
/// @param newSigner The address of the new signer.
event SignerTransferred(
address indexed account, uint32 indexed validationId, address previousSigner, address newSigner
);

error NotAuthorized();

/// @notice Transfer Signer of the account's validation to `newSigner`.
/// @param validationId The validationId for the account and the signer.
/// @param newSigner The address of the new signer.
function transferSigner(uint32 validationId, address newSigner) external;

/// @notice Get the signer of the `account`'s validation.
/// @param validationId The validationId for the account and the signer.
/// @param account The account to get the signer of.
/// @return The address of the signer.
function signerOf(uint32 validationId, address account) external view returns (address);
}
146 changes: 146 additions & 0 deletions src/plugins/validation/SingleSignerValidation.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.25;

import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol";
import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol";

import {IPlugin, PluginManifest, PluginMetadata} from "../../interfaces/IPlugin.sol";
import {IValidation} from "../../interfaces/IValidation.sol";
import {BasePlugin} from "../BasePlugin.sol";
import {ISingleSignerValidation} from "./ISingleSignerValidation.sol";

/// @title ECSDA Validation
/// @author ERC-6900 Authors
/// @notice This validation enables any ECDSA (secp256k1 curve) signature validation. It handles installation by
/// each entity (validationId).
/// Note: Uninstallation will NOT disable all installed validation entities. None of the functions are installed on
/// the account. Account states are to be retrieved from this global singleton directly.
///
/// - This validation supports ERC-1271. The signature is valid if it is signed by the owner's private key
/// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the
/// owner (if the owner is a contract).
///
/// - This validation supports composition that other validation can relay on entities in this validation
/// to validate partially or fully.
contract SingleSignerValidation is ISingleSignerValidation, BasePlugin {
using ECDSA for bytes32;
using MessageHashUtils for bytes32;

string public constant NAME = "SingleSigner Validation";
string public constant VERSION = "1.0.0";
string public constant AUTHOR = "ERC-6900 Authors";

uint256 internal constant _SIG_VALIDATION_PASSED = 0;
uint256 internal constant _SIG_VALIDATION_FAILED = 1;

// bytes4(keccak256("isValidSignature(bytes32,bytes)"))
bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
bytes4 internal constant _1271_INVALID = 0xffffffff;

mapping(uint32 validationId => mapping(address account => address)) public signer;

/// @inheritdoc ISingleSignerValidation
function signerOf(uint32 validationId, address account) external view returns (address) {
return signer[validationId][account];
}

/// @inheritdoc ISingleSignerValidation
function transferSigner(uint32 validationId, address newSigner) external {

Check warning on line 50 in src/plugins/validation/SingleSignerValidation.sol

View workflow job for this annotation

GitHub Actions / Run Linters

Function order is incorrect, external function can not go after external view function (line 45)
_transferSigner(validationId, newSigner);
}

// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ Plugin interface functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

/// @inheritdoc IPlugin
function pluginManifest() external pure override returns (PluginManifest memory) {
PluginManifest memory manifest;
return manifest;
}

/// @inheritdoc IPlugin
function pluginMetadata() external pure virtual override returns (PluginMetadata memory) {
PluginMetadata memory metadata;
metadata.name = NAME;
metadata.version = VERSION;
metadata.author = AUTHOR;
return metadata;
}

/// @inheritdoc IPlugin
function onInstall(bytes calldata data) external override {
(uint32 validationId, address newSigner) = abi.decode(data, (uint32, address));
_transferSigner(validationId, newSigner);
}

/// @inheritdoc IPlugin
function onUninstall(bytes calldata data) external override {
// ToDo: what does it mean in the world of composable validation world to uninstall one type of validation
// We can either get rid of all SingleSigner signers. What about the nested ones?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to support the flow of getting rid of 1. Use case would be something like revoking 1 dapp session key by uninstalling the validation for that entity. Open to having a global "uninstall all" type of feature though

_transferSigner(abi.decode(data, (uint32)), address(0));
}

/// @inheritdoc IValidation
function validateUserOp(uint32 validationId, PackedUserOperation calldata userOp, bytes32 userOpHash)
external
view
override
returns (uint256)
{
// Validate the user op signature against the owner.
(address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature);
if (sigSigner == address(0) || sigSigner != signer[validationId][userOp.sender]) {
return _SIG_VALIDATION_FAILED;
}
return _SIG_VALIDATION_PASSED;
}

/// @inheritdoc IValidation
function validateRuntime(
address account,
uint32 validationId,
address sender,
uint256,
bytes calldata,
bytes calldata
) external view override {
// Validate that the sender is the owner of the account or self.
if (sender != signer[validationId][account]) {
revert NotAuthorized();
}
return;
}

/// @inheritdoc IValidation
/// @dev The signature is valid if it is signed by the owner's private key
/// (if the owner is an EOA) or if it is a valid ERC-1271 signature from the
/// owner (if the owner is a contract). Note that unlike the signature
/// validation used in `validateUserOp`, this does///*not** wrap the digest in
/// an "Ethereum Signed Message" envelope before checking the signature in
/// the EOA-owner case.
function validateSignature(
address account,
uint32 validationId,
address,
bytes32 digest,
bytes calldata signature
) external view override returns (bytes4) {
if (SignatureChecker.isValidSignatureNow(signer[validationId][account], digest, signature)) {
return _1271_MAGIC_VALUE;
}
return _1271_INVALID;
}

// ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
// ┃ Internal / Private functions ┃
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

function _transferSigner(uint32 validationId, address newSigner) internal {
address previousSigner = signer[validationId][msg.sender];
signer[validationId][msg.sender] = newSigner;
emit SignerTransferred(msg.sender, validationId, previousSigner, newSigner);
}
}
79 changes: 79 additions & 0 deletions test/mocks/SingleSignerFactoryFixture.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {Create2} from "@openzeppelin/contracts/utils/Create2.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol";

import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol";
import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol";
import {SingleSignerValidation} from "../../src/plugins/validation/SingleSignerValidation.sol";

import {OptimizedTest} from "../utils/OptimizedTest.sol";
import {TEST_DEFAULT_VALIDATION_ID} from "../utils/TestConstants.sol";

contract SingleSignerFactoryFixture is OptimizedTest {
UpgradeableModularAccount public accountImplementation;
SingleSignerValidation public singleSignerValidation;
bytes32 private immutable _PROXY_BYTECODE_HASH;

uint32 public constant UNSTAKE_DELAY = 1 weeks;

IEntryPoint public entryPoint;

address public self;

constructor(IEntryPoint _entryPoint, SingleSignerValidation _singleSignerValidation) {
entryPoint = _entryPoint;
accountImplementation = _deployUpgradeableModularAccount(_entryPoint);
_PROXY_BYTECODE_HASH = keccak256(
abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), ""))
);
singleSignerValidation = _singleSignerValidation;
self = address(this);
}

/**
* create an account, and return its address.
* returns the address even if the account is already deployed.
* Note that during user operation execution, this method is called only if the account is not deployed.
* This method returns an existing account address so that entryPoint.getSenderAddress() would work even after
* account creation
*/
function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) {
address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH);

// short circuit if exists
if (addr.code.length == 0) {
bytes memory pluginInstallData = abi.encode(TEST_DEFAULT_VALIDATION_ID, owner);
// not necessary to check return addr since next call will fail if so
new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), "");

// point proxy to actual implementation and init plugins
UpgradeableModularAccount(payable(addr)).initializeWithValidation(
ValidationConfigLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ID, true, false),
new bytes4[](0),
pluginInstallData,
"",
""
);
}

return UpgradeableModularAccount(payable(addr));
}

/**
* calculate the counterfactual address of this account as it would be returned by createAccount()
*/
function getAddress(address owner, uint256 salt) public view returns (address) {
return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH);
}

function addStake() external payable {
entryPoint.addStake{value: msg.value}(UNSTAKE_DELAY);
}

function getSalt(address owner, uint256 salt) public pure returns (bytes32) {
return keccak256(abi.encodePacked(owner, salt));
}
}
8 changes: 6 additions & 2 deletions test/mocks/plugins/ComprehensivePlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba
revert NotImplemented();
}

function validateRuntime(uint32 entityId, address, uint256, bytes calldata, bytes calldata)
function validateRuntime(address, uint32 entityId, address, uint256, bytes calldata, bytes calldata)
external
pure
override
Expand All @@ -96,7 +96,11 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba
revert NotImplemented();
}

function validateSignature(uint32 entityId, address, bytes32, bytes calldata) external pure returns (bytes4) {
function validateSignature(address, uint32 entityId, address, bytes32, bytes calldata)
external
pure
returns (bytes4)
{
if (entityId == uint32(EntityId.SIG_VALIDATION)) {
return 0xffffffff;
}
Expand Down
7 changes: 5 additions & 2 deletions test/mocks/plugins/ReturnDataPluginMocks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,16 @@ contract ResultConsumerPlugin is BasePlugin, IValidation {
revert NotImplemented();
}

function validateRuntime(uint32, address sender, uint256, bytes calldata, bytes calldata) external view {
function validateRuntime(address, uint32, address sender, uint256, bytes calldata, bytes calldata)
external
view
{
if (sender != address(this)) {
revert NotAuthorized();
}
}

function validateSignature(uint32, address, bytes32, bytes calldata) external pure returns (bytes4) {
function validateSignature(address, uint32, address, bytes32, bytes calldata) external pure returns (bytes4) {
revert NotImplemented();
}

Expand Down
Loading
Loading