Skip to content

Commit

Permalink
move signature to calldata
Browse files Browse the repository at this point in the history
  • Loading branch information
Amxx committed Jul 12, 2024
1 parent add9961 commit 2004e2a
Show file tree
Hide file tree
Showing 11 changed files with 92 additions and 65 deletions.
12 changes: 6 additions & 6 deletions contracts/abstraction/account/Account.sol
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ abstract contract Account is IAccount {
*
* Subclass must implement this using their own choice of cryptography.
*/
function _recoverSigner(bytes memory signature, bytes32 userOpHash) internal virtual returns (address);
function _recoverSigner(bytes32 userOpHash, bytes calldata signature) internal virtual returns (address);

/****************************************************************************************************************
* Public interface *
Expand All @@ -73,7 +73,7 @@ abstract contract Account is IAccount {
bytes32 userOpHash,
uint256 missingAccountFunds
) public virtual override onlyEntryPoint returns (uint256 validationData) {
(bool valid, , uint48 validAfter, uint48 validUntil) = _processSignature(userOp.signature, userOpHash);
(bool valid, , uint48 validAfter, uint48 validUntil) = _processSignature(userOpHash, userOp.signature);
_validateNonce(userOp.nonce);
_payPrefund(missingAccountFunds);
return ERC4337Utils.packValidationData(valid, validAfter, validUntil);
Expand All @@ -85,18 +85,18 @@ abstract contract Account is IAccount {

/**
* @dev Process the signature is valid for this message.
* @param signature - The user's signature
* @param userOpHash - Hash of the request that must be signed (includes the entrypoint and chain id)
* @param signature - The user's signature
* @return valid - Signature is valid
* @return signer - Address of the signer that produced the signature
* @return validAfter - first timestamp this operation is valid
* @return validUntil - last timestamp this operation is valid. 0 for "indefinite"
*/
function _processSignature(
bytes memory signature,
bytes32 userOpHash
bytes32 userOpHash,
bytes calldata signature
) internal virtual returns (bool valid, address signer, uint48 validAfter, uint48 validUntil) {
address recovered = _recoverSigner(signature, userOpHash);
address recovered = _recoverSigner(userOpHash, signature);
return (recovered != address(0) && _isAuthorized(recovered), recovered, 0, 0);
}

Expand Down
30 changes: 23 additions & 7 deletions contracts/abstraction/account/modules/AccountMultisig.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,24 @@ abstract contract AccountMultisig is Account {
function requiredSignatures() public view virtual returns (uint256);

function _processSignature(
bytes memory signature,
bytes32 userOpHash
bytes32 userOpHash,
bytes calldata signatures
) internal virtual override returns (bool, address, uint48, uint48) {
bytes[] memory signatures = abi.decode(signature, (bytes[]));
uint256 arrayLength = _getUint256(signatures, _getUint256(signatures, 0));

if (signatures.length < requiredSignatures()) {
if (arrayLength < requiredSignatures()) {
return (false, address(0), 0, 0);
}

address lastSigner = address(0);
uint48 globalValidAfter = 0;
uint48 globalValidUntil = 0;

for (uint256 i = 0; i < signatures.length; ++i) {
for (uint256 i = 0; i < arrayLength; ++i) {
bytes calldata signature = _getBytesArrayElement(signatures, i);
(bool valid, address signer, uint48 validAfter, uint48 validUntil) = super._processSignature(
signatures[i],
userOpHash
userOpHash,
signature
);
if (valid && signer > lastSigner) {
lastSigner = signer;
Expand All @@ -41,4 +42,19 @@ abstract contract AccountMultisig is Account {
}
return (true, address(this), globalValidAfter, globalValidUntil);
}

function _getUint256(bytes calldata data, uint256 pos) private pure returns (uint256 result) {
assembly ("memory-safe") {
result := calldataload(add(data.offset, pos))
}
}

function _getBytesArrayElement(bytes calldata data, uint256 i) private pure returns (bytes calldata result) {
assembly ("memory-safe") {
let begin := add(calldataload(data.offset), 0x20)
let offset := add(calldataload(add(add(data.offset, begin), mul(i, 0x20))), begin)
result.length := calldataload(add(data.offset, offset))
result.offset := add(add(data.offset, offset), 0x20)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ abstract contract AccountAllSignatures is AccountECDSA, AccountERC1271 {
}

function _recoverSigner(
bytes memory signature,
bytes32 userOpHash
bytes32 userOpHash,
bytes calldata signature
) internal virtual override(AccountECDSA, AccountERC1271) returns (address) {
(SignatureType sigType, bytes memory sigData) = abi.decode(signature, (SignatureType, bytes));
SignatureType sigType = SignatureType(uint8(bytes1(signature)));

if (sigType == SignatureType.ECDSA) {
return AccountECDSA._recoverSigner(sigData, userOpHash);
return AccountECDSA._recoverSigner(userOpHash, signature[0x01:]);
} else if (sigType == SignatureType.ERC1271) {
return AccountERC1271._recoverSigner(sigData, userOpHash);
return AccountERC1271._recoverSigner(userOpHash, signature[0x01:]);
} else {
return address(0);
}
Expand Down
14 changes: 7 additions & 7 deletions contracts/abstraction/account/modules/recovery/AccountECDSA.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import {Account} from "../../Account.sol";

abstract contract AccountECDSA is Account {
function _recoverSigner(
bytes memory signature,
bytes32 userOpHash
bytes32 userOpHash,
bytes calldata signature
) internal virtual override returns (address signer) {
bytes32 msgHash = MessageHashUtils.toEthSignedMessageHash(userOpHash);

Expand All @@ -24,18 +24,18 @@ abstract contract AccountECDSA is Account {
uint8 v;
/// @solidity memory-safe-assembly
assembly {
r := mload(add(signature, 0x20))
s := mload(add(signature, 0x40))
v := byte(0, mload(add(signature, 0x60)))
r := calldataload(add(signature.offset, 0x00))
s := calldataload(add(signature.offset, 0x20))
v := byte(0, calldataload(add(signature.offset, 0x40)))
}
(signer, , ) = ECDSA.tryRecover(msgHash, v, r, s); // return address(0) on errors
} else if (signature.length == 64) {
bytes32 r;
bytes32 vs;
/// @solidity memory-safe-assembly
assembly {
r := mload(add(signature, 0x20))
vs := mload(add(signature, 0x40))
r := calldataload(add(signature.offset, 0x00))
vs := calldataload(add(signature.offset, 0x20))
}
(signer, , ) = ECDSA.tryRecover(msgHash, r, vs);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import {Account} from "../../Account.sol";
abstract contract AccountERC1271 is Account {
error P256InvalidSignatureLength(uint256 length);

function _recoverSigner(bytes memory signature, bytes32 userOpHash) internal virtual override returns (address) {
function _recoverSigner(bytes32 userOpHash, bytes calldata signature) internal virtual override returns (address) {
bytes32 msgHash = MessageHashUtils.toEthSignedMessageHash(userOpHash);
(address signer, bytes memory sig) = abi.decode(signature, (address, bytes));
address signer = address(bytes20(signature[0x00:0x14]));

return SignatureChecker.isValidERC1271SignatureNow(signer, msgHash, sig) ? signer : address(0);
return SignatureChecker.isValidERC1271SignatureNow(signer, msgHash, signature[0x14:]) ? signer : address(0);
}
}
6 changes: 3 additions & 3 deletions contracts/abstraction/mocks/AdvancedAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ contract AdvancedAccount is AccessControl, AccountCommon, AccountAllSignatures,
}

function _processSignature(
bytes memory signature,
bytes32 userOpHash
bytes32 userOpHash,
bytes calldata signature
) internal virtual override(Account, AccountMultisig) returns (bool, address, uint48, uint48) {
return super._processSignature(signature, userOpHash);
return super._processSignature(userOpHash, signature);
}
}
14 changes: 7 additions & 7 deletions test/abstraction/accountMultisig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ async function fixture() {
accounts.beneficiary = accounts.shift();

// 4337 helper
const helper = new ERC4337Helper('AdvancedAccount');
const helper = new ERC4337Helper('AdvancedAccount', { withTypePrefix: true });
const identity = new IdentityHelper();

// environment
Expand Down Expand Up @@ -48,7 +48,7 @@ describe('AccountMultisig', function () {
});

describe('account not deployed yet', function () {
it('success: deploy and call', async function () {
it.only('success: deploy and call', async function () {
const operation = await this.sender
.createOp({
callData: this.sender.interface.encodeFunctionData('execute', [
Expand All @@ -58,7 +58,7 @@ describe('AccountMultisig', function () {
]),
})
.then(op => op.addInitCode())
.then(op => op.sign(this.signers, true));
.then(op => op.sign(this.signers));

await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary))
.to.emit(this.entrypoint, 'AccountDeployed')
Expand All @@ -82,7 +82,7 @@ describe('AccountMultisig', function () {
this.target.interface.encodeFunctionData('mockFunctionExtra'),
]),
})
.then(op => op.sign(this.signers, true));
.then(op => op.sign(this.signers));

await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary))
.to.emit(this.target, 'MockFunctionCalledExtra')
Expand All @@ -98,7 +98,7 @@ describe('AccountMultisig', function () {
this.target.interface.encodeFunctionData('mockFunctionExtra'),
]),
})
.then(op => op.sign([this.signers[0], this.signers[2]], true));
.then(op => op.sign([this.signers[0], this.signers[2]]));

await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary))
.to.emit(this.target, 'MockFunctionCalledExtra')
Expand All @@ -114,7 +114,7 @@ describe('AccountMultisig', function () {
this.target.interface.encodeFunctionData('mockFunctionExtra'),
]),
})
.then(op => op.sign([this.signers[2]], true));
.then(op => op.sign([this.signers[2]]));

await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary))
.to.be.revertedWithCustomError(this.entrypoint, 'FailedOp')
Expand All @@ -130,7 +130,7 @@ describe('AccountMultisig', function () {
this.target.interface.encodeFunctionData('mockFunctionExtra'),
]),
})
.then(op => op.sign([this.accounts.relayer, this.signers[2]], true));
.then(op => op.sign([this.accounts.relayer, this.signers[2]]));

await expect(this.entrypoint.handleOps([operation.packed], this.accounts.beneficiary))
.to.be.revertedWithCustomError(this.entrypoint, 'FailedOp')
Expand Down
1 change: 1 addition & 0 deletions test/helpers/enums.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ module.exports = {
Rounding: Enum('Floor', 'Ceil', 'Trunc', 'Expand'),
OperationState: Enum('Unset', 'Waiting', 'Ready', 'Done'),
RevertType: Enum('None', 'RevertWithoutMessage', 'RevertWithMessage', 'RevertWithCustomError', 'Panic'),
SignatureType: Enum('ECDSA', 'ERC1271'),
};
14 changes: 7 additions & 7 deletions test/helpers/erc4337.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
const { ethers } = require('hardhat');

const { SignatureType } = require('./identity');
const { SignatureType } = require('./enums');

function pack(left, right) {
return ethers.solidityPacked(['uint128', 'uint128'], [left, right]);
}

/// Global ERC-4337 environment helper.
class ERC4337Helper {
constructor(account = 'SimpleAccountECDSA') {
constructor(account = 'SimpleAccountECDSA', params = {}) {
this.entrypointAsPromise = ethers.deployContract('EntryPoint');
this.factoryAsPromise = ethers.deployContract('$Create2');
this.accountAsPromise = ethers.getContractFactory(account);
this.chainIdAsPromise = ethers.provider.getNetwork().then(({ chainId }) => chainId);
this.params = params;
}

async wait() {
Expand Down Expand Up @@ -133,18 +134,17 @@ class UserOperation {
return this;
}

async sign(signer = this.sender.runner, withTypePrefix = false) {
async sign(signer = this.sender.runner, args = {}) {
const withTypePrefix = args.withTypePrefix ?? this.sender.context.params.withTypePrefix;

const signers = (Array.isArray(signer) ? signer : [signer]).sort(
(signer1, signer2) => signer1.address - signer2.address,
);
const signatures = await Promise.all(
signers.map(signer =>
Promise.resolve(signer.signMessage(ethers.getBytes(this.hash))).then(signature =>
withTypePrefix
? ethers.AbiCoder.defaultAbiCoder().encode(
['uint8', 'bytes'],
[signer.type ?? SignatureType.ECDSA, signature],
)
? ethers.solidityPacked(['uint8', 'bytes'], [signer.type ?? SignatureType.ECDSA, signature])
: signature,
),
),
Expand Down
14 changes: 7 additions & 7 deletions test/helpers/identity.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
const { ethers } = require('hardhat');

const { Enum } = require('./enums');
const { P256Signer } = require('./p256');

const SignatureType = Enum('ECDSA', 'ERC1271');
const { SignatureType } = require('./enums');

class IdentityHelper {
constructor() {
Expand All @@ -21,13 +19,15 @@ class IdentityHelper {
return Object.assign(ethers.Wallet.createRandom(), { type: SignatureType.ECDSA });
}

async newP256Signer(sigParams = { prefixAddress: true }) {
async newP256Signer(params = { withPrefixAddress: true }) {
await this.wait();

const signer = P256Signer.random();
return Promise.all([this.p256Factory.predict(signer.publicKey), this.p256Factory.create(signer.publicKey)]).then(
([address]) => Object.assign(signer, { address, sigParams, type: SignatureType.ERC1271 }),
const signer = P256Signer.random(params);
await Promise.all([this.p256Factory.predict(signer.publicKey), this.p256Factory.create(signer.publicKey)]).then(
([address]) => Object.assign(signer, { address }),
);

return signer;
}

async newRSASigner() {
Expand Down
35 changes: 22 additions & 13 deletions test/helpers/p256.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
const { ethers } = require('hardhat');
const { secp256r1 } = require('@noble/curves/p256');

const N = 0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551n;
const { SignatureType } = require('./enums');

class P256Signer {
constructor(privateKey) {
constructor(privateKey, params = {}) {
this.privateKey = privateKey;
this.publicKey = ethers.concat(
[
Expand All @@ -13,11 +13,15 @@ class P256Signer {
].map(ethers.hexlify),
);
this.address = ethers.getAddress(ethers.keccak256(this.publicKey).slice(-40));
this.sigParams = { prefixAddress: false, includeRecovery: true };
this.params = Object.assign({ withPrefixAddress: false, withRecovery: true }, params);
}

static random() {
return new P256Signer(secp256r1.utils.randomPrivateKey());
get type() {
return SignatureType.ERC1271;
}

static random(params = {}) {
return new P256Signer(secp256r1.utils.randomPrivateKey(), params);
}

getAddress() {
Expand All @@ -28,18 +32,23 @@ class P256Signer {
let { r, s, recovery } = secp256r1.sign(ethers.hashMessage(message).replace(/0x/, ''), this.privateKey);

// ensureLowerOrderS
if (s > N / 2n) {
s = N - s;
if (s > secp256r1.CURVE.n / 2n) {
s = secp256r1.CURVE.n - s;
recovery = 1 - recovery;
}

// pack signature
const signature = this.sigParams.includeRecovery
? ethers.solidityPacked(['uint256', 'uint256', 'uint8'], [r, s, recovery])
: ethers.solidityPacked(['uint256', 'uint256'], [r, s]);
return this.sigParams.prefixAddress
? ethers.AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [this.address, signature])
: signature;
const elements = [
this.params.withPrefixAddress && { type: 'address', value: this.address },
{ type: 'uint256', value: ethers.toBeHex(r) },
{ type: 'uint256', value: ethers.toBeHex(s) },
this.params.withRecovery && { type: 'uint8', value: recovery },
].filter(Boolean);

return ethers.solidityPacked(
elements.map(({ type }) => type),
elements.map(({ value }) => value),
);
}
}

Expand Down

0 comments on commit 2004e2a

Please sign in to comment.