Evaluate Providing Additional Context Data to Module Guards

[akshay-ap]

Context / issue

While fixing #754, we discovered that Module guard does not get any additional information/context.
Current module guard interface.

If there was a way to pass additional data to the module guard, it would be helpful in addressing #754 where additionalData could contain signatures of co-signers for module tx which is verified in module guard.

This issue is to discuss the possible ways to pass additional data to the module guard, possible other use cases for it and potentially making it a part of Safe v1.5.0.

Proposed solution

Proposed change in checkModuleTransaction function

function checkModuleTransaction(
    address to,
    uint256 value,
    bytes memory data,
    Enum.Operation operation,
    address module
+   bytes memory additionalData
) external returns (bytes32 moduleTxHash);

This follows that the additionalData should be received from execTransactionFromModule* functions and introduces a breaking change.

Alternatives

There are other potential alternatives that need to be explored as part of this issue.

Expected Outcome

The expected outcome of to evaluate how we want to pass additional context to module guards. We should attempt to explore other alternatives and provide some details to the pros/cons of each approach (for example, this approach would require smart contract changes to existing modules in order to make use of this feature).

[akshay-ap]

Approach 1: Modify Safe Contract and module guard interface to include context field

This approach involves adding a context parameter to the module guard's checkModuleTransaction function and introducing two new overloaded functions in the Safe contract that accept context as a parameter.

POC here:
https://github.com/safe-global/safe-smart-account/compare/feature-context-in-module-tx?expand=1

Pros:

• The module guard interface introduced in v1.5.0 has not yet been released, so it is safe to assume that these changes won't force any redeployment.
• Changes in Safe contract are backward compatible.

Cons:

• Increases the size of Safe contract but still within the 24kb limit.
• To use this feature modules have to adapt their implementation.

Approach 2: Set context information in Guard before execution of module transaction.

In this approach, the module guard provides a method to set context information. The module guard stores the context information in its storage (can also use transient storage). The module guard reads the context information from its storage in checkModuleTransaction function.

Safe account stores the context in Guard using setContext(bytes32 moduleTxHash, bytes calldata context) whenever required before executing the module transaction. This can be achieved by using multisend or a separate transaction. Module guard has to implement additional logic to permit calls that are to itself and method signature is bytes4(keccak256(setContext(bytes32,bytes))).

Pro

• No Change in Safe contract.

Cons

• Module guard has to implement additional logic to store context information.
• Additional gas costs associated with storing/reading context info in module guard.
• A call to guard must be made before the transaction.

Approach 3: Module stores context information in its own storage. Module guard reads context info from module storage.

In this approach, it is up to the module to store the context information for the module transaction and make it available to the guard when requested. The module guard reads the context from the module storage before executing the module transaction.

interface IModule {
 function getContext(bytes32 moduleTxHash) external view returns (bytes memory);
}

Guard implementation:

...
 function checkModuleTransaction(....){
 bytes memory context = IModule(module).getContext(moduleTxHash);
 // check context
 ...
 }
...

Pros:

• No change in Safe contract

Cons:

• Requires module to implement additional interface to provide context information to guard.
• Additional gas costs associated with storing/reading context from module storage.
• Module and Guard need to have an agreement on a protocol (to generate nonce, replay protection)

Personal opinion: I would prefer approach 1. It is also backward compatible with existing Safe contracts.

[nlordell]

If we want to support module guards with external context for only specific modules, we can implement the guard in such a way that:

• Use Option 2 from Akshay's proposal above
• Modify the guard so that it has exceptions to the specific modules and a specific ISpecializedGuard interface
• Have the modules call that guard interface.

Concretely, what this looks like for the 4337 module would be:

• A negative control guard has an exception to the Safe4337Module contract.
• The negative control guard additionally implements IUserOpGuard interface
• The Safe 4337 Module calls checkUserOp(...) on the guard if installed on the Safe during user op validation.

This requires changes to the Safe 4337 module (which is true for all approaches above) but does not require any changes to the core smart contracts.

[remedcu]

For Approach 2:

Cons

Module guard has to implement additional logic to store context information.
Additional gas costs associated with storing/reading context info in module guard.
A call to guard must be made before the transaction.

If we use MultiSend to store the context in a slot in Tx 1 and then remove the context in the after checks of Module Tx, then that would result in the least amount of gas used due to gas refund, also making it a single transaction.

[mmv08]

To be frank, I'm still trying to understand why this feature is needed. We figured out that passing an additional bytes array for the guard to check can be avoided by implementing the check directly in the module itself.

I looked up the safe-core-protocol-specs, and the execution metadata was also a "TODO:" there, meaning to me that there were not enough product requirements to draw a specification out of them.

From the example above:

 function checkModuleTransaction(....){
 bytes memory context = IModule(module).getContext(moduleTxHash);
 // check context
 ...
 }

Given that we have a single guard for all the modules, the encoding of this bytes array needs to be heavily standardized and IMO, because it's a "loose" data structure, an incorrect encoding/decoding implementation might break the module execution flow, requiring higher standards for developers and auditors. Imagine if you add a new guard that is incompatible with the encoding of the existing modules.

Did we ask any teams (Zodiac, for example) whether they'd like to see this feature?

[akshay-ap]

To be frank, I'm still trying to understand why this feature is needed.

The aim of this feature is to enable use cases where additional info is helpful for transaction verification. e.g., co-signer signature verification when executing module tx for negative control on a Safe.
But, this is the only specific use case that we have encountered so far that need context in the Module Guard.

Though this feature can be implemented inside a module, for the specific use case of negative control on Safe, every module has to implement this verification logic.

Did we ask any teams (Zodiac, for example) whether they'd like to see this feature?

No. This feature request was brought up internally.

[mmv08]

Though this feature can be implemented inside a module, for the specific use case of negative control on Safe, every module has to implement this verification logic.

I personally deem this as a feature particular to the negative control use case, and that doesn't justify a change in core functionality to me. They can use a shared library contract if they need to share a certain co-signer verification logic.

The requirement of interoperability with other modules doesn't help here.

[akshay-ap]

After discussion with the team we conclude the following:

Since the context is specific to each use case and may require different interpretations, and there is already a method to add context information using module guard as a multisend, we agree not to modify the Safe contract or introduce any new protocol that enforces Guard or Module to implement any specific interface. Also, there are alternatives like using ERC7579 standard with Safe to achieve this functionality.