Skip to content

Commit

Permalink
Add an E2E test for nested Safes in 4337 context (#273)
Browse files Browse the repository at this point in the history
This PR adds an E2E test case for nested Safe transaction execution in
the 4337 Context. The test uses the following ownership structure:

![image](https://github.com/safe-global/safe-modules/assets/16622558/3e420068-3d3b-4db4-91f6-3da4febc350e)

I implemented helper classes representing the Safe ownership tree with
two node types: Safe and EOA. The tests also required functions to find
an execution path and build the user operation from the execution path.
The alternative would've been to do everything imperatively, but using
those functions, we could theoretically represent arbitrary ownership
structure, find an execution path (currently, the function doesn't find
the most optimal one) and generate a user operation.

The current algorithm works as follows (thanks to @nlordell for the
help):
1. **Finding an execution path:** It does a DFS on the tree until it
finds the first Executor node. We consider an executor node a leaf safe
owned by an EOA. In the context of the ownership structure on the
screenshot, the executor would be the leftmost Leaf Safe. The execution
path is represented by an array ordered from the root node to the
executor node.
2. **Building the user operation:** Using the execution path and our
desired `SafeTransaction` (the one executed with `execTransaction`), we
recursively sign the transaction with all the node owners in the
execution path. After that, we re-encode the `SafeTransaction` for the
next node with a call to the preceding node in the execution path and do
the signing again. It stops when we reach the starting node (the node
before the executor 4337 Safe). After that, we encode and send the
SafeTransaction in a User Operation.

- Implements #251 
- I also had to bring quite some code for executing regular Safe
transactions from the `safe-smart-account` repo

I tried to document the code as much as possible, but somehow, I feel it
still wasn't enough - please feel free to point me to the parts that are
unclear, and I'll do my best to explain them with comments.

I'll quote the Slack message from @nlordell that describes the idea
implemented in this PR:

> As you may already be aware, nested Safe ownership structures are
problematic in the context of 4337 user operations because of storage
access restrictions. However, I think it is technically possible to
accomplish at a UX cost.
> Specifically, imagine an acyclic Safe ownership tree, where we want to
execute a transaction with the root. The idea would be to pick a path to
a "leaf" Safe (that is, a Safe with only EOA owners). Since we assume an
acyclic Safe ownership tree, this must always be possible. The idea
would then be to have the "leaf" Safe sign a user operation to execute
parent.execTransaction(parent.execTransaction(...(root.execTransaction(payload))))
on its parent and make use of "approved hash from executor" signatures
(i.e. v = 1). All other Safes/owners sign regular SafeTransactions (i.e.
NOT 4337 user operation signature) for executing the root transaction.
> The problem with this scheme is that the UX is not great:
> - You only know what signatures are needed once you know the signers,
so signature collection is super messy (for example, imagine a 2 of 3
Safe where two owners are EOAs and one is a Safe, you can't know what
kind of signatures to collect without knowing who the signers will be).
There may be a way to work around this by expanding the Safe4337Module
to allow SafeUserOperation signatures to be used for calling
4337-enabled parent Safes, but I would need to think about it a bit
more...
> - The user operation would appears as if it was from the "leaf" Safe
in 4337-user-operation-explorers.
  • Loading branch information
mmv08 committed Mar 1, 2024
1 parent 4fbae2f commit bd7b756
Show file tree
Hide file tree
Showing 9 changed files with 593 additions and 39 deletions.
5 changes: 4 additions & 1 deletion modules/4337/contracts/test/SafeMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ contract SafeMock {
bytes32 s;
(v, r, s) = _signatureSplit(signature);
require(
owner == ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v, r, s),
// The original safe contract requires the V value to be > 30 for EIP-191 signed messages
// The canonical eth V value is 27 or 28, so by adding 4 to it we get a value > 30
// And when we verify the signature we subtract 4 from it
owner == ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s),
"Invalid signature"
);
}
Expand Down
114 changes: 90 additions & 24 deletions modules/4337/src/utils/execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,22 @@ export const EIP_DOMAIN = {
],
}

export const EIP712_SAFE_TX_TYPE = {
// "SafeTx(address to,uint256 value,bytes data,uint8 operation,uint256 safeTxGas,uint256 baseGas,uint256 gasPrice,address gasToken,address refundReceiver,uint256 nonce)"
SafeTx: [
{ type: 'address', name: 'to' },
{ type: 'uint256', name: 'value' },
{ type: 'bytes', name: 'data' },
{ type: 'uint8', name: 'operation' },
{ type: 'uint256', name: 'safeTxGas' },
{ type: 'uint256', name: 'baseGas' },
{ type: 'uint256', name: 'gasPrice' },
{ type: 'address', name: 'gasToken' },
{ type: 'address', name: 'refundReceiver' },
{ type: 'uint256', name: 'nonce' },
],
}

