Kilnfi Staking (Consensys)

1 Executive Summary

This report presents the results of our engagement with ConsenSys/Kilnfi to review the Staking Contracts.

The review was conducted over two weeks, from Aug 28, 2023 to Sept 08, 2023. A total of 2x10 person-days were spent.

The examined system encompasses a suite of staking contracts designed to facilitate registered node operators’ deposits of validator keys funded by external users. This architecture streamlines the staking process for end users, enabling them to stake with one or multiple validators using a single transaction.

Distinct roles within the system include the EndUser, responsible for providing funds for validator node staking; the Operator, tasked with managing and operating a set of validator nodes, including their associated keys; and Administrative accounts (system and proxy) responsible for system maintenance.

The system is equipped with intricate logic and data structures to accommodate multiple operators, introducing a significant level of complexity into the contracts. However, it’s worth noting that the system currently permits only one operator, as per the client’s intention to retain this role exclusively, with no plans to involve external operators. Although there exists the potential for deposit contract front-running by operators, it is easily detectable. Furthermore, there is also potential for operators to not comply with their staking users requests to exit from staking or operators changing/presetting beacon chain credentials. However, given that the system is not open to untrusted external operators and the client is an officially recognized legal entity, the likelihood of such events leading to legal disputes or compliance issues is minimal.

The contracts are designed with pausability and upgradeability features. The proxy administrator retains the authority to execute system upgrades at any point in time, and parameters can be modified without prior notice to users, a centralization aspect that warrants consideration. WithdrawCredentials for nodes are determined and linked to a deterministic address generated using the create2 method. This address is indirectly controlled by the EndUser, acting as the “withdrawer.”

It’s important to highlight that the contract deployed via create2 functions as a clone proxy, pointing to a designated FeeRecipientImplementation. While there is presently no inherent logic to alter the FeeRecipientImplementation, an internal setter is in place, but it is exclusively utilized during the initialization phase.

It is crucial to note that making changes to the FeeRecipientImplementation while users have active stakes could result in a withdrawal credential mismatch, rendering funds unwithdrawable. This scenario may occur when user stakes are associated with withdrawal credentials configured for their create2 rewards recipient. Should the contract administrator decide to upgrade the contracts and modify the FeeRecipientImplementation, users attempting to withdraw rewards may face complications, as the create2 deployment would direct to a different address due to the altered implementation.

2 Scope

Our review focused on the commit hash kilnfi/staking-contracts@f33eb8dc37fab40217dbe1e69853ca3fcd884a2d. The list of files in scope can be found in the Appendix. The following official documents were provided to the review team:

2.1 Objectives

Together with the client, we identified the following priorities for our review:

  1. Correctness of the implementation, consistent with the intended functionality and without unintended edge cases.
  2. Identify known vulnerabilities particular to smart contract systems, as outlined in our Smart Contract Best Practices, and the Smart Contract Weakness Classification Registry.

3 System Overview

This section describes the top-level/deployable contracts, their inheritance structure and interfaces, actors, permissions and important contract interactions of the system under review.

Contracts are depicted as boxes. Public reachable interface methods are outlined as rows in the box. The 🔍 icon indicates that a method is declared as non-state-changing (view/pure) while other methods may change state. A row at the top of the contract shows inherited contracts. A green row at the top of the contract indicates that that contract is used in a usingFor declaration. Modifiers used as ACL are connected as bubbles in front of methods.

Architecture Overview

Architecture Overview

4 Findings

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

4.1 Incorrect Priviliges setOperatorAddresses Major  Acknowledged

Description

The function setOperatorAddresses instead of allowing the Operator to update its own, as well as the Fee Recipient address, incorrectly provides the privileges to the Fee Recipient. As a result, the Fee Recipient can modify the operator address as and when needed, to DoS the operator and exploit the system. Additionally, upon reviewing the documentation, we found that there are no administrative rights defined for the Fee Recipient, hence highlighting the incorrect privilege allocation.

src/contracts/StakingContract.sol:L412-L424

function setOperatorAddresses(
    uint256 _operatorIndex,
    address _operatorAddress,
    address _feeRecipientAddress
) external onlyActiveOperatorFeeRecipient(_operatorIndex) {
    _checkAddress(_operatorAddress);
    _checkAddress(_feeRecipientAddress);
    StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();

    operators.value[_operatorIndex].operator = _operatorAddress;
    operators.value[_operatorIndex].feeRecipient = _feeRecipientAddress;
    emit ChangedOperatorAddresses(_operatorIndex, _operatorAddress, _feeRecipientAddress);
}

Recommendation

The modifier should be onlyActiveOperatorOrAdmin allowing only the operator itself or admin of the system, to update the necessary addresses.

