Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add account specifications #57

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Add account specifications #57

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e186ad2
Add account specifications
mmv08 Oct 19, 2023
b1c35fe
Update accounts/README.md
mmv08 Oct 25, 2023
f5eca39
Adjust signature check function
mmv08 Oct 25, 2023
2e6ebf1
Merge branch 'specs/account-specs' of github.com:safe-global/safe-cor…
mmv08 Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 114 additions & 3 deletions accounts/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,121 @@
# Accounts

The Safe{Core} Protocol is designed to be **account agnostic**. This initial alpha version sets a focus on the 1.x versions of Safe Smart Accounts to expedite the development process and gather feedback. These learnings are the foundation upon which the protocol is opened up to other account implementations.
The Safe{Core} Protocol is designed to be **account agnostic**. This initial alpha version focuses on the 1.x versions of Safe Smart Accounts to expedite the development process and gather feedback. These learnings are the foundation upon which the protocol is opened to other account implementations.

## Safe 1.4.0
## High-level overview

[Source Code](https://github.com/safe-global/safe-contracts/tree/v1.4.0)
The [Safe{Core} Protocol Manager](https://github.com/safe-global/safe-core-protocol-specs/blob/2bffd759dd12be5583594f302d97c35e0ab9fcf5/manager/README.md) contract is the main entity interacting with accounts and vice versa. The Manager expects accounts to implement a specific interface, described in this document.

## API Specification

### General requirements

The Account MUST append the 20 bytes of the `msg.sender` address to the calldata to each Account configuration-related method (setting a hook,
adding a plugin, etc., for the full list, refer to [Manager's](../manager/README.md) specification). This is required because
nlordell marked this conversation as resolved.
Show resolved Hide resolved
to support some functionality, such as function handlers, an Account may be required to execute a CALL
operation that potentially bypasses the authorization to the Manager contract. This is required to allow the Manager to identify the transaction's sender. It is assumed
that the only way to have the `msg.sender` equal to the Account address in the call frame is to execute a call through the Account's built-in authorization scheme.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👌.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


### Expected interface per Safe{Core} Protocol Module

#### Plugins

The account MUST implement the following interface:

```Solidity
interface IAccount {
function execTransactionFromModuleReturnData(
address to,
uint256 value,
bytes memory data,
uint8 operation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

) external returns (bool success, bytes memory returnData);
}
```

where:

| Type | Name | Description |
|--------------|-----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| address | to | The address of the contract to be called |
| uint256 | value | The amount of Native Token to be sent |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😂

| bytes memory | data | The call data |
| uint8 | operation | The operation type to be executed. `0` for CALL, `1` for DELEGATECALL. The DELEGATECALL operation can only be executed by a Plugin with root access permissions. More information about transaction types can be found in the [Manager contract](https://github.com/safe-global/safe-core-protocol-specs/blob/2bffd759dd12be5583594f302d97c35e0ab9fcf5/manager/README.md) specifications. |
mmv08 marked this conversation as resolved.
Show resolved Hide resolved

The account MUST execute a corresponding `operation` to the `to` address with the provided `value` and `data` parameters. The account MUST return a tuple of `(bool success, bytes memory returnData)`.
It is RECOMMENDED that the account supports the DELEGATECALL operation.

#### Hooks

Hooks do not require any specific interface to be implemented by the account. To use hooks, use the interface defined in the
[Safe{Core} Protocol Manager specification](../manager/README.md)

#### Function Handlers

The account MUST implement a `fallback` function, which forwards all the calldata
to the Safe{Core} Protocol Manager AND appends the 20 bytes of the `msg.sender` address to the calldata.

An example implementation of such a `fallback` function is:
```Solidity
fallback() external {
bytes32 slot = SAFE_CORE_PROTOCOL_MANAGER_SLOT;
/// @solidity memory-safe-assembly
assembly {
function allocate(length) -> pos {
pos := mload(0x40)
mstore(0x40, add(pos, length))
}
nlordell marked this conversation as resolved.
Show resolved Hide resolved

let protocol := sload(slot)
if iszero(protocol) {
return(0, 0)
}

let calldataPtr := allocate(calldatasize())
calldatacopy(calldataPtr, 0, calldatasize())

// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
let senderPtr := allocate(20)
mstore(senderPtr, shl(96, caller()))

// Add 20 bytes for the address appended add the end
let success := call(gas(), protocol, 0, calldataPtr, add(calldatasize(), 20), 0, 0)

let returnDataPtr := allocate(returndatasize())
returndatacopy(returnDataPtr, 0, returndatasize())
if iszero(success) {
revert(returnDataPtr, returndatasize())
}
return(returnDataPtr, returndatasize())
}
}
```

#### Signature Validators

1. The account MUST follow the requirements defined in the Function Handlers section, or the account MUST forward the EIP-1271 isValidSignature call to the Safe{Core} Validator Manager, following the expected encoding defined in the [Signature Validator specs](../modules/README.md).
2. The account MUST implement the following interface:

```Solidity
interface IAccount {
function checkSignatures(bytes32 messageHash, bytes memory messageData, bytes memory signatures) external view;
}
```
where:
- `checkSignatures` function parameters are:

| Type | Name | Description |
|--------------|-------------|------------------------|
| bytes32 | messageHash | Hash of the message |
| bytes memory | messageData | EIP-712 pre-image data |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| bytes memory | signatures | Signature bytes |

The function MUST revert if the signatures are not valid.

## Reference implementations

- [Safe{Core} Account](https://github.com/safe-global/safe-contracts) - starting from version 1.3.0

<img src="../_assets/accounts_safe_140.png" width=400/>

Expand Down
Loading