Predicate (formerly Aethos) AVS

The 2-transaction model is a major flow in the protocol. The documentation states:

The second transaction, originated (off-chain) by the Aethos Aggregator, passes through the Aethos Service Manager. Thus, the Aethos Service Manager needs to be permissioned inside the Client Protocol: it should be able to call functions with special privileges, after ascertaining that Aethos Operators have authorized the transaction.

The problem is that this model is not compatible with arbitrary asynchronous transaction execution. For instance, before the Aggregator confirms the first task, an attacker can submit a second task and set an extremely low quorumThresholdCount, e.g., 1. Then the attacker can have just 1 operator (the attacker themselves, possibly) submit a valid signature, and the second task will get executed. But the second task can perform any call, including the call that the first task would perform, with its privileges inside the Client Protocol.

The obvious ways to address such problems are fraught with issues of their own. Major design decisions need to be made, enforced in the code, and communicated in the documentation. For instance:

• The Aethos Aggregator can hold a strictly-ordered queue of tasks, only one of which is executable at any time. This decision creates obvious potential for denial-of-service.

• The Client Protocol can re-validate signatures. This defeats the purpose of the 2-transaction flow, making it instead be equivalent to a 1-transaction flow, where the client calls the Aethos Service Manager to validate signatures.

• The generality of the design will need to be sacrificed. For instance, all tasks will need the same strong majority (so any quorum is valid, for any task), not a definition of quorum agreement (e.g., quorumThresholdCount) per-task.

Generally, the protocol implementation should include full integrations with clients, so that security auditors can examine the potential of attacks, like the above. In the current state of the implementation, we can only speculate about what Client Protocols expect of Aethos and what assumptions they enforce. See issue P4 for specific examples of threats.

The current check for signature validity is based on a simple numeric count. Stakes (field totalStake) are only used to compare against a threshold, to tell whether an operator is registered. Signature validation does not use stakes at all, only counts signers.

function _validateSignatures(TaskParams memory _params, bytes[] memory signatures, bytes32 taskHash)
    	internal
    	view
    	returns (bool)
{
  for (uint256 i; i != _params.quorumThresholdCount;) {
    address recoveredSigner = ECDSA.recover(taskHash, signatures[i]);
    require(
     	operators[recoveredSigner].status == OperatorStatus.REGISTERED,
      "Signer is not a registered operator"
    );
    unchecked {
     	++i;
    }
  }
  return true;
}

This violates the principle of economic security that EigenLayer supports, which is based on stakes, and not simply the number of signers. The weaker economic security will be more vulnerable to sybil attacks and more.

The Aethos Service Manager interacts directly with EigenLayer, skipping the EigenLayer Middleware. (This is not a problem by itself, just a design choice. EigenLayer Middleware would likely replace much of the Aethos Service Manager functionality, with different underlying technology—e.g., BLS signatures instead of ECDSA. Skipping the middleware when its full functionality is not needed is fine.)

However, the central concept of a “quorum” is used partially. The Aethos Service Manager effectively keeps its own “quorum” (the set of participating operators) and inherits its strategies from a quorum:

function addStrategy(address _strategy, uint8 quorumNumber, uint256 index) external onlyOwner {
 // check if strategy exists
 IStakeRegistry.StrategyParams memory strategyParams =

 IStakeRegistry(stakeRegistry).strategyParamsByIndex(quorumNumber, index);
 if (address(strategyParams.strategy) != _strategy) {
   revert ServiceManager__InvalidStrategy();
 }

 strategy.push(_strategy);
 emit StrategyAdded(_strategy);
}

(The above code is not entirely well-motivated: if the _strategy is to match one looked up in internal data structures of the stakeRegistry, why have it be externally supplied at all?)

There is no consistency requirement documented in the above functionality. Are all the strategies that the Aethos Service Manager will use in a single EigenLayer quorum? Are they all the strategies of a quorum? Whatever the policy, it should be clearly documented and the relationship of Aethos consensus to EigenLayer quorums should be very clear. See also issue M2.

Another instance of such confusion is the function below, which accepts an array of quorum numbers and subsequently ignores it. (Only the length of the array is read, and it is unnecessary, since it is also supplied by other parameters.)

