Closed akshay-ap closed 1 month ago
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:
Cons:
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
Cons
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:
Cons:
Personal opinion: I would prefer approach 1. It is also backward compatible with existing Safe contracts.
If we want to support module guards with external context for only specific modules, we can implement the guard in such a way that:
ISpecializedGuard
interfaceConcretely, what this looks like for the 4337 module would be:
Safe4337Module
contract.IUserOpGuard
interfacecheckUserOp(...)
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.
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.
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?
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.
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.
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.
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
functionThis follows that the
additionalData
should be received fromexecTransactionFromModule*
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).