Agent app, arbitrary actions from DAOs

Builds on the discussions in this thread Dynamic Permissions for Organization "Actions" with Signer Integration, creating a new topic to focus the conversation on this new spec.

WIP implementation: https://github.com/aragon/aragon-apps/pull/580

Aragon Agent app

The Agent app is a superset of the Vault app. It can hold valuable assets (ETH and ERC20) and perform external actions. In many instances the Agent app will be the external interface of the DAO, as it can perform actions on her behalf.

The Agent app may end up replacing the Vault app as the default app where assets are held in a DAO. However, a more conservative rollout is proposed having both apps available and allow DAOs to migrate whenever they decide to do so.

The Agent app builds on top jvluso’s Identity app and generalizes it to fit more use cases.

User stories

  • An Aragon DAO can interact with other Ethereum smart contracts or protocols without the need of implementing a custom Aragon app for every protocol.
  • A user/member of a DAO can use any Ethereum dApp identified as their DAO, signer integrations can take care of routing the intent through the governance process of the DAO.
  • An Aragon DAO can participate as a stakeholder in another DAO, allowing the creation of DAO stacks.

Contract specification v1

1. Vault inheritance (Agent is Vault)

See Vault v4 implementation.

Functions:

  • transfer : moves tokens out of the Actor app. Protected by TRANSFER_TOKENS_ROLE
  • deposit : pulls tokens from msg.sender to the Actor app.

2. Arbitrary call execution

Executes an arbitrary call from the Agent app to a user inputed address.

Signature:

function execute(address target, uint256 ethValue, bytes data) 
	authP(EXECUTE_ROLE, arr(target, ethValue, extractSignature(data)))
	external

Functionality

  • Perform an EVM call sending all available gas to target, sending the specified ETH amount and calldata.
  • If the call reverts, revert forwarding the error data as the error data of the main call frame.
  • If the call succeeds, emit an event logging the arguments the function was called with.

Security

It would be extremely cumbersome to ensure that ‘vanilla’ ETH and ERC20 transfers cannot happen, as they could be masqueraded in many ways (e.g. create a contract that receives ETH regardless of the call data, or send tokens with approve + transferFrom)

The EXECUTE_ROLE should be treated as a super-role of TRANSFER_TOKENS_ROLE, as someone with the role would be able to transfer tokens and also perform additional actions (unless extremely well protected with ACL params).

However we could have the following check, mostly as a sanity check:

  • If ethValue is positive: data must be non-empty and target code size be greater than 0. For vanilla ETH transfers, the transfer function should be used.

3. Forwarding interface (Agent is IForwarder)

Making the Agent app a fully-fledged forwarder will ease inter-DAO interactions (DAOs acting in other DAOs) with inter-DAO transaction pathing™️, as well as allow EVMScript execution for the ease of executing more complex actions in one call (even though the Agent will probably be called from a script in most cases).

Executing EVMScripts with the Agent app should require holding the RUN_SCRIPT_ROLE role, which can be parametrized with the keccak256 hash of the script.

The reasons for supporting arbitrary call executions too and not only pure script execution are:

  • ACL parametrization will be less powerful when executing scripts, as script inspection is virtually impossible.
  • No existing EVMScript executor supports sending ETH with calls.

Note that granting the RUN_SCRIPT_ROLE is virtually like granting TRANSFER_TOKENS_ROLE but without the possibility of parametrizing permissions, therefore it should be more restricted.

4. Signature handling

Smart contracts addresses don’t derive from private keys, therefore it is impossible for a contract to do an ECDSA signature. However, there are protocols in which users authorize actions using signatures (e.g. making an order in 0x). As the ecosystem moves forward with account abstraction, and the assumption that everyone uses a EOA to interact with contract dies, we can push for a standard way for contracts to ‘sign messages’.

A standard for contracts ‘signing messages’ (already live in 0x v2), is for the contract to expose a isValidSignature function that gets called for verifying whether the contract approves a given signature as its own:

function isValidSignature(bytes32 hash, bytes sig) public view returns (bool)

Note that the function is a view and shouldn’t modify state. We should assume it is always executed with a staticcall.

There are two routes that the Agent app could return true to a isValidSignature call, both of which can (and should) co-exist:

4.1 Designated signer

Protected by the DESIGNATE_SIGNER_ROLE one designated signed for the Agent app can be set.

Function signature:

function setDesignatedSigner(address designatedSigner) 
	authP(DESIGNATE_SIGNER_ROLE, arr(designatedSigner))
	external

The designated signer should replace the current designated signer in the contract state and emit an event.

The response to isValidSignature depends on the nature of the designated signer:

4.1.0 Designated signer is not set

Return false unless the hash has been pre-signed (see section 4.2)

4.1.1 Designated signer is an EOA

Checks whether the signature is a valid signature of the hash by the designated signer.

  • Extract signature components from the data byte array.
  • If ecrecover(hash, sig[64], sig[0:31], sig[32:63]) equals to the designated signer address, return true
  • Otherwise, return false unless the hash has been pre-signed (see section 4.2)

4.1.2 Designated signer is a contract

Forwards the signature checking to the designated signer. A contract designated signer may implement a different signing algorithm (e.g. the designated signer may check a ring signature).

  • Perform a staticcall (designatedSigner.isValidSignature(hash, sig)).
  • If it returns 32 bytes of data equal representing a 1, return true
  • If the call reverts or returns false, return false unless the hash has been pre-signed (see section 4.2)

4.2 Pre-signed hashes

Protected with the PRESIGN_HASH_ROLE, this function allows to mark a hash as presigned, and therefore make the isValidSignature function always return true for that hash.

Function signature:

function presignHash(bytes32 hash) 
	authP(PRESIGN_HASH_ROLE, arr(hash))
	external

The hash will be marked as signed in the contract storage, and once marked as signed it should never be reverted as not signed (in the same way that you cannot revoke an ECDSA signature).

An optimized implementation should always check first if the hash had been presigned before checking if the hash is properly signed by the designated signer.

7 Likes

This is great. I have a few comments:

2.3 For preventing the EXECUTE_ROLE from being able to move tokens, would it be possible for use to have a EXECUTE_TO_ROLE which would be a whitelist (or blacklist) of contracts that transactions can be forwarded to?

4.1.2 Can we extract information from the signature to determine the contract to check isValidSignature?

4.2 Do EXECUTE_ROLE, PRESIGN_HASH_ROLE, and DESIGNATE_SIGNER_ROLE need to be separate if we can specify the signer in the signature?

This can be achieved with ACL parametrization, as permissions can be issued with restrictions on what the to address is. But it wouldn’t be as flexible or easy to use as an in-contract list. Thinking about it.

As it is right now, we can’t. Philippe pointed out to me on Twitter that there is a discussion about sending the data that is being checked rather than its hash:

Will be reviewing the ERC-1271 standard and making a decision on whether to support what 0x is using, ERC-1271, or both.

I can’t fully follow, can you elaborate?

Just to be clear, 1271 is absolutely not finalized and has seen little discussion. Hence, the standard could be compatible with 0x’s implementation without issues. I suspect the main reason is because not many projects yet see the appeal of using a method similar to isValidSignature(). I would happily add you (or anyone else for the matter) as an author if you contribute to the discussion on 1271 to ensure maximum support within the community.

I believe standardizing this aspect is quite important for the next year or so as more and more contracts will hold funds and be managed by various validation schemes like yours.

4 Likes

This would only matter if isValidSignature could specify the preceding contract. If it can, then we can make the permission to sign transactions be the same as the permission to pre-sign a transaction. If not, we would have to have only one designated signer to validate a pre-signed transaction.

1 Like

Totally agree on the importance of the ERC. Will definitely be taking a deeper look and posting my thoughts. The good thing about this app being upgradeable is that even if the standard is not finalized people can deploy the app with the current spec and if a different spec becomes popular it can be upgraded

3 Likes

TL;DR: Looks good, can’t come up with a different design that would simplify the permissioning requirements.


I’ve been thinking for a while about how to logically separate the Actor from the Vault. I haven’t been able to come up with a satisfying approach, but wanted to leave some thoughts.

From both a security and intuitive standpoint, it initially felt wrong to approach the Actor app as what may come to be the default vault of an organization. This may be a misconception in how I’ve viewed the Actor app up to now, as I likened it to an “actor/agent of an organization” rather than the “full might” of an organization itself. Typically, an organization will, in aggregate, hold a number of assets, but actors in that organization are given access to only a very limited subset. However, the organization itself can act as a single entity, in full consideration of all of its assets.

The former seems to be mimic’d well with roles and their parameterization on particular addresses (agents), and the latter requires some way for an organization’s vault to interact with other contracts due to how accounting and authentication are typically implemented, as described here.


Originally, I was thinking of relating the Vault to the Actor app with composition instead of inheritance. This, in particular, could avoid the complex, risk-escalating nature of the different roles.

The main security risk with the current design is someone being able to maliciously transfer tokens. So I thought, “wouldn’t it be great if only the Vault could transfer out, and the Actor could only transfer tokens back to the Vault? What if the Actor app could only invoke functions (some blacklisted, e.g. token transfers), and ‘vanilla’ transfers could only come from the Vault?”