export const EIP712_SAFE_MESSAGE_TYPE = {
// "SafeMessage(bytes message)"
SafeMessage: [{ type: 'bytes', name: 'message' }],
Expand All @@ -28,44 +44,94 @@ export interface SafeTransaction extends MetaTransaction {
nonce: string | number
}

export interface SignedSafeTransaction extends SafeTransaction {
signatures: SafeSignature[]
}

export interface SafeSignature {
signer: string
data: string
// a flag to indicate if the signature is a contract signature and the data has to be appended to the dynamic part of signature bytes
dynamic?: true
}

export const calculateSafeDomainSeparator = (safeAddress: string, chainId: BigNumberish): string => {
return ethers.TypedDataEncoder.hashDomain({ verifyingContract: safeAddress, chainId })
}

export const preimageSafeTransactionHash = (safeAddress: string, safeTx: SafeTransaction, chainId: BigNumberish): string => {
return ethers.TypedDataEncoder.encode({ verifyingContract: safeAddress, chainId }, EIP712_SAFE_TX_TYPE, safeTx)
}

export const calculateSafeTransactionHash = (safeAddress: string, safeTx: SafeTransaction, chainId: BigNumberish): string => {
return ethers.TypedDataEncoder.hash({ verifyingContract: safeAddress, chainId }, EIP712_SAFE_TX_TYPE, safeTx)
}

export const preimageSafeMessageHash = (safeAddress: string, message: string, chainId: BigNumberish): string => {
return ethers.TypedDataEncoder.encode({ verifyingContract: safeAddress, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message })
}

export const calculateSafeMessageHash = (safeAddress: string, message: string, chainId: BigNumberish): string => {
return ethers.TypedDataEncoder.hash({ verifyingContract: safeAddress, chainId }, EIP712_SAFE_MESSAGE_TYPE, { message })
}

export const signHash = async (signer: Signer, hash: string): Promise<SafeSignature> => {
const typedDataHash = ethers.getBytes(hash)
const signerAddress = await signer.getAddress()
const signature = await signer.signMessage(typedDataHash)

return {
signer: await signer.getAddress(),
data: await signer.signMessage(typedDataHash),
signer: signerAddress,
data: signature.replace(/1b$/, '1f').replace(/1c$/, '20'),
}
}

const sortSignatures = (signatures: SafeSignature[]) => {
signatures.sort((left, right) => left.signer.toLowerCase().localeCompare(right.signer.toLowerCase()))
export const getPrevalidatedSignature = (signerAddress: string): SafeSignature => {
return {
signer: signerAddress,
data: '0x000000000000000000000000' + signerAddress.slice(2) + '0000000000000000000000000000000000000000000000000000000000000000' + '01',
}
}

export const buildContractSignature = (signerAddress: string, signature: string): SafeSignature => {
return {
signer: signerAddress,
data: signature,
dynamic: true,
}
}

export const buildSignatureBytes = (signatures: SafeSignature[]): string => {
sortSignatures(signatures)
return ethers.concat(signatures.map((signature) => signature.data))
}

export const buildContractSignatureBytes = (signatures: SafeSignature[]): string => {
sortSignatures(signatures)
const start = 65 * signatures.length
const { segments } = signatures.reduce(
({ segments, offset }, { signer, data }) => {
return {
segments: [...segments, ethers.solidityPacked(['uint256', 'uint256', 'uint8'], [signer, start + offset, 0])],
offset: offset + 32 + ethers.dataLength(data),
}
},
{ segments: [] as string[], offset: 0 },
)
return ethers.concat([
...segments,
...signatures.map(({ data }) => ethers.solidityPacked(['uint256', 'bytes'], [ethers.dataLength(data), data])),
])
const SIGNATURE_LENGTH_BYTES = 65
signatures.sort((left, right) => left.signer.toLowerCase().localeCompare(right.signer.toLowerCase()))

let signatureBytes = '0x'
let dynamicBytes = ''
for (const sig of signatures) {
if (sig.dynamic) {
/*
A contract signature has a static part of 65 bytes and the dynamic part that needs to be appended
at the end of signature bytes.
The signature format is
Signature type == 0
Constant part: 65 bytes
{32-bytes signature verifier}{32-bytes dynamic data position}{1-byte signature type}
Dynamic part (solidity bytes): 32 bytes + signature data length
{32-bytes signature length}{bytes signature data}
*/
const dynamicPartPosition = (signatures.length * SIGNATURE_LENGTH_BYTES + dynamicBytes.length / 2).toString(16).padStart(64, '0')
const dynamicPartLength = (sig.data.slice(2).length / 2).toString(16).padStart(64, '0')
const staticSignature = `${sig.signer.slice(2).padStart(64, '0')}${dynamicPartPosition}00`
const dynamicPartWithLength = `${dynamicPartLength}${sig.data.slice(2)}`

signatureBytes += staticSignature
dynamicBytes += dynamicPartWithLength
} else {
signatureBytes += sig.data.slice(2)
}
}

return signatureBytes + dynamicBytes
}

export const logGas = async (message: string, tx: Promise<TransactionResponse>, skip?: boolean): Promise<TransactionResponse> => {
Expand Down
61 changes: 57 additions & 4 deletions modules/4337/src/utils/safe.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { AddressLike, JsonRpcProvider, Provider, Signer, ethers } from 'ethers'

// Import from Safe contracts repo once fixed
import { MetaTransaction, SafeSignature, buildSignatureBytes } from './execution'
// Import from Safe contracts repo once it is upgraded to ethers v6 and can be installed via npm
import { MetaTransaction, SafeSignature, SignedSafeTransaction, buildSignatureBytes } from './execution'
import { UserOperation, EIP712_SAFE_OPERATION_TYPE } from './userOp'

const AddressOne = '0x0000000000000000000000000000000000000001'
Expand All @@ -13,6 +13,7 @@ const INTERFACES = new ethers.Interface([
'function proxyCreationCode() returns (bytes)',
'function enableModules(address[])',
'function execTransactionFromModule(address to, uint256 value, bytes calldata data, uint8 operation) external payable returns (bool success)',
'function execTransaction(address to, uint256 value, bytes calldata data, uint8 operation, uint256 safeTxGas, uint256 baseGas, uint256 gasPrice, address gasToken, address payable refundReceiver, bytes memory signatures) external payable returns (bool success)',
'function executeUserOp(address to, uint256 value, bytes calldata data, uint8 operation)',
'function getNonce(address,uint192) returns (uint256 nonce)',
'function supportedEntryPoint() returns (address)',
Expand Down Expand Up @@ -326,17 +327,69 @@ export class Safe4337 {
return Safe4337Operation.build(this.provider, this, action, this.globalConfig)
}

static async withSigner(signer: string, globalConfig: GlobalConfig): Promise<Safe4337> {
/**
* Creates a new instance of Safe4337 with the specified signer and global configuration.
*
* @param {string} signer - A signer address.
* @param {GlobalConfig} globalConfig - The global configuration for the Safe4337 instance.
* @returns {Safe4337} A new instance of Safe4337.
*/
static withSigner(signer: string, globalConfig: GlobalConfig): Safe4337 {
const safeConfig: SafeConfig = {
signers: [signer],
threshold: 1,
nonce: 0,
}

return Safe4337.withConfigs(safeConfig, globalConfig)
}

static async withConfigs(safeConfig: SafeConfig, globalConfig: GlobalConfig): Promise<Safe4337> {
/**
* Creates a new instance of Safe4337 with the specified signers, threshold, and global configuration.
*
* @param {string[]} signers - An array of signer addresses.
* @param {number} threshold - The number of signatures required to execute a transaction.
* @param {GlobalConfig} globalConfig - The global configuration for the Safe4337 instance.
* @returns {Safe4337} A new instance of Safe4337.
*/
static withSigners(signers: string[], threshold: number, globalConfig: GlobalConfig): Safe4337 {
const safeConfig: SafeConfig = {
signers,
threshold,
nonce: 0,
}

return Safe4337.withConfigs(safeConfig, globalConfig)
}

/**
* Creates a new instance of Safe4337 with the provided SafeConfig and GlobalConfig.
* @param {SafeConfig} safeConfig - The SafeConfig object containing the configuration for the Safe4337 instance.
* @param {GlobalConfig} globalConfig - The GlobalConfig object containing the global configuration for the Safe4337 instance.
* @returns {Safe4337} A new instance of Safe4337.
*/
static withConfigs(safeConfig: SafeConfig, globalConfig: GlobalConfig): Safe4337 {
const initParams = buildInitParamsForConfig(safeConfig, globalConfig)
return new Safe4337(initParams.safeAddress, globalConfig, safeConfig)
}

/**
* Encodes the transaction data for executing a safe transaction.
* @param {SignedSafeTransaction} transaction - The signed safe transaction object.
* @returns {string} The encoded transaction data.
*/
static getExecTransactionData(transaction: SignedSafeTransaction): string {
return INTERFACES.encodeFunctionData('execTransaction', [
transaction.to,
transaction.value,
transaction.data,
transaction.operation,
transaction.safeTxGas,
transaction.baseGas,
transaction.gasPrice,
transaction.gasToken,
transaction.refundReceiver,
buildSignatureBytes(transaction.signatures),
])
}
}
Loading

0 comments on commit bd7b756

Please sign in to comment.