Infura DIN AVS

The protocol distinguishes two roles, the AllowLister and the Watchers which both can control permission for operators to register for an operator set. The first being through the Allowlist functionalities, and the second being through the Qualification functionalities. One reason for having both access controls is to give more decentralization over the control of the participation of the operators. In that sense, it requires both roles to accept the operator for it to be an active operator. In the current design, when a watcher chooses to submit a slash request, the operator is both disqualified and removed from the allowlist.

Our recommendation is that a watcher should be able only to control the qualified mapping and should not be able to unilaterally remove operators from the allowlist .

A scenario one might consider is the case where a watcher makes a wrong decision about slashing an operator. Then during the veto period, the slashing request is canceled by the veto committee. In that case, the admin (or the watcher) might re-qualify the operator. However, based on the current design this will not be sufficient to allow the operator to re-register because he is not yet allowlisted. That will require the involvement of the allowlister to add the operator back to the allowlist. Given that in this case it was a watcher mistake to request a slash, involving the allowlister to fix the mistake seems unnecessary. Therefore, it makes more sense to remove the operator from the allowlist, only when the slash is confirmed.

The DIN AVS performs a complex interaction with the EigenLayer protocol. Hence, it is highly recommended to test different functionalities in the DIN AVS using a comprehensive test suit. Currently, the included tests do not cover all the functionalities. An example of missing tests is:

• Queuing slash requests, canceling them, and fulfilling them.
• Registering and deregistering operators. Consider adding tests for different numbers of operators and different stake allocation values.
• Distributing rewards on watchers and operators.

Note that these examples serve as a guideline but the developers are highly advised to do their own analysis and add more tests as needed. Also, the developers can adapt the unit tests of the middleware contracts to the DIN-AVS.

The owner of the contract ServiceManager is currently set as an admin by calling _permissionController.addPendingAdmin(). After the owner accepts the admin role, the permissionController functions are only callable by the admin. Therefore, the following functions of ServiceManager contract cannot be called by anyone:

• ServiceManager::addPendingAdmin()
• ServiceManager::removePendingAdmin()
• ServiceManager::removeAdmin()
• ServiceManager::setAppointee()
• ServiceManager::removeAppointee()

Therefore, any further updates for the permissions of DIN-AVS can be done by the admin only by directly calling the PermissionController contract and not through ServiceManager functions. For example, the call ServiceManager::removeAdmin() will revert when called by the admin.

If this behavior is not convenient for the developers, then they must consider adding the DIN-AVS contract as an admin to itself by adding in the constructor of ServiceManager the following calls:

_permissionController.addPendingAdmin(address(this), address(this)) _permissionController.acceptAdmin(address(this)).

However, we must stress that security-wise, it is safer not to take this approach since it is preferred to use setAppointee() to only give the ServiceManager the permission to call specific functions (as currently being done). The latter approach follows the least privilege security principle.

Qualification::finalizeQualification takes an array of operator set ids, operatorSetIds, as input and iterates over these to ensure fees have been paid before granting/finalising the qualification for each operator set. Initially, a loop is performed over operatorSetIds to check that indeed there are qualifications pending for the specified operator sets and operator, and that the fees have been paid. After this, another loop takes care of accounting the fees by resetting the relevant variable corresponding to the pending qualification. Finally the fees are transferred to the watcher.

However, consider that operatorSetIds contains the same id twice. In the current implementation the corresponding fee for this id will be added to totalPaidFee twice, and the watcher will be overpaid. This would result in making the accounting of the fees paid invalid, and creating a race condition as in H1.

Advisory A4 suggests joining the two loops as a gas optimisation, doing this would also have the effect of preventing this issue. If these are joined, note that the code of the second loop removes finalised pending qualifications and any duplicates would thus cause a revert when executing the checks currently implemented in the first loop.

The function ServiceManager::fulfillSlashingRequest() performs a check

require(
    block.number
        <= request.requestBlock + _stakeRegistry.delegation().minWithdrawalDelayBlocks() - vetoWindowBlocks,
    SlashingPeriodOver()
);

The check is intended to make sure that the slash does not affect stakers that had chosen to un-delegate and withdraw their shares from the operator being slashed prior to the slash by MIN_WITHDRAWAL_DELAY_BLOCKS blocks. However, this check is not actually needed as the DelegationManager contract already provides this guarantee. Therefore, this check can be removed.

Qualification.sol provides functionality for prospective operators to apply for qualification to register to operator sets (initiateQualification). This involves payment of fees, the accounting of which is kept track of in paidQualificationFees. Watchers can approve this qualification (finalizeQualification) and receive the fees paid by the operator.

The issue arises since the admin has the ability to drain any amount of fees from the contract (drainFees).

function drainFees(uint256 amount, address recipient) external onlyQualificationAdmin {
        require(recipient != address(0), "Invalid recipient");
        emit FeesDrained(recipient, amount);
        qualificationFeeToken.safeTransfer(recipient, amount);
    }

Using this function leads to an incorrect accounting of fees. Fees paid (paidQualificationFees) is used to calculate the amount of fees to be paid to the watcher finalising (granting or rejecting) the qualification, modulo an operator. An implicit invariant is that the total amount of fees paid per operator should be the minimum amount of tokens held by contract, otherwise not every qualification can be finalised and instead they remain pending.

In effect, if any non-zero amount of paid fees are drained from the contract a race condition is created: at least one operator will not be able to be qualified, depending on which order the watchers finalise qualifications.

The protocol defines a veto committee, typically an account with multisig access from trusted entities, which acts as the authority to approve operator slashes. This mechanism is used to ensure less centralization and reduce the trust assumption of the DIN-AVS.

However, the contract ServiceManager sets a pending admin in its initialize function using IPermissionController. Once an admin account accepts, the admin is permitted to call the function IAllocationManager::slashOperator() directly on behalf of the DIN-AVS enabling them to perform immediate slashes to operators bypassing the veto committee approval. Therefore, giving the admin the permission to perform slashes defeats the purpose of having a veto committee.

The ServiceManager::onlyWatcher modifier is used to ensure that only watchers are allowed to finalise qualifications and slash operators. However, the only constraints on msg.sender to satisfy onlyWatcher are: i) they are registered as an operator to some operator set; ii) they are in the allowlist to be a watcher (or the watcher allowlist is disabled), which does not require them to actually be registered to the watcher operator set.
While allowlisting gives security that the watcher is trusted, not requiring their registration to an operator set means that reward calculations for watchers will not take into account unregistered watchers. Note these are based on the watcher operator set, and happen outside the AVS.

Qualification::getTotalQualificationFee, which takes as input a list of operator set ids, is meant to provide prospective operators with the total amount of fees they need to pay to register for a list of operator sets. However, it does not take into account that if there is any operator set with id i for which the fee amount is 0, qualificationFeeAmounts[i] == 0, then initiateQualification will revert (see Qualification::194).

ServiceManager.fulfillSlashingRequest enables watchers to fulfill a slashing request, slashing an operator from a pre-specified operator set. There are certain requires that must be satisfied for this to happen:

IVetoableSlasher.VetoableSlashingRequest storage request = slashingRequests[requestId];

require(block.number >= request.requestBlock + vetoWindowBlocks, VetoPeriodNotPassed());
...
require(
        block.number
           <= request.requestBlock +
       _stakeRegistry.delegation().minWithdrawalDelayBlocks() - vetoWindowBlocks,
            SlashingPeriodOver()
);

Note that the second require can only be satisfied if the following constraint holds MIN_WITHDRAWAL_DELAY_BLOCKS >= 2*vetoWindowBlocks. However this is not enforced anywhere, in fact vetoWindowBlocks can be set to any value in the constructor of ServiceManager. Should ServiceManager not be initialised properly, slashing will be disabled. Notice that M1 points out that the check in fulfillSlashingRequest is not needed. However, it is still recommended to implement a validation in the Deployer contract to check that MIN_WITHDRAWAL_DELAY_BLOCKS >= vetoWindowBlocks + some_buffer to ensure that stakers do not get the opportunity to withdraw their stakes before the slash is executed.

Slashing in very small increments, slashing operators with very low magnitudes, or slashing operators with very low share balances may lead to precision loss that results in burns being far lower than expected.
This may occur primarily when:

• Operators have very low allocated magnitudes
• Operators have very few delegated shares
• Very small slashing percentages are used
• Tokens with low decimal precision are involved

To avoid these edge cases, EigenLayer recommends a set of guidelines that involves pre-registration validations and pre-slashing validation. The details of these guidelines can be found in the EigenLayer docs.

The QualificationAdmin controls important aspects of qualification (which is required for operators to register for operator sets):

• It can disable qualifications for any operator set, by setting (setQualificationFeeAmounts) the fee amount to be paid to 0 (see Qualification::194 which reverts initiateQualification if the fee to be paid for one operator set is 0).
• It can bypass the normal registration process by manually adding qualification operators (addQualifiedOperator, batchAddQualifiedOperator).

The deployer of the AVS is set as the admin in the initialization function of the ServiceManager contract. Once the deployer account accepts its admin role, it acquires the following privileges:

• Add a pending admin
• Remove a pending admin
• Remove an admin
• Set and remove appointee to grant permission for a specific function of the EigenLayer core contracts
• Slash operators (see H3).
• Change the AVS Registrar
• Update the metadata URI for the AVS
• Create new operator sets
• Add and remove strategies from operator sets
• Deregister operators
• Change the address authorized to claim rewards

Due to its critical privileges, the admin account is considered a single point of failure for the protocol.

The allowlister account can add and remove operators from the allowlist which gives him the ability to censor operators from the protocol.

Any account who is added to the watcher operators set acquires the following privileges:

• Approve and disapprove qualifications of operators
• Deregister operators by submitting slashing requests
• Collect the qualification fees

A centralization issue is raised on the watcher role, as the protocol trusts these accounts without any mechanism in place to enforce a correct behavior of a watcher account.

When operators are added or removed from an allowlist (AllowList::{addOperatorsToAllowlist, removeOperatorsFromAllowlist}, and the corresponding batch functions) events are emitted with the status of the operator. If, e.g., addOperatorsToAllowlist is called with operators already in the allowlist and the same qualified status, an OperatorUpdated event will still be emitted.

Similarly for _disqualify function, an OperatorUpdated is emitted even when the operator is already unqualified.

AllowList::{addOperatorsToAllowlist, batchAddOperatorsToAllowlists} replicate the same code iterating over the input operator array. This can be optimised with an internal function that both depend on, as is being done for the remove functions.

In some places important addresses are input to the system without any validation. We advise validating that these addresses are not the zero address, to avoid easy-to-make mistakes. See Qualification::setQualificationFeeToken, Deployer::_create, Allowlist::_setAllowlister, and ServiceManager::{constructor, initialize}.

Qualification::finalizeQualification performs two separate iterations over operatorSetIds, first doing some validation and then both emitting relevant events and clearing storage. These can be done in the same loop as a gas optimisation.

ServiceManager::cancelSlashingRequest replicates checks already done in the function it calls VetoableSlasher::_cancelSlashingRequest. This can be removed, and instead rely on the public function VetoableSlasher::cancelSlashingRequest, which already performs the same behaviour (note ServiceManager inherits VetoableSlasher).

Additionally, the function ServiceManager::canRegister can reuse the function ServiceManager::isOperatorQualified to populate the variable isQualified which may reduce the code size.

The wrapper functions BN254Wrapper::{hashG1Point, hashG2Point} calls the functions BN254::{hashG1Point, hashG2Point} which do not perform a check whether the point to be hashed is of reduced coordinates. Therefore, a point A with reduced coordinates (x, y) can be passed using the coordinates (x+kp, y+kp) for any integer k representing the same point A but leading to a different hash result. Such behavior might lead to security bugs in the protocol if used incorrectly.

To fulfill a slash request, a call to the function SlasherBase::_fulfillSlashingRequest is performed which simply calls allocationManager.slashOperator. Usually, after a slash is performed, it is required to call SlashingRegistryCoordinator::updateOperators() to trigger an update of the stakes accounted for in StakeRegistry contract as shown in VetoableSlasher::_fulfillSlashingRequestAndMarkAsCompleted(). However, in this case the call to updateOperators() is omitted because the slashed operator has been deregistered by the watcher at the time the slash is requested. As the operator is deregistered, his stake in the stakeRegistry is reduced to zero and thus the update is not needed after executing the slash.

While this sounds like a valid assumption, our thorough testing of comparing the case of calling the update after the slash and ignoring it shows that there might be a slight difference in the resulting stake of that operator in other operator sets that the operator might still be registered to. The difference comes from rounding errors. To help explain the case, consider the following example.

• Operator A registers to sets 2 and 3 and allocates 2 Eth for each set (total of 4 eth).
  StakeRegistry: [stake[A][2] = 2, stake[A][3] = 2]
  AllocationManager: [currentMagnitude[A][2] = 5e17, currentMagnitude[A][3] = 5e17, maxMagnitude[A] = 1e18]
  DelegationManager: [shares[A] : 4eth]
• A watcher requests a slash with 50% magnitude on operator A for set 2. As a result, the request leads to deregistering operator A from set 2.
  StakeRegistry: [stake[A][2] = 0, stake[A][3] = 2]
  AllocationManager: [currentMagnitude[A] = 5e17, currentMagnitude[A][3] = 5e17, maxMagnitude[A] = 1e18]
  DelegationManager: [shares[A] : 4eth]
  Note here that the stake on set 2 had gone to zero because the operator was deregistered.
• Later after the veto period passes, the slash request is fulfilled leading to a slash for operator A stakes equal to 1 ETH. Now consider the two options:

  • updateOperators(A) is not called: StakeRegistry: [stake[A][2] = 0, stake[A][3] = 2]
    AllocationManager: [currentMagnitude[A][2] = 2.5e17, currentMagnitude[A][3] = 5e17, maxMagnitude[A] = 7.5e17]
    DelegationManager: [shares[A] : 3eth]

  • updateOperators(A) is called => StakeRegistry: [stake[A][2] = 0, stake[A][3] = 1.999999]
    AllocationManager: [currentMagnitude[A][2] = 2.5e17, currentMagnitude[A][3] = 5e17, maxMagnitude[A] = 7.5e17]
    DelegationManager: [shares[A] : 3eth]

The reason for this difference is that when the slash is performed, the stake is recalculated from the state of the AllocationManager and DelegationManager contracts as follows:

This calculation undergoes some rounding errors leading to the result 1.99999 instead of 2.

While we do not see direct problems resulting from this difference in the calculation, we suggest that it is better to use the protocol as intended and call the updateOperators() function after executing the slashing to avoid any edge case scenarios due to this inconsistency in the values between various contracts.

The code is compiled with Solidity 0.8.27. Version 0.8.27, in particular, has no known bugs.