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.

Sign up for GitHub

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: single signer validation duplicate view func, missing 1271 UO validation #124

Merged
merged 1 commit into from
Aug 1, 2024
Merged

fix: single signer validation duplicate view func, missing 1271 UO validation #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 0 additions & 6 deletions src/modules/validation/ISingleSignerValidation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,4 @@ interface ISingleSignerValidation is IValidation {
/// @param entityId The entityId for the account and the signer.
/// @param newSigner The address of the new signer.
function transferSigner(uint32 entityId, address newSigner) external;

/// @notice Get the signer of the `account`'s validation.
/// @param entityId The entityId for the account and the signer.
/// @param account The account to get the signer of.
/// @return The address of the signer.
function signerOf(uint32 entityId, address account) external view returns (address);
}
28 changes: 12 additions & 16 deletions src/modules/validation/SingleSignerValidation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
pragma solidity ^0.8.25;

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

Expand All @@ -25,7 +24,6 @@ import {ISingleSignerValidation} from "./ISingleSignerValidation.sol";
/// - This validation supports composition that other validation can relay on entities in this validation
/// to validate partially or fully.
contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
using ECDSA for bytes32;
using MessageHashUtils for bytes32;

string internal constant _NAME = "SingleSigner Validation";
Expand All @@ -39,7 +37,7 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
bytes4 internal constant _1271_INVALID = 0xffffffff;

mapping(uint32 entityId => mapping(address account => address)) public signer;
mapping(uint32 entityId => mapping(address account => address)) public signers;

/// @inheritdoc ISingleSignerValidation
function transferSigner(uint32 entityId, address newSigner) external {
Expand All @@ -59,11 +57,6 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
_transferSigner(abi.decode(data, (uint32)), address(0));
}

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

/// @inheritdoc IValidation
function validateUserOp(uint32 entityId, PackedUserOperation calldata userOp, bytes32 userOpHash)
external
Expand All @@ -72,11 +65,14 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
returns (uint256)
{
// Validate the user op signature against the owner.
(address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature);
if (sigSigner == address(0) || sigSigner != signer[entityId][userOp.sender]) {
return _SIG_VALIDATION_FAILED;
if (
SignatureChecker.isValidSignatureNow(
signers[entityId][userOp.sender], userOpHash.toEthSignedMessageHash(), userOp.signature
)
) {
return _SIG_VALIDATION_PASSED;
}
return _SIG_VALIDATION_PASSED;
return _SIG_VALIDATION_FAILED;
}

/// @inheritdoc IValidation
Expand All @@ -89,7 +85,7 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
bytes calldata
) external view override {
// Validate that the sender is the owner of the account or self.
if (sender != signer[entityId][account]) {
if (sender != signers[entityId][account]) {
revert NotAuthorized();
}
return;
Expand All @@ -108,7 +104,7 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
override
returns (bytes4)
{
if (SignatureChecker.isValidSignatureNow(signer[entityId][account], digest, signature)) {
if (SignatureChecker.isValidSignatureNow(signers[entityId][account], digest, signature)) {
return _1271_MAGIC_VALUE;
}
return _1271_INVALID;
Expand All @@ -132,8 +128,8 @@ contract SingleSignerValidation is ISingleSignerValidation, BaseModule {
// ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

function _transferSigner(uint32 entityId, address newSigner) internal {
address previousSigner = signer[entityId][msg.sender];
signer[entityId][msg.sender] = newSigner;
address previousSigner = signers[entityId][msg.sender];
signers[entityId][msg.sender] = newSigner;
emit SignerTransferred(msg.sender, entityId, previousSigner, newSigner);
}
}
4 changes: 2 additions & 2 deletions test/account/UpgradeableModularAccount.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ contract UpgradeableModularAccountTest is AccountTestBase {
}

function test_transferOwnership() public {
assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1);
assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner1);

vm.prank(address(entryPoint));
account1.execute(
Expand All @@ -363,7 +363,7 @@ contract UpgradeableModularAccountTest is AccountTestBase {
abi.encodeCall(SingleSignerValidation.transferSigner, (TEST_DEFAULT_VALIDATION_ENTITY_ID, owner2))
);

assertEq(singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner2);
assertEq(singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, address(account1)), owner2);
}

function test_isValidSignature() public {
Expand Down
2 changes: 1 addition & 1 deletion test/module/SingleSignerValidation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ contract SingleSignerValidationTest is AccountTestBase {

// transfer ownership to signer
singleSignerValidation.transferSigner(TEST_DEFAULT_VALIDATION_ENTITY_ID, signer);
assertEq(signer, singleSignerValidation.signerOf(TEST_DEFAULT_VALIDATION_ENTITY_ID, accountAddr));
assertEq(signer, singleSignerValidation.signers(TEST_DEFAULT_VALIDATION_ENTITY_ID, accountAddr));

// sig check should pass
assertEq(
Expand Down
Loading