function updateOperatorsForQuorum(address[][] calldata operatorsPerQuorum,
  bytes calldata quorumNumbers) external {
    	if (operatorsPerQuorum.length != quorumNumbers.length) {
        	revert ServiceManager__ArrayLengthMismatch();
    	}
    	address[] memory currQuorumOperators;
    	address currOperatorAddress;
    	OperatorInfo storage currOperator;
    	for (uint256 i; i != quorumNumbers.length;) {
        ... // Dedaub: quorumNumbers is entirely unused after this point

Client protocols and operators are expected to be able to enforce numerous security properties, which the Service Manager does not currently enforce.

Clients need to validate the task data that is forwarded to the ServiceManager contract. There are several potential issues.

• Hashing admits collisions

bytes32 messageHash = keccak256(
    abi.encode(
    _params.msgSender,
    _params.target,
    _params.functionSig,
    _params.functionArgs,
    _params.quorumThresholdCount,
    _params.expireByBlockNumber
    )
);

The hashing of task data does not include:

• a nonce, to prevent repeating tasks
• a chainId, to prevent replaying tasks in case of multi-chain deployment
• a domain separator, to prevent replaying tasks across Client Protocols with similar APIs.

A Client Protocol could still avoid collisions by including such parameters inside its own API, i.e., as _params.functionArgs. If this is the assumption, it should be clearly documented, since it is quite onerous for clients.

• Client protocols are implicitly expected to carry out important checks when invoking ServiceManager::aethosVerifiedTask. The _params argument contains data that are provided to the client protocol by an external user, but the client protocol has to make sure that the quorumThresholdCount field is not tainted, as an external user can make it be 0 and signature validation will be completely bypassed.

• Only the first params.quorumThresholdCount elements of the signerAddresses array are checked by the Service Manager.

for (uint256 i = 0; i < _params.quorumThresholdCount;) {
  if (i > 0 &&
      uint160(signerAddresses[i]) <= uint160(signerAddresses[i - 1])) {
    revert("Signer addresses must be unique and sorted");
  } ...

The rest could contain duplicates, non-sorted elements, etc. The client should not assume anything about such an array, merely because it passes validation.

Finally, the params.msgSender supplied to, e.g., submitTask is not explicitly tied to the msg.sender of the submitTask transaction. The trust connection between the two is currently left to the operators that sign the transaction. Client Protocols either should not trust the params.msgSender field, or, alternatively, operators need to have a way to verify, within a given policy, that the params.msgSender of a task is not maliciously spoofed.

function getOperatorRestakedStrategies(address operator) external view returns (address[] memory) {
    	address[] memory restakedStrategies;
    	uint256 index = 0;
    	for (uint256 i = 0; i < strategy.length; i++) {
            	            	...
            	            	restakedStrategies[index] = strategy[i];
            	            	index++;
        	}
    	}
    	return restakedStrategies;
}

In the referenced code snippet, the restakedStrategies[index] = strategy[i]; line should be replaced by restakedStrategies.push(strategy[i]). The index variable will be no longer needed.

It seems inconsistent that the thresholdStake for a registered operator is fixed after Service Manager initialization (can only be set in initializer). The number and nature of strategies can change by calling addStrategy/removeStrategy. Strategies do not appear to be otherwise weighted in any way.

The stake of an operator could drop and the operator could become unregistered between his signing of a transaction and the calling of ServiceManager::executeTask. In such case, the call will fail, instead of continuing on to the next signer.

  for (uint256 i; i != _params.quorumThresholdCount;) {
    address recoveredSigner = ECDSA.recover(taskHash, signatures[i]);
    require(
     	operators[recoveredSigner].status == OperatorStatus.REGISTERED,
      "Signer is not a registered operator"
    );
    unchecked {
     	++i;
    }
  }

In an extreme case, this race condition can be leveraged for a denial-of-service. A staker might front-run ServiceManager::executeTask and prevent the execution of a task. Assuming that:

• a staker has delegated enough shares to affect whether an operator satisfies the protocol’s share threshold;
• the staker has incentives to block the execution of a task at a particular moment;

the staker could front-run the Aggregator’s executeTask transaction by calling:

• DelegationManager::queueWithdrawals to remove his stake from an operator that has signed the task to-be-executed
• ServiceManager::updateOperatorsForQuorum to unregister the operator for which the share number has now fallen below the protocol’s threshold

This will cause a revert when ServiceManager::_validateSignatures is executed.

The owner of the ServiceManager contract can influence any vote outcome, by deregistering operators, changing strategies, etc. The aggregator is trusted to execute tasks only when truly approved. (Otherwise can supply params that evade all on-chain signature validation, e.g., with quorumThresholdCount == 0, which always validates correctly.)

Issue P3 mentioned that the Aethos system could be re-implemented to leverage the EigenLayer Middleware functionality. Another consideration for integration, however, is the version of EigenLayer itself that is used. For instance, the m2-mainnet (also HEAD, currently) branch of the eigenlayer-contracts repository seems like a good candidate for integration in the relatively near future. We are not certain of the full extent of the changes, but we did observe that interface IAVSDirectory is not there and the role of avsDirectory is played by the delegation manager itself.

The code below is non-idiomatic:

if (pendingTasks[taskHash] == false ...

Can become:

if (!pendingTasks[taskHash] ...

 * @notice Validates signatures using the solady ECDSA library

(The ECDSA library switched to OZ.)

/**
  * @notice Updates the stakes of all operators for each of the specified quorums in the
  * StakeRegistry. Each quorum also has their quorumUpdateBlockNumber updated. which is meant
  * to keep track of when operators were last all updated at once.
  ...
  */
function updateOperatorsForQuorum

The comment is copy-paste from EigenLayer middleware code, but no part of it applies. Neither the stakes in StakeRegistry get updated nor quorumUpdateBlockNumber. The rest of the comment (not shown) is also not applicable.

The loop below can be optimized to accept an (optional?) index hint and confirm that the strategy is at the expected index, to avoid the linear search.

function removeStrategy(address _strategy) external onlyOwner {
    	for (uint256 i = 0; i != strategy.length;) {
        	if (strategy[i] == _strategy) {
            	strategy[i] = strategy[strategy.length - 1];
            	strategy.pop();
            	emit StrategyRemoved(_strategy);
            	break;
        	}
        	unchecked {
            	++i;
        	}
    	}
}

The function below has no side effects and can be labeled view:

function _aethosVerified(
    	TaskParamsWithExpiry memory _params,
    	address[] memory _signerIds,
    	bytes[] memory _signatures
) internal returns (bool) {
   	return serviceManager.aethosVerifiedTask(_params, _signerIds,
                                               _signatures);
}

The code is compiled with Solidity 0.8.12. This version has some known bugs, which we do not believe to affect the correctness of the contracts.