But then we have a problem with things like staking, the malicious masquerading of “vanilla” transfers as you pointed out, or accounting and authentication issues depending on the design of the other contract since it wouldn’t be the Actor app itself sending tokens.


Rather than stating capabilities (e.g. “it can hold valuable assets”), it might be nice to flesh out a number of requirements needed to satisfy the user stories. The most important, security-wise, is how this app holds and transfers tokens to allow it to properly interact with other smart contracts or protocols:

  • It’s important for the Actor app to hold any tokens with snapshotting requirements, to correctly interact with the token protocol
  • It’s important for the Actor app to transfer ETH or tokens directly from itself to another contract, to satisfy accounting and authentication rules in most contract designs
2 Likes

Yes, this looks really good! It is very exciting. For instance, retaking the idea of personal DAOs, which I love and which I did right after mainnet launch, I feel this is filling an important gap for it to be really useful. Of course there are tons of other use cases, and probably more important, but I particularly love this one :wink:
Some minor comments:

  • Does TRANSFER_TOKENS_ROLE mean TRANSFER_ROLE from Vault app or is it a new one?
  • About the RUN_SCRIPT_ROLE:

which can be parametrized with the keccak256 hash of the script.

What would be the idea? Black or whitelisting scripts? I wonder how useful that would be, as the list in both cases could be quite big.

  • I completely agree about the importance of standardization for something like this.
  • About this (from last @sohkai reply) :

it initially felt wrong to approach the Actor app as what may come to be the default vault of an organization
(…)
Typically, an organization will, in aggregate, hold a number of assets, but actors in that organization are given access to only a very limited subset

I guess this will always be optional, in the sense that a DAO could decide to have an instance of a plain Vault, plus an instance of an Actor app (or more than one) for certain interactions.
So I don’t think these 2 ideas are mutually exclusive.

  • About this (from last @sohkai reply):

It’s important for the Actor app to hold any tokens with snapshotting requirements

Not sure if I misunderstood, but I would say that snapshotting would be out of the scope of the Actor app and would belong in the Staking app or somewhere else.

Another comment is that I feel that having both the execute function plus the Aragon forwarding mechanism with EVMScript stuff is kind of redundant.
The differences and the reasons are well explained and I see the advantages of each one. Besides I don’t know right now how to avoid this duplication so not suggesting any change actually, but still… Will try to give it another thought.

WIP implementation: https://github.com/aragon/aragon-apps/pull/580

2 Likes

@PhABC Regarding signature validation, I have opted for implementing both isValidSignature(bytes32 hash, bytes sig) and isValidSignature(bytes data, bytes sig) (just performs isValidSignature(keccak256(data), sig)), but when the Actor app has a designated signer that is also a contract, it uses the isValidSignature(bytes32 hash, bytes sig).

I have published a deeper version of my reasoning in the ERC-1271 discussion.

I made a modified version of the actor app you made with the recommendations I made above at https://github.com/jvluso/aragon-apps/blob/actor-app/future-apps/actor/contracts/Actor.sol . To summarize - I don’t think execute does anything that forward doesn’t. I think we can use one permission for everything related to signing arbitrary transactions, rather than 3.

Awesome @jvluso, so much easier to get the idea by reading the code.

The only really important thing it adds is the ability for us to get the target address and the signature of the function that will be called, which can be passed as parameters for the permission (so conditional permissions can be created depending on what address the actor app calls with or what is the function executed). Just having script execution would require inspecting scripts in order to have conditional permissions, which would be a bad idea (since the script format varies depending on the executor).

I’m actually not really sure about whether this app needs to have the ability to execute scripts at all (or we can keep it but the permission should be disabled by default). In most cases, another app (i.e. Voting) will be forwarding a script to the Actor app so there is no need for both apps to have the ability to execute scripts.

We can think about merging the permissions related to signatures, as right now they are giving almost the same power (the ability to make isValidSignature return true in different ways), with the difference that presignHash, as implemented right now, is a final action that cannot be undo, which was the reason for having two different roles.

I just spoke to some people from DAOStack. They call this type of contract an Avatar. Should we adopt that name?

1 Like

Little update on the progress here: we have decided to rename Actor app for Agent app as it is a more approachable name, less confusing with movie actors and still pretty precise for what the Agent app does. We are making an effort to editing the name in every document/code that has the old name.

The app is close to being merged and Aragon One is targeting releasing it on mainnet this week, with the appropriate warnings as it still hasn’t gotten an external security review:

4 Likes