Add account specifications

[mmv08]

This PR:

• Implements [Protocol Manager] Define requirements for Safe{Core} Protocol accounts #34
• This is the first draft of the account specifications. While defining this, I was assuming that the account might choose to use only a specific type of module and not necessarily have to support all of them
• I followed https://www.ietf.org/rfc/rfc2119.txt for keywords to define requirement levels
• I also identified many missing pieces while working on this:

1. [Protocol Specs] There is no straightforward details on how to use the SignatureValidatorManager with the protocol #55
2. [Protocol Specs] Missing detailed spec for function handlers #53
3. [Protocol Specs] Missing specs for hooks usage #52
4. [Protocol Specs] The specs are missing the expected encoding for signature validators #56

[nlordell]

Is this always available? I'm asking because I feel like I've seen calls to this function with messageData = "" and can imagine scenarios where this isn't available (for example, when verifying EIP-1271 signatures for example with the default verifier).

[mmv08]

So the messageData variable has a legacy background because the older EIP-1271 spec used bytes instead of bytes32 and the data hash. If it's not always available, I'd explicitly state that the account must not expect this argument but keep the signature consistent

[nlordell]

Looking through the checkSignatures code, it looks like older versions of the Safe used this for the old EIP-1271 spec for contract signatures. Seeing as, since Safe v1.5.0 this is no longer the case (it uses the the new EIP-1271 signature verification for all contract signatures), could we just remove data from the interface?

If the old checkSignatures function is kept around for backwards compatibility on the Safe, that is fine, but I don't think it should be needed for the IAccount interface. Essentially, you can add the following methods to the Safe so keep a backwards compatible interface, without adding the data "tech debt" to the new IAccount interface:

function checkSignatures(bytes32 dataHash, bytes memory /* data */, bytes memory signatures) public view {
    checkSignatures(dataHash, signatures);
}

function checkNSignatures(
    address executor,
    bytes32 dataHash,
    bytes memory /* data */,
    bytes memory signatures,
    uint256 requiredSignatures
) public view {
    checkNSignatures(executor, dataHash, signatures, requiredSignatures);
}

This can even be implemented as a plugin to not take precious code space from the Safe contract.

[nlordell]

Is adding msg.sender completely covered by the reference fallback implementation?

The reason I ask is that I would propose not having this requirement:

1. If the Account implementation is expected to internally make configuration calls like adding a plugin, then I would change the Manager.addPlugin implementation to take an explicit caller parameter instead of having this "rule"
2. If the Account calls configuration function through its fallback handler, then this is already specified through the reference fallback implementation right?

Sorry for being nit-picky but this paragraph "felt a bit off" to me and it's the spec 😅 which needs to be 👌.

[mmv08]

Is adding msg.sender completely covered by the reference fallback implementation?

If the account implements the specification required for the function handlers, it is covered. But it is a requirement for all state-changing methods.

It is coming as an after-effect of having function handlers. So we have the Manager set as a fallback handler, right? That means an arbitrary sender can call into the account and execute an arbitrary CALL to the Manager. Because of this, we had to add this requirement for the Manager to authenticate the caller.

I'm also not highly fond of this requirement, but I do not see a possible workaround. I'm happy to iterate on the specs to make it more clear.

If the Account implementation is expected to internally make configuration calls like adding a plugin, then I would change the Manager.addPlugin implementation to take an explicit caller parameter instead of having this "rule"

Then the sender can specify an arbitrary caller, can't they?

[nlordell]

That means an arbitrary sender can call into the account and execute an arbitrary CALL to the Manager. Because of this, we had to add this requirement for the Manager to authenticate the caller.

I don't think this is a form of authentication in the Manager side though - a malicious account implementation or caller can just as easily add a different address at the end of the Manager call.

My point was more about questioning "why is this requirement needed". If the configuration change methods are always called via the fallback mechanism, then this is a non-issue correct (as in, this requirement doesn't need to be mentioned as it is captured by the fallback mechanism specification)?

Is my understanding correct that if the Account has code where it internally makes configuration changes, then it needs to either:

• IManager(this).configurationMethod() (to ensure that it goes over the fallback mechanism, at the cost of an additional CALL, which I think should be fine in the name of simplicity)
• Manually add the msg.sender at the end of the call data, in which case it likely makes sense to expose methods for configuration in the form of a library:

library ManagerLib {
    function addPluginInternal(IManager manager, ...) {
        bytes memory callData = abi.encodeCall(manager.addPluginInternal, (...));
        manager.call(callData);
    }
}

contract Account is IAccount {
    using ManagerLib for IManager;
    
    IManager manager;
    
    function doSomeConfiguration() {
        manager.addPluginInternal(...);
    }
}

And in this context, this requirement is meant to capture the requirement for the second option right?

[nlordell]

@rmeissner - do you have context on the choice of using uint8 instead of an enum for the operation?

I think that they are equivalent from an ABI standpoint, but an enum might be more self-documenting.

[nlordell]

I wouldn't capitalise "native token" here.

Also, something that is not obvious (and related to EVM semantics) is that this value can't be specified when doing a DELEGATECALL (since you don't send values with DELEGATECALL and it inherits the current transaction value).

Actually, thinking about it, how do you do a DELEGATECALL with a value under 4337? I assume it that you would have to make the Safe call itself with a value to do the actual DELEGATECALL you want?

[mmv08]

Also, something that is not obvious (and related to EVM semantics) is that this value can't be specified when doing a DELEGATECALL (since you don't send values with DELEGATECALL and it inherits the current transaction value).

I specified that the value would be ignored if the operation is delegatecall.

Actually, thinking about it, how do you do a DELEGATECALL with a value under 4337? I assume it that you would have to make the Safe call itself with a value to do the actual DELEGATECALL you want?

🤯 Something like this, yeah 😂