Also, for transferring crucial privileges from one address to another, the operator’s address should follow a 2-step approach like transferring ownership.

4.2 Unconstrained Snapshot While Setting Operator Limit Medium

Description

Function setOperatorLimit as the name says, allows the SYS_ADMIN to set/update the staking limit for an operator. The function ensures that if the limit is being increased, the _snapshot must be ahead of the last validator edit(block.number at which the last validator edit occurred). However, the parameter _snapshot is unconstrained and can be any number. Also, the functions addValidators and removeValidators update the block.number signifying the last validator edit, but never constrain the new edits with it. Since there are no publicly available functions to access this value, makes the functionality even more confusing and may be unnecessary.

src/contracts/StakingContract.sol:L468-L473

if (
    operators.value[_operatorIndex].limit < _limit &&
    StakingContractStorageLib.getLastValidatorEdit() > _snapshot
) {
    revert LastEditAfterSnapshot();
}

Recommendation

If the functionality is not needed, consider removing it. Otherwise, add some necessary logic to either constrain the last validator edit or add public functions for the users to access it.

4.3 Missing Input Validation Medium

Description

  • There is no zero address check in function addOperator for the operator address and fee recipient. Also, the function doesn’t check whether the operator already exists.

src/contracts/StakingContract.sol:L392-L405

function addOperator(address _operatorAddress, address _feeRecipientAddress) external onlyAdmin returns (uint256) {
    StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();
    StakingContractStorageLib.OperatorInfo memory newOperator;

    if (operators.value.length == 1) {
        revert MaximumOperatorCountAlreadyReached();
    }
    newOperator.operator = _operatorAddress;
    newOperator.feeRecipient = _feeRecipientAddress;
    operators.value.push(newOperator);
    uint256 operatorIndex = operators.value.length - 1;
    emit NewOperator(_operatorAddress, _feeRecipientAddress, operatorIndex);
    return operatorIndex;
}
  • No zero address check in function setTreasury for updating the treasury address.

src/contracts/StakingContract.sol:L214-L217

function setTreasury(address _newTreasury) external onlyAdmin {
    emit ChangedTreasury(_newTreasury);
    StakingContractStorageLib.setTreasury(_newTreasury);
}
  • Functions activateOperator and deactivateOperator as the name hints, allow the admin to activate or deactivate an operator. However, the functions don’t check for already activated/deactivated operators, and may still emit DeactivatedOperator/ActivatedOperator events for an operator. It may trigger false alarms for off-chain monitoring tools and create unnecessary panic.

src/contracts/StakingContract.sol:L479-L502

/// @notice Deactivates an operator and changes the fee recipient address and the staking limit
/// @param _operatorIndex Operator Index
/// @param _temporaryFeeRecipient Temporary address to receive funds decided by the system admin
function deactivateOperator(uint256 _operatorIndex, address _temporaryFeeRecipient) external onlyAdmin {
    StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();
    operators.value[_operatorIndex].limit = 0;
    emit ChangedOperatorLimit(_operatorIndex, 0);
    operators.value[_operatorIndex].deactivated = true;
    emit DeactivatedOperator(_operatorIndex);
    operators.value[_operatorIndex].feeRecipient = _temporaryFeeRecipient;
    emit ChangedOperatorAddresses(_operatorIndex, operators.value[_operatorIndex].operator, _temporaryFeeRecipient);
    _updateAvailableValidatorCount(_operatorIndex);
}

/// @notice Activates an operator, without changing its 0 staking limit
/// @param _operatorIndex Operator Index
/// @param _newFeeRecipient Sets the fee recipient address
function activateOperator(uint256 _operatorIndex, address _newFeeRecipient) external onlyAdmin {
    StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();
    operators.value[_operatorIndex].deactivated = false;
    emit ActivatedOperator(_operatorIndex);
    operators.value[_operatorIndex].feeRecipient = _newFeeRecipient;
    emit ChangedOperatorAddresses(_operatorIndex, operators.value[_operatorIndex].operator, _newFeeRecipient);
}

4.4 Hardcoded Operator Limit Logic Medium

Description

The contract defines some hardcoded limits which is not the right approach for upgradeable contracts and opens doors for accidental mistakes, if not handled with care.

The operators for the current version are limited to 1. If the auditee team decides to open the system to work with more operators but fails to change the limit while upgrading, the upgraded contract will have no effect, and will still disallow any more operators to be added.

src/contracts/StakingContract.sol:L392-L398

function addOperator(address _operatorAddress, address _feeRecipientAddress) external onlyAdmin returns (uint256) {
    StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();
    StakingContractStorageLib.OperatorInfo memory newOperator;

    if (operators.value.length == 1) {
        revert MaximumOperatorCountAlreadyReached();
    }

Also, the function _depositOnOneOperator hardcodes the operator Index as 0 since the contract only supports one operator.

src/contracts/StakingContract.sol:L893-L896

function _depositOnOneOperator(uint256 _depositCount, uint256 _totalAvailableValidators) internal {
    StakingContractStorageLib.setTotalAvailableValidators(_totalAvailableValidators - _depositCount);
    _depositValidatorsOfOperator(0, _depositCount);
}

Recommendation

A better approach could be to constrain the limit of operators that can be added with a storage variable or constant, provided at the time of contract initialization. The contract should also consider supporting dynamic operator deposits for future versions instead of the default hardcoded index.

4.5 StakingContract - PubKey Length Checks Not Always Enforced Medium

Description

addValidators checks that the provided bytes pubKey is a multiple of the expected pubkey length while functions like setWithdrawer do not enforce similar length checks. This is an inconsistency that should be avoided.

  • addValidators enforcing input length checks

src/contracts/StakingContract.sol:L530-L542

function addValidators(
    uint256 _operatorIndex,
    uint256 _keyCount,
    bytes calldata _publicKeys,
    bytes calldata _signatures
) external onlyActiveOperator(_operatorIndex) {
    if (_keyCount == 0) {
        revert InvalidArgument();
    }

    if (_publicKeys.length % PUBLIC_KEY_LENGTH != 0 || _publicKeys.length / PUBLIC_KEY_LENGTH != _keyCount) {
        revert InvalidPublicKeys();
    }
  • setWithdrawer accepting any length for a pubKey. Note that _getPubKeyRoot will take any input provided and concat it the zero bytes.

src/contracts/StakingContract.sol:L426-L445

/// @notice Set withdrawer for public key
/// @dev Only callable by current public key withdrawer
/// @param _publicKey Public key to change withdrawer
/// @param _newWithdrawer New withdrawer address
function setWithdrawer(bytes calldata _publicKey, address _newWithdrawer) external {
    if (!StakingContractStorageLib.getWithdrawerCustomizationEnabled()) {
        revert Forbidden();
    }
    _checkAddress(_newWithdrawer);
    bytes32 pubkeyRoot = _getPubKeyRoot(_publicKey);
    StakingContractStorageLib.WithdrawersSlot storage withdrawers = StakingContractStorageLib.getWithdrawers();

    if (withdrawers.value[pubkeyRoot] != msg.sender) {
        revert Unauthorized();
    }

    emit ChangedWithdrawer(_publicKey, _newWithdrawer);

    withdrawers.value[pubkeyRoot] = _newWithdrawer;
}

src/contracts/StakingContract.sol:L775-L777

function _getPubKeyRoot(bytes memory _publicKey) internal pure returns (bytes32) {
    return sha256(abi.encodePacked(_publicKey, bytes16(0)));
}
  • similarly, the withdraw family of functions does not enforce a pubkey length either. However, it is unlikely that someone finds a pubkey that matches a root for the attackers address.

src/contracts/StakingContract.sol:L696-L702

/// @notice Withdraw the Execution Layer Fee for a given validator public key
/// @dev Funds are sent to the withdrawer account
/// @param _publicKey Validator to withdraw Execution Layer Fees from
function withdrawELFee(bytes calldata _publicKey) external {
    _onlyWithdrawerOrAdmin(_publicKey);
    _deployAndWithdraw(_publicKey, EXECUTION_LAYER_SALT_PREFIX, StakingContractStorageLib.getELDispatcher());
}

Nevertheless, the methods should be hardened so as not to give a malicious actor the freedom to use an unexpected input size for the pubKey argument.

Recommendation

Enforce pubkey length checks when accepting a single pubkey as bytes similar to the batch functions that check for a multiple of ´PUBLIC_KEY_LENGTH´. Alternatively, declare the function argument as bytes48 (however, in this case inputs may be auto-padded to fit the expected length, pot. covering situations that otherwise would throw an error)

4.6 Unpredictable Behavior Due to Admin Front Running or General Bad Timing Medium

Description

In a number of cases, administrators of contracts can update or upgrade things in the system without warning. This has the potential to violate a security goal of the system.

Specifically, privileged roles could use front running to make malicious changes just ahead of incoming transactions, or purely accidental negative effects could occur due to the unfortunate timing of changes.

Some instances of this are more important than others, but in general, users of the system should have assurances about the behavior of the action they’re about to take.

Examples

  • Upgradeable TU proxy

  • Fee changes take effect immediately

src/contracts/StakingContract.sol:L504-L512

/// @notice Change the Operator fee
/// @param _operatorFee Fee in Basis Point
function setOperatorFee(uint256 _operatorFee) external onlyAdmin {
    if (_operatorFee > StakingContractStorageLib.getOperatorCommissionLimit()) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setOperatorFee(_operatorFee);
    emit ChangedOperatorFee(_operatorFee);
}

src/contracts/StakingContract.sol:L513-L522


/// @notice Change the Global fee
/// @param _globalFee Fee in Basis Point
function setGlobalFee(uint256 _globalFee) external onlyAdmin {
    if (_globalFee > StakingContractStorageLib.getGlobalCommissionLimit()) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setGlobalFee(_globalFee);
    emit ChangedGlobalFee(_globalFee);
}

Recommendation

The underlying issue is that users of the system can’t be sure what the behavior of a function call will be, and this is because the behavior can change at any time.

We recommend giving the user advance notice of changes with a time lock. For example, make all upgrades require two steps with a mandatory time window between them. The first step merely broadcasts to users that a particular change is coming, and the second step commits that change after a suitable waiting period.

4.7 Potentially Uninitialized Implementations Medium

Description

Most contracts in the system are meant to be used with a proxy pattern. First, the implementations are deployed, and then proxies are deployed that delegatecall into the respective implementations following an initialization call (hardhat, with same transaction). However, the implementations are initialized explicitly nor are they protected from other actors claiming/initializing them. This allows anyone to call initialization functions on implementations for use with phishing attacks (i.e. contract implementation addresses are typically listed on the official project website as valid contracts) which may affect the reputation of the system.

None of the implementations allow unprotected delegatecalls or selfdesturcts. lowering the severity of this finding.

Examples

src/contracts/StakingContract.sol:L151-L162

function initialize_1(
    address _admin,
    address _treasury,
    address _depositContract,
    address _elDispatcher,
    address _clDispatcher,
    address _feeRecipientImplementation,
    uint256 _globalFee,
    uint256 _operatorFee,
    uint256 globalCommissionLimitBPS,
    uint256 operatorCommissionLimitBPS
) external init(1) {

src/contracts/AuthorizedFeeRecipient.sol:L21-L32

/// @notice Initializes the receiver
/// @param _dispatcher Address that will handle the fee dispatching
/// @param _publicKeyRoot Public Key root assigned to this receiver
function init(address _dispatcher, bytes32 _publicKeyRoot) external {
    if (initialized) {
        revert AlreadyInitialized();
    }
    initialized = true;
    dispatcher = IFeeDispatcher(_dispatcher);
    publicKeyRoot = _publicKeyRoot;
    stakingContract = msg.sender; // The staking contract always calls init
}

src/contracts/FeeRecipient.sol:L18-L27

/// @param _publicKeyRoot Public Key root assigned to this receiver
function init(address _dispatcher, bytes32 _publicKeyRoot) external {
    if (initialized) {
        revert AlreadyInitialized();
    }
    initialized = true;
    dispatcher = IFeeDispatcher(_dispatcher);
    publicKeyRoot = _publicKeyRoot;
}

Recommendation

Petrify contracts in the constructor and disallow other actors from claiming/initializing the implementations.

4.8 Operator May DoS the Withdrawal or Make It More Expensive Medium

Description

While collecting fees, the operator may:

  1. cause DoS for the funds/rewards withdrawal by reverting the call, thus reverting the whole transaction. By doing this, it won’t be receiving any rewards, but so the treasury and withdrawer.

  2. make the withdrawal more expensive by sending a huge chunk of returndata. As the returndata is copied into memory in the caller’s context, it will add an extra gas overhead for the withdrawer making it more expensive.

  3. or mint gas token

src/contracts/ConsensusLayerFeeDispatcher.sol:L105-L110

if (operatorFee > 0) {
    (status, data) = operator.call{value: operatorFee}("");
    if (status == false) {
        revert FeeRecipientReceiveError(data);
    }
}

Recommendation

A possible solution could be to make a low-level call in an inline assembly block, restricting the returndata to a couple of bytes, and instead of reverting on the failed call, emit an event, flagging the call that failed.

4.9 ProxyAdmin May Cause DoS for SYS_ADMIN Medium

Description

As the TransparentUpgradeableProxy doesn’t allow the ProxyAdmin to delegate calls to the implementation, it means the SYS_ADMIN can’t be the same as ProxyAdmin. Now, talking about the design, the proxy defines a system-wide feature to pause or unpause. If the proxyAdmin pauses the staking contract, it implies no one can interact with it, not even the SYS_ADMIN, which might not be what the auditee team wants. There may be multiple scenarios where the auditee team wants to intervene manually in the system even if the system is paused, for instance, withdrawing funds while restricting the withdrawer.

        if (msg.sender == _getAdmin()) {
            bytes memory ret;
            bytes4 selector = msg.sig;
            if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) {
                ret = _dispatchUpgradeTo();
            } else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
                ret = _dispatchUpgradeToAndCall();
            } else if (selector == ITransparentUpgradeableProxy.changeAdmin.selector) {
                ret = _dispatchChangeAdmin();
            } else if (selector == ITransparentUpgradeableProxy.admin.selector) {
                ret = _dispatchAdmin();
            } else if (selector == ITransparentUpgradeableProxy.implementation.selector) {
                ret = _dispatchImplementation();
            } else {
                revert("TransparentUpgradeableProxy: admin cannot fallback to proxy target");
            }
            assembly {
                return(add(ret, 0x20), mload(ret))
            }

Recommendation

The system-wide pause feature should allow the SYS_ADMIN to call the staking functions if the system is paused. Something like:

function _beforeFallback() internal override {
 if (StorageSlot.getBooleanSlot(_PAUSE_SLOT).value == false || msg.sender == stakingContract.getAdmin() || msg.sender == address(0)) {

            super._beforeFallback();
        }

4.10 Ambiguous/Misleading Return Data Minor

Description

The NATSPEC comment says that the getters return address(0) or boolean false if the key doesn’t exist in the contract. However, the functions don’t check for any such logic. As a result, for any disabled validator, the functions may still return existing values besides the expected null values, conveying incorrect information to the users.

src/contracts/StakingContract.sol:L258-L263

/// @notice Retrieve withdrawer of public key
/// @notice In case the validator is not enabled, it will return address(0)
/// @param _publicKey Public Key to check
function getWithdrawer(bytes calldata _publicKey) external view returns (address) {
    return _getWithdrawer(_getPubKeyRoot(_publicKey));
}

src/contracts/StakingContract.sol:L265-L270

/// @notice Retrieve withdrawer of public key root
/// @notice In case the validator is not enabled, it will return address(0)
/// @param _publicKeyRoot Hash of the public key
function getWithdrawerFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (address) {
    return _getWithdrawer(_publicKeyRoot);
}

src/contracts/StakingContract.sol:L272-L277

/// @notice Retrieve whether the validator exit has been requested
/// @notice In case the validator is not enabled, it will return false
/// @param _publicKeyRoot Public Key Root to check
function getExitRequestedFromRoot(bytes32 _publicKeyRoot) external view returns (bool) {
    return _getExitRequest(_publicKeyRoot);
}

src/contracts/StakingContract.sol:L279-L284

/// @notice Return true if the validator already went through the exit logic
/// @notice In case the validator is not enabled, it will return false
/// @param _publicKeyRoot Public Key Root of the validator
function getWithdrawnFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (bool) {
    return StakingContractStorageLib.getWithdrawnMap().value[_publicKeyRoot];
}

src/contracts/StakingContract.sol:L286-L290

/// @notice Retrieve the enabled status of public key root, true if the key is in the contract
/// @param _publicKeyRoot Hash of the public key
function getEnabledFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (bool) {
    return StakingContractStorageLib.getOperatorIndexPerValidator().value[_publicKeyRoot].enabled;
}

4.11 ConsensusLayerFeeDispatcher/ExecutionLayerFeeDispatcher - Should Hardcode autoPetrify With Highest Initializable Version Instead of User Provided Argument Minor

Description

The version to auto-initialize is not hardcoded with the constructor. On deployment, the deployer may accidentally use the wrong version, allowing anyone to call initialize on the contract.

Examples

src/contracts/ConsensusLayerFeeDispatcher.sol:L47-L50

/// @notice Constructor method allowing us to prevent calls to initCLFR by setting the appropriate version
constructor(uint256 _version) {
    VERSION_SLOT.setUint256(_version);
}

src/contracts/ExecutionLayerFeeDispatcher.sol:L47-L57

/// @notice Constructor method allowing us to prevent calls to initCLFR by setting the appropriate version
constructor(uint256 _version) {
    VERSION_SLOT.setUint256(_version);
}

/// @notice Initialize the contract by storing the staking contract and the public key in storage
/// @param _stakingContract Address of the Staking Contract
function initELD(address _stakingContract) external init(1) {
    STAKING_CONTRACT_ADDRESS_SLOT.setAddress(_stakingContract);
}

Recommendation

Similar to the init(1) modifier, it is suggested to track the highest version as a const int with the contract and auto-initialize to the highest version in the constructor instead of taking the highest version as a deployment argument.

4.12 StakingContract - Misleading Comment Minor

Description

The comment notes that the expected caller is admin while the modifier checks that msg.sender is an active operator.

src/contracts/StakingContract.sol:L109-L114

/// @notice Ensures that the caller is the admin
modifier onlyActiveOperator(uint256 _operatorIndex) {
    _onlyActiveOperator(_operatorIndex);
    _;
}

Recommendation

Rectify the comment to accurately describe the intention of the method/modifier.

4.13 Impractical Checks for Global/Operator Fees and the Commission Limits Minor

Description

The contract initialization sets up the global and operator fees and also their commission limits. However, the checks just make sure that the fees or commission limit is up to 100% which is not a very practical check. Any unusual value, for instance, if set to 100% will mean the whole rewards/funds will be non-exempted and taxed as global fees, which we believe will never be a case practically.

src/contracts/StakingContract.sol:L168-L175

if (_globalFee > BASIS_POINTS) {
    revert InvalidFee();
}
StakingContractStorageLib.setGlobalFee(_globalFee);
if (_operatorFee > BASIS_POINTS) {
    revert InvalidFee();
}
StakingContractStorageLib.setOperatorFee(_operatorFee);

src/contracts/StakingContract.sol:L188-L197

function initialize_2(uint256 globalCommissionLimitBPS, uint256 operatorCommissionLimitBPS) public init(2) {
    if (globalCommissionLimitBPS > BASIS_POINTS) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setGlobalCommissionLimit(globalCommissionLimitBPS);
    if (operatorCommissionLimitBPS > BASIS_POINTS) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setOperatorCommissionLimit(operatorCommissionLimitBPS);
}

src/contracts/StakingContract.sol:L516-L522

function setGlobalFee(uint256 _globalFee) external onlyAdmin {
    if (_globalFee > StakingContractStorageLib.getGlobalCommissionLimit()) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setGlobalFee(_globalFee);
    emit ChangedGlobalFee(_globalFee);
}

src/contracts/StakingContract.sol:L506-L512

function setOperatorFee(uint256 _operatorFee) external onlyAdmin {
    if (_operatorFee > StakingContractStorageLib.getOperatorCommissionLimit()) {
        revert InvalidFee();
    }
    StakingContractStorageLib.setOperatorFee(_operatorFee);
    emit ChangedOperatorFee(_operatorFee);
}

Recommendation

The fees should be checked with a more practical limit. For instance, checking against a min - max limit, like 20% - 40%.

4.14 Proxy Design Inconsistencies Minor

Description

  1. The feature to pause/unpause the contract is available in the proxy, however, via the inherited isAdmin modifier the functions still delegate calls to the implementation, if the caller is not the Admin and the system is not paused. This can produce unintended results (including the pause function being callable in the delegate target or its fallback potentially being executable).

  2. Function isPaused() is a state-changing function but should be view.

  3. If the returndata for the call to function isPaused() is less than 32 bytes, the function will still succeed, but with a decoding error, since the function expects an abi encoded boolean.

  4. The documentation outlines that “no non-view functions can be called,” but even the view functions will become uncallable by other contracts in the paused state. image

  5. It should be avoided to implement a “hack” to allow off-chain view function calls to bypass pause mode (msg.sender == address(0)). This is highly client/library specific, and implementing bypasses for off-chain components on-chain may introduce inconsistencies and unwanted side effects.

Examples

src/contracts/TUPProxy.sol:L23-L47

function isPaused() external ifAdmin returns (bool) {
    return StorageSlot.getBooleanSlot(_PAUSE_SLOT).value;
}

/// @dev Pauses system
function pause() external ifAdmin {
    StorageSlot.getBooleanSlot(_PAUSE_SLOT).value = true;
}

/// @dev Unpauses system
function unpause() external ifAdmin {
    StorageSlot.getBooleanSlot(_PAUSE_SLOT).value = false;
}

/// @dev Overrides the fallback method to check if system is not paused before
/// @dev Address Zero is allowed to perform calls even if system is paused. This allows
/// view functions to be called when the system is paused as rpc providers can easily
/// set the sender address to zero.
function _beforeFallback() internal override {
    if (StorageSlot.getBooleanSlot(_PAUSE_SLOT).value == false || msg.sender == address(0)) {
        super._beforeFallback();
    } else {
        revert CallWhenPaused();
    }
}

4.15 Contracts Should Inherit From Their Interfaces Minor

Description

The following contracts should enforce correct interface implementation by inheriting from the interface declarations.

Examples

src/contracts/StakingContract.sol:L10-L15

/// @title Ethereum Staking Contract
/// @author Kiln
/// @notice You can use this contract to store validator keys and have users fund them and trigger deposits.
contract StakingContract {
    using StakingContractStorageLib for bytes32;

src/contracts/interfaces/IStakingContractFeeDetails.sol:L3-L20


interface IStakingContractFeeDetails {
    function getWithdrawerFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (address);

    function getTreasury() external view returns (address);

    function getOperatorFeeRecipient(bytes32 pubKeyRoot) external view returns (address);

    function getGlobalFee() external view returns (uint256);

    function getOperatorFee() external view returns (uint256);

    function getExitRequestedFromRoot(bytes32 _publicKeyRoot) external view returns (bool);

    function getWithdrawnFromPublicKeyRoot(bytes32 _publicKeyRoot) external view returns (bool);

    function toggleWithdrawnFromPublicKeyRoot(bytes32 _publicKeyRoot) external;
}

src/contracts/FeeRecipient.sol:L4-L6

import "./interfaces/IFeeDispatcher.sol";

contract FeeRecipient {

src/contracts/interfaces/IFeeRecipient.sol:L3-L8


interface IFeeRecipient {
    function init(address _dispatcher, bytes32 _publicKeyRoot) external;

    function withdraw() external;
}

Recommendation

Inherit from interface.

4.16 Misleading Error Statements Minor

Description

The contracts define custom errors to revert transactions on failed operations or invalid input, however, they convey little to no information, making it difficult for the off-chain monitoring tools to track relevant updates.

src/contracts/StakingContract.sol:L28-L51

error Forbidden();
error InvalidFee();
error Deactivated();
error NoOperators();
error InvalidCall();
error Unauthorized();
error DepositFailure();
error DepositsStopped();
error InvalidArgument();
error UnsortedIndexes();
error InvalidPublicKeys();
error InvalidSignatures();
error InvalidWithdrawer();
error InvalidZeroAddress();
error AlreadyInitialized();
error InvalidDepositValue();
error NotEnoughValidators();
error InvalidValidatorCount();
error DuplicateValidatorKey(bytes);
error FundedValidatorDeletionAttempt();
error OperatorLimitTooHigh(uint256 limit, uint256 keyCount);
error MaximumOperatorCountAlreadyReached();
error LastEditAfterSnapshot();
error PublicKeyNotInContract();

For instance, the init modifier is used to initialize the contracts with the current Version. The Version initialization ensures that the provided version must be an increment of the previous version, if not, it reverts with an error as AlreadyInitialized(). However, the error doesn’t convey an appropriate message correctly, as any version other than the expected version will signify that the version has already been initialized.

src/contracts/ConsensusLayerFeeDispatcher.sol:L37-L40

modifier init(uint256 _version) {
    if (_version != VERSION_SLOT.getUint256() + 1) {
        revert AlreadyInitialized();
    }

src/contracts/ExecutionLayerFeeDispatcher.sol:L37-L40

modifier init(uint256 _version) {
    if (_version != VERSION_SLOT.getUint256() + 1) {
        revert AlreadyInitialized();
    }

src/contracts/StakingContract.sol:L81-L84

modifier init(uint256 _version) {
    if (_version != StakingContractStorageLib.getVersion() + 1) {
        revert AlreadyInitialized();
    }

Recommendation

Use a more meaningful statement with enough information to track off-chain for all the custom errors in every contract in scope. For instance, add the current and supplied versions as indexed parameters, like: IncorrectVersionInitialization(current version, supplied version);

Also, the function can be simplified as

    function initELD(address _stakingContract) external init(VERSION_SLOT.getUint256() + 1) {
        STAKING_CONTRACT_ADDRESS_SLOT.setAddress(_stakingContract);
    }

4.17 Inconsistent Storage Slot Design

Description

Some storage slots are constructed by hashing a string as:

src/contracts/libs/StakingContractStorageLib.sol:L110-L111

bytes32 internal constant OPERATORS_SLOT = keccak256("StakingContract.operators");

while others subtract 1 from the hashed string as:

src/contracts/libs/StakingContractStorageLib.sol:L351-L352

bytes32 internal constant WITHDRAWN_MAPPING_SLOT = bytes32(uint256(keccak256("StakingContract.withdrawn")) - 1);

Recommendation

It is recommended that a consistent design should be adopted.

4.18 Opportunities for Code Improvements

Description

  • If operatorFee is 100% of globalFee, the check will still be satisfied, even though there is no reward for the treasury.

src/contracts/ConsensusLayerFeeDispatcher.sol:L99-L104

if (globalFee > 0) {
    (status, data) = treasury.call{value: globalFee - operatorFee}("");
    if (status == false) {
        revert TreasuryReceiveError(data);
    }
}

Instead the check should be if ((globalFee - operatorFee) > 0)

  • Both the conditional branches can be combined in a single check with a logical or operator to increase readability

src/contracts/StakingContract.sol:L100-L107

modifier onlyActiveOperatorOrAdmin(uint256 _operatorIndex) {
    if (msg.sender == StakingContractStorageLib.getAdmin()) {
        _;
    } else {
        _onlyActiveOperator(_operatorIndex);
        _;
    }
}
  • Instead of referencing the outer struct, the functions should reference the nested struct. For instance, StakingContractStorageLib.OperatorsSlot struct, the functions can reference the StakingContractStorageLib.OperatorInfo based on the operator’s index. This will increase the code readability, as OperatorInfo is what actually stores the operator’s information.

src/contracts/StakingContract.sol:L460-L474

StakingContractStorageLib.OperatorsSlot storage operators = StakingContractStorageLib.getOperators();
if (operators.value[_operatorIndex].deactivated) {
    revert Deactivated();
}
uint256 publicKeyCount = operators.value[_operatorIndex].publicKeys.length;
if (publicKeyCount < _limit) {
    revert OperatorLimitTooHigh(_limit, publicKeyCount);
}
if (
    operators.value[_operatorIndex].limit < _limit &&
    StakingContractStorageLib.getLastValidatorEdit() > _snapshot
) {
    revert LastEditAfterSnapshot();
}
operators.value[_operatorIndex].limit = _limit;
  • Solidity provides the feature to index calldata arrays. Hence, instead of using an external library

src/contracts/StakingContract.sol:L553-L554

bytes memory publicKey = BytesLib.slice(_publicKeys, i * PUBLIC_KEY_LENGTH, PUBLIC_KEY_LENGTH);
bytes memory signature = BytesLib.slice(_signatures, i * SIGNATURE_LENGTH, SIGNATURE_LENGTH);

the byte slicing can be done as :

 bytes calldata publicKey =_publicKeys[i * PUBLIC_KEY_LENGTH:PUBLIC_KEY_LENGTH];
 bytes calldata signature = _signatures[i * SIGNATURE_LENGTH:SIGNATURE_LENGTH];

Appendix 1 - Files in Scope

This audit covered the following files:

File SHA-1 hash
src/contracts/AuthorizedFeeRecipient.sol 5ef5304884508024c35242de6c1ba0944c9d17db
src/contracts/ConsensusLayerFeeDispatcher.sol b829573b4957dffbec7f8e816a95a5b3e3fe742b
src/contracts/ExecutionLayerFeeDispatcher.sol d442298c236ecb2814b21004c94cbc71dda338fd
src/contracts/FeeRecipient.sol 80ff6067645d7eb9ee41cbaa31e1be84fbcb49b7
src/contracts/StakingContract.sol a2887bf364aed54e96a5d0223b61f2ddfcddcc82
src/contracts/TUPProxy.sol e73fdd8194d8849162fe229ad083d9b18b3263a0
src/contracts/interfaces/IDepositContract.sol bc451bf5184a0325ca51a901ab67d601e277cd3b
src/contracts/interfaces/IFeeDispatcher.sol f987bafbfddbae39a4a13c7d52959b909c41db5e
src/contracts/interfaces/IFeeRecipient.sol 1b85e916b93e26b9b14a402abba0c4e13c830f13
src/contracts/interfaces/IStakingContractFeeDetails.sol 628d14e6e853d0965620d158b9da3c8f37f7d492
src/contracts/libs/BytesLib.sol ab4c22e18868faae26d895b6152c407203759878
src/contracts/libs/DispatchersStorageLib.sol 9ec31af30a59860b0aaa232a34e3da72e64e54b5
src/contracts/libs/StakingContractStorageLib.sol 563ee4555517391c4a95bdbc9b43ceb037e576fa

Appendix 2 - Disclosure

Consensys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via Consensys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any third party in any respect, including regarding the bug-free nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any third party by virtue of publishing these Reports.

A.2.1 Purpose of Reports

The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of code and only the code we note as being within the scope of our review within this report. Any Solidity code itself presents unique and unquantifiable risks as the Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond specified code that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. In some instances, we may perform penetration testing or infrastructure assessments depending on the scope of the particular engagement.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) on its website. CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

You may, through hypertext or other computer links, gain access to web sites operated by persons other than Consensys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites’ owners. You agree that Consensys and CD are not responsible for the content or operation of such Web sites, and that Consensys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that Consensys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. Consensys and CD assumes no responsibility for the use of third-party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

A.2.3 Timeliness of Content

The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice unless indicated otherwise, by Consensys and CD.