diff --git a/CLAUDE.md b/CLAUDE.md index 6cc4c28..85a9ec6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,8 @@ Two distinct content types used throughout the documentation: - **Conceptual articles** - Explain _why_ something exists or works. No steps. Confident and friendly tone. Structure: (1) intro, (2) what it is, (3) why it's essential, (4) related topics, (5) summary. ### Formatting Rules -- **Titles** follow sentence structure — only names and places are capitalized, e.g., `## Distributed key generation` not `## Distributed Key Generation` +- **Headings** use title case (including entries in `SUMMARY.md`), e.g., `## Distributed Key Generation`. Lowercase short function words inside multi-word headings (a, an, the, of, to, with, in, on, for, at, by, vs, and, or, but) unless they are the first word. Proper nouns and product names (e.g. *Obol*, *Charon*, *OBOL Token*, *DV Launchpad*) are capitalized everywhere. +- **Inline prose** uses sentence case — don't capitalize concept nouns mid-sentence ("distributed validator", "key share", "cluster lock file"); only proper nouns and product names. - **Bold** (`**text**`) for UI elements the reader interacts with (buttons, fields, window names) - **Italics** (`_text_`) for names of things (products, documents, concepts) - **Lists** use `-` for unordered items; each item ends with a period `.` diff --git a/README.md b/README.md index 36b1b87..6b29052 100644 --- a/README.md +++ b/README.md @@ -62,3 +62,5 @@ Whether you’re here to learn about DVT, integrate it into your staking stack, + +> **Browsing as an AI agent?** Start with [obol.org/llms.txt](https://obol.org/llms.txt) for a terse index of the ecosystem, or [obol.org/llms-full.txt](https://obol.org/llms-full.txt) for a self-contained briefing. The [`ObolNetwork/skills`](https://github.com/ObolNetwork/skills) repo publishes Claude Code skills for running DVs and the Obol Stack. diff --git a/SUMMARY.md b/SUMMARY.md index 33176a8..4088ffa 100644 --- a/SUMMARY.md +++ b/SUMMARY.md @@ -76,6 +76,7 @@ * [Self-Host a Relay](advanced-and-troubleshooting/advanced/self-relay.md) * [Advanced Docker Configs](advanced-and-troubleshooting/advanced/adv-docker-configs.md) * [Assign OVM Roles](advanced-and-troubleshooting/advanced/assign-ovm-roles.md) + * [NAT Hole Punching](advanced-and-troubleshooting/advanced/hole-punching.md) * [Troubleshooting](advanced-and-troubleshooting/troubleshooting/README.md) * [Errors & Resolutions](advanced-and-troubleshooting/troubleshooting/errors.md) * [Handling DKG Failure](advanced-and-troubleshooting/troubleshooting/dkg_failure.md) diff --git a/adv/_category_.json b/adv/_category_.json deleted file mode 100644 index 55aa807..0000000 --- a/adv/_category_.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "label": "ADVANCED & TROUBLESHOOTING", - "position": 3, - "collapsed": false, - "collapsible": false, - "className": "menuSection" -} \ No newline at end of file diff --git a/adv/advanced/_category_.json b/adv/advanced/_category_.json deleted file mode 100644 index 6547998..0000000 --- a/adv/advanced/_category_.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "label": "Advanced Guides", - "position": 1, - "collapsed": true -} \ No newline at end of file diff --git a/adv/advanced/quickstart-builder-api.mdx b/adv/advanced/quickstart-builder-api.mdx deleted file mode 100644 index b77ccb7..0000000 --- a/adv/advanced/quickstart-builder-api.mdx +++ /dev/null @@ -1,168 +0,0 @@ ---- -sidebar_position: 4 -description: Run a distributed validator cluster with the builder API (MEV-Boost) ---- - -import Tabs from '@theme/Tabs'; -import TabItem from '@theme/TabItem'; - -# Enable MEV - -This quickstart guide focuses on configuring the builder API for Charon and supported validator and consensus clients. - -## Getting started with Charon & the Builder API - -Running a distributed validator cluster with the builder API enabled will give the validators in the cluster access to the builder network. This builder network is a network of "Block Builders" -who work with MEV searchers to produce the most valuable blocks a validator can propose. - -[MEV-Boost](https://boost.flashbots.net/) is one such product from Flashbots that enables you to ask multiple -block relays (who communicate with the "Block Builders") for blocks to propose. The block that pays the largest reward to the validator will be signed and returned to the relay for broadcasting to the wider -network. The end result for the validator is generally an increased APR as they receive some share of the MEV. - -:::info -Before completing this guide, please check your cluster version, which can be found inside the `cluster-lock.json` file. If you are using cluster-lock version `1.7.0` or higher, Charon seamlessly accommodates all validator client implementations within a MEV-enabled distributed validator cluster. - -For clusters with a `cluster-lock.json` version `1.6.0` and below, Charon is compatible only with [Teku](https://github.com/ConsenSys/teku). Use the version history feature of this documentation to see the instructions for configuring a cluster in that manner (`v0.16.0`). -::: - -## Client configuration - -:::note -You need to add CLI flags to your consensus client, Charon client, and validator client, to enable the builder API. - -You need all operators in the cluster to have their nodes properly configured to use the builder API, or you risk missing a proposal. -::: - -### Charon - -Charon supports builder API with the `--builder-api` flag. To use builder API, one simply needs to add this flag to the `charon run` command: - -```shell -charon run --builder-api -``` - -### Consensus Clients - -The following flags need to be configured on your chosen consensus client. A Flashbots relay URL is provided for example purposes, you should use the [charon test mev command](../../run/prepare/test-command.mdx#test-mev-relay) and select the two or three relays with the lowest latency to your node that also conform to your block building preferences. A public list of MEV relays is available [here](https://github.com/eth-educators/ethstaker-guides/blob/main/MEV-relay-list.md#mev-relay-list-for-mainnet). - - - - Teku can communicate with a single relay directly: - 
-      
-    {String.raw`teku --builder-endpoint="https://0xac6e77dfe25ecd6110b8e780608cce0dab71fdd5ebea22a16c0205200f2f8e2e3ad3b71d3499c54ad14d6c21b41a37ae@boost-relay.flashbots.net"`}
-      
-
- Or you can configure it to communicate with a local MEV-boost sidecar to configure multiple relays: - 
-      
-    {String.raw`teku --builder-endpoint=http://mev-boost:18550`}
-      
-
- - - Lighthouse can communicate with a single relay directly: - 
-      
-    {String.raw`lighthouse bn --builder="https://0xac6e77dfe25ecd6110b8e780608cce0dab71fdd5ebea22a16c0205200f2f8e2e3ad3b71d3499c54ad14d6c21b41a37ae@boost-relay.flashbots.net"`}
-      
-
- Or you can configure it to communicate with a local MEV-boost sidecar to configure multiple relays: - 
-      
-    {String.raw`lighthouse bn --builder="http://mev-boost:18550"`}
-      
-
- - - Prysm can communicate with a single relay directly: - 
-      
-    {String.raw`prysm beacon-chain --http-mev-relay="https://0xac6e77dfe25ecd6110b8e780608cce0dab71fdd5ebea22a16c0205200f2f8e2e3ad3b71d3499c54ad14d6c21b41a37ae@boost-relay.flashbots.net"`}
-      
-
- - - Nimbus can communicate with a single relay directly: - 
-      
-        {String.raw`nimbus_beacon_node \
-        --payload-builder=true \
-        --payload-builder-url="https://0xac6e77dfe25ecd6110b8e780608cce0dab71fdd5ebea22a16c0205200f2f8e2e3ad3b71d3499c54ad14d6c21b41a37ae@boost-relay.flashbots.net"`}
-      
-
- - - Lodestar can communicate with a single relay directly: - 
-      
-    {String.raw`node ./lodestar --builder --builder.urls="https://0xac6e77dfe25ecd6110b8e780608cce0dab71fdd5ebea22a16c0205200f2f8e2e3ad3b71d3499c54ad14d6c21b41a37ae@boost-relay.flashbots.net"`}
-      
-
- - - -### Validator Clients - -The following flags need to be configured on your chosen validator client - - - - 
-      
-    {String.raw`teku validator-client --validators-builder-registration-default-enabled=true`}
-      
-
- - - - 
-      
-    {String.raw`lighthouse vc --builder-proposals`}
-      
-
- - - 
-      
-    {String.raw`prysm validator --enable-builder`}
-      
-
- - - 
-      
-    {String.raw`nimbus_validator_client --payload-builder=true`}
-      
-
- - - 
-      
-    {String.raw`node ./lodestar validator --builder="true" --builder.selection="builderalways"`}
-      
-
- - - -:::info -For at-scale deployments, additional flags should be configured on the consensus or validator client to ensure builder bids are preferred over locally-built blocks whenever available. See [Builder Block Selection](../../run/prepare/deployment-best-practices.mdx#builder-block-selection) in the deployment best practices for the recommended flag per client. -::: - -## Verify your cluster is correctly configured - -It can be difficult to confirm everything is configured correctly with your cluster until a proposal opportunity arrives, but here are some things you can check. - -When your cluster is running, you should see if Charon is logging something like this each epoch: - -```log -13:10:47.094 INFO bcast Successfully submitted validator registration to beacon node {"delay": "24913h10m12.094667699s", "pubkey": "84b_713", "duty": "1/builder_registration"} -``` - -This indicates that your Charon node is successfully registering with the relay for a blinded block when the time comes. - -If you are using the [ultrasound relay](https://relay.ultrasound.money), you can enter your cluster's distributed validator public key(s) into their website, to confirm they also see the validator as correctly registered. - -You should check that your validator client's logs look healthy, and ensure that you haven't added a `fee-recipient` address that conflicts with what has been selected by your cluster in your `cluster-lock.json` file, as that may prevent your validator from producing a signature for the block when the opportunity arises. You should also confirm the same for all of the other peers in your cluster. - -Once a proposal has been made, you should look at the `Block Extra Data` field under `Execution Payload` for the block on [Beaconcha.in](https://beaconcha.in/block/18450364), and confirm there is text present, this generally suggests the block came from a builder, and was not a locally constructed block. diff --git a/adv/advanced/quickstart-sdk.mdx b/adv/advanced/quickstart-sdk.mdx deleted file mode 100644 index 6e6395d..0000000 --- a/adv/advanced/quickstart-sdk.mdx +++ /dev/null @@ -1,129 +0,0 @@ ---- -sidebar_position: 2 -description: Create a DV cluster using the Obol Typescript SDK ---- - -import Tabs from '@theme/Tabs'; -import TabItem from '@theme/TabItem'; - -# Create a DV Using the SDK - -This is a walkthrough of using the [Obol-SDK](https://www.npmjs.com/package/@obolnetwork/obol-sdk) to propose a four-node distributed validator cluster for creation using the [DV Launchpad](../../learn/intro/launchpad.md). - -## Pre-requisites - -- You have [node.js](https://nodejs.org/en) installed. - -## Install the package - -Install the Obol-SDK package into your development environment - - - - 
-      npm install --save @obolnetwork/obol-sdk
-
- - - 
-      yarn add @obolnetwork/obol-sdk
-
- - - -## Instantiate the client - -The first thing you need to do is create an instance of the Obol SDK client. The client takes two constructor parameters: - -- The `chainID` for the chain you intend to use. -- An ethers.js [signer](https://docs.ethers.org/v6/api/providers/#Signer-signTypedData) object. - -```ts -import { Client } from "@obolnetwork/obol-sdk"; -import { ethers } from "ethers"; - -// Create a dummy ethers signer object with a throwaway private key -const mnemonic = ethers.Wallet.createRandom().mnemonic?.phrase || ""; -const privateKey = ethers.Wallet.fromPhrase(mnemonic).privateKey; -const wallet = new ethers.Wallet(privateKey); -const signer = wallet.connect(null); - -// Instantiate the Obol Client for Hoodi -const obol = new Client({ chainId: 560048 }, signer); -``` - -## Propose the cluster - -List the Ethereum addresses of participating operators, along with withdrawal and fee recipient address data for each validator you intend for the operators to create. - -```ts -// A config hash is a deterministic hash of the proposed DV cluster configuration -const configHash = await obol.createClusterDefinition({ - name: "SDK Demo Cluster", - operators: [ - { address: "0xC35CfCd67b9C27345a54EDEcC1033F2284148c81" }, - { address: "0x33807D6F1DCe44b9C599fFE03640762A6F08C496" }, - { address: "0xc6e76F72Ea672FAe05C357157CfC37720F0aF26f" }, - { address: "0x86B8145c98e5BD25BA722645b15eD65f024a87EC" }, - ], - validators: [ - { - fee_recipient_address: "0x3CD4958e76C317abcEA19faDd076348808424F99", - withdrawal_address: "0xE0C5ceA4D3869F156717C66E188Ae81C80914a6e", - }, - ], -}); - -console.log( - `Direct the operators to https://hoodi.launchpad.obol.org/dv?configHash=${configHash} to complete the key generation process` -); -``` - -## Invite the Operators to complete the DKG - -Once the Obol-API returns a `configHash` string from the `createClusterDefinition` method, you can use this identifier to invite the operators to the [Launchpad](../../learn/intro/launchpad.md) to complete the process - -1. Operators navigate to `https://.launchpad.obol.org/dv?configHash=` and complete the [run a DV with others](../../run-a-dv/start/create-a-dv-with-a-group.md) flow. -1. Once the DKG is complete, and operators are using the `--publish` flag, the created cluster details will be posted to the Obol API. -1. The creator will be able to retrieve this data with `obol.getClusterLock(configHash)`, to use for activating the newly created validator. - -## Retrieve the created Distributed Validators using the SDK - -Once the DKG is complete, the proposer of the cluster can retrieve key data such as the validator public keys and their associated deposit data messages. - -```js -const clusterLock = await obol.getClusterLock(configHash); -``` - -Reference lock files can be found [here](https://github.com/ObolNetwork/charon/tree/main/cluster/testdata). - -## Activate the DVs using the deposit contract - -In order to activate the distributed validators, the cluster operator can retrieve the validators' associated deposit data from the lock file and use it to craft transactions to the `deposit()` method on the deposit contract. - -```js -const validatorDepositData = - clusterLock.distributed_validators[validatorIndex].deposit_data; - -const depositContract = new ethers.Contract( - DEPOSIT_CONTRACT_ADDRESS, // 0x00000000219ab540356cBB839Cbe05303d7705Fa for Mainnet, 0xff50ed3d0ec03aC01D4C79aAd74928BFF48a7b2b for Goerli - depositContractABI, // https://etherscan.io/address/0x00000000219ab540356cBB839Cbe05303d7705Fa#code for Mainnet, and replace the address for Goerli - signer -); - -const TX_VALUE = ethers.parseEther("32"); - -const tx = await depositContract.deposit( - validatorDepositData.pubkey, - validatorDepositData.withdrawal_credentials, - validatorDepositData.signature, - validatorDepositData.deposit_data_root, - { value: TX_VALUE } -); - -const txResult = await tx.wait(); -``` - -## Usage Examples - -Examples of how our SDK can be used are found [here](https://github.com/ObolNetwork/obol-sdk-examples). diff --git a/adv/security/_category_.json b/adv/security/_category_.json deleted file mode 100644 index f445b9f..0000000 --- a/adv/security/_category_.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "label": "Security", - "position": 8, - "collapsed": true -} diff --git a/adv/security/smart_contract_audit.mdx b/adv/security/smart_contract_audit.mdx deleted file mode 100644 index 15d4528..0000000 --- a/adv/security/smart_contract_audit.mdx +++ /dev/null @@ -1,495 +0,0 @@ ---- -sidebar_position: 4 -description: Smart Contract Audit ---- - -# Smart Contract Audit - - - - - - - -
-

Obol Audit Report

-

Obol Manager Contracts

-

Prepared by: Zach Obront, Independent Security Researcher

-

Date: Sept 18 to 22, 2023

-

PDF Version

-
- -## About **Obol** - -The Obol Network is an ecosystem for trust minimized staking that enables people to create, test, run & co-ordinate distributed validators. - -The Obol Manager contracts are responsible for distributing validator rewards and withdrawals among the validator and node operators involved in a distributed validator. - -## About **zachobront** - -Zach Obront is an independent smart contract security researcher. He serves as a Lead Senior Watson at Sherlock, a Security Researcher at Spearbit, and has identified multiple critical severity bugs in the wild, including in a Top 5 Protocol on Immunefi. You can say hi on Twitter at [@zachobront](http://x.com/zachobront). - -## Summary & Scope - -The [ObolNetwork/obol-manager-contracts](https://github.com/ObolNetwork/obol-manager-contracts/) repository was audited at commit [50ce277919723c80b96f6353fa8d1f8facda6e0e](https://github.com/ObolNetwork/obol-manager-contracts/tree/50ce277919723c80b96f6353fa8d1f8facda6e0e). - -The following contracts were in scope: - -- src/controllers/ImmutableSplitController.sol -- src/controllers/ImmutableSplitControllerFactory.sol -- src/lido/LidoSplit.sol -- src/lido/LidoSplitFactory.sol -- src/owr/OptimisticWithdrawalReceiver.sol -- src/owr/OptimisticWithdrawalReceiverFactory.sol - -After completion of the fixes, the [2f4f059bfd145f5f05d794948c918d65d222c3a9](https://github.com/ObolNetwork/obol-manager-contracts/tree/2f4f059bfd145f5f05d794948c918d65d222c3a9) commit was reviewed. After this review, the updated Lido fee share system in [PR #96](https://github.com/ObolNetwork/obol-manager-contracts/pull/96/files) (at commit [fd244a05f964617707b0a40ebb11b523bbd683b8](https://github.com/ObolNetwork/obol-splits/pull/96/commits/fd244a05f964617707b0a40ebb11b523bbd683b8)) was reviewed. - -## Summary of Findings - -| Identifier | Title | Severity | Fixed | -| :------: | ---------------------------- | :-------------: | :-----: | -| [M-01](#m-01-future-fees-may-be-skirted-by-setting-a-non-eth-reward-token) | Future fees may be skirted by setting a non-ETH reward token | Medium | ✓ | -| [M-02](#m-02-splits-with-256-or-more-node-operators-will-not-be-able-to-switch-on-fees) | Splits with 256 or more node operators will not be able to switch on fees | Medium | ✓ | -| [M-03](#m-03-in-a-mass-slashing-event-node-operators-are-incentivized-to-get-slashed) | In a mass slashing event, node operators are incentivized to get slashed | Medium | | -| [L-01](#l-01-obol-fees-will-be-applied-retroactively-to-all-non-distributed-funds-in-the-splitter) | Obol fees will be applied retroactively to all non-distributed funds in the Splitter | Low | ✓ | -| [L-02](#l-02-if-owr-is-used-with-rebase-tokens-and-theres-a-negative-rebase-principal-can-be-lost) | If OWR is used with rebase tokens and there's a negative rebase, principal can be lost | Low | ✓ | -| [L-03](#l-03-lidosplit-can-receive-eth-which-will-be-locked-in-contract) | LidoSplit can receive ETH, which will be locked in contract | Low | ✓ | -| [L-04](#l-04-upgrade-to-latest-version-of-solady-to-fix-libclone-bug) | Upgrade to latest version of Solady to fix LibClone bug | Low | ✓ | -| [G-01](#g-01-steth-and-wsteth-addresses-can-be-saved-on-implementation-to-save-gas) | stETH and wstETH addresses can be saved on implementation to save gas | Gas | ✓ | -| [G-02](#g-02-owr-can-be-simplified-and-save-gas-by-not-tracking-distributedfunds) | OWR can be simplified and save gas by not tracking distributedFunds | Gas | ✓ | -| [I-01](#i-01-strong-trust-assumptions-between-validators-and-node-operators) | Strong trust assumptions between validators and node operators | Informational | | -| [I-02](#i-02-provide-node-operator-checklist-to-validate-setup) | Provide node operator checklist to validate setup | Informational | | - -## Detailed Findings - -### [M-01] Future fees may be skirted by setting a non-ETH reward token - -Fees are planned to be implemented on the `rewardRecipient` splitter by updating to a new fee structure using the `ImmutableSplitController`. - -It is assumed that all rewards will flow through the splitter, because (a) all distributed rewards less than 16 ETH are sent to the `rewardRecipient`, and (b) even if a team waited for rewards to be greater than 16 ETH, rewards sent to the `principalRecipient` are capped at the `amountOfPrincipalStake`. - -This creates a fairly strong guarantee that reward funds will flow to the `rewardRecipient`. Even if a user were to set their `amountOfPrincipalStake` high enough that the `principalRecipient` could receive unlimited funds, the Obol team could call `distributeFunds()` when the balance got near 16 ETH to ensure fees were paid. - -However, if the user selects a non-ETH token, all ETH will be withdrawable only thorugh the `recoverFunds()` function. If they set up a split with their node operators as their `recoveryAddress`, all funds will be withdrawable via `recoverFunds()` without ever touching the `rewardRecipient` or paying a fee. - -#### Recommendation - -I would recommend removing the ability to use a non-ETH token from the `OptimisticWithdrawalRecipient`. Alternatively, if it feels like it may be a use case that is needed, it may make sense to always include ETH as a valid token, in addition to any `OWRToken` set. - -#### Review - -Fixed in [PR 85](https://github.com/ObolNetwork/obol-manager-contracts/pull/85) by removing the ability to use non-ETH tokens. - -### [M-02] Splits with 256 or more node operators will not be able to switch on fees - -0xSplits is used to distribute rewards across node operators. All Splits are deployed with an ImmutableSplitController, which is given permissions to update the split one time to add a fee for Obol at a future date. - -The Factory deploys these controllers as Clones with Immutable Args, hard coding the `owner`, `accounts`, `percentAllocations`, and `distributorFee` for the future update. This data is packed as follows: - -```solidity - function _packSplitControllerData( - address owner, - address[] calldata accounts, - uint32[] calldata percentAllocations, - uint32 distributorFee - ) internal view returns (bytes memory data) { - uint256 recipientsSize = accounts.length; - uint256[] memory recipients = new uint[](recipientsSize); - - uint256 i = 0; - for (; i < recipientsSize;) { - recipients[i] = (uint256(percentAllocations[i]) << ADDRESS_BITS) | uint256(uint160(accounts[i])); - - unchecked { - i++; - } - } - - data = abi.encodePacked(splitMain, distributorFee, owner, uint8(recipientsSize), recipients); - } -``` - -In the process, `recipientsSize` is unsafely downcasted into a `uint8`, which has a maximum value of `256`. As a result, any values greater than 256 will overflow and result in a lower value of `recipients.length % 256` being passed as `recipientsSize`. - -When the Controller is deployed, the full list of `percentAllocations` is passed to the `validSplit` check, which will pass as expected. However, later, when `updateSplit()` is called, the `getNewSplitConfiguation()` function will only return the first `recipientsSize` accounts, ignoring the rest. - -```solidity - function getNewSplitConfiguration() - public - pure - returns (address[] memory accounts, uint32[] memory percentAllocations) - { - // fetch the size first - // then parse the data gradually - uint256 size = _recipientsSize(); - accounts = new address[](size); - percentAllocations = new uint32[](size); - - uint256 i = 0; - for (; i < size;) { - uint256 recipient = _getRecipient(i); - accounts[i] = address(uint160(recipient)); - percentAllocations[i] = uint32(recipient >> ADDRESS_BITS); - unchecked { - i++; - } - } - } -``` - -When `updateSplit()` is eventually called on `splitsMain` to turn on fees, the `validSplit()` check on that contract will revert because the sum of the percent allocations will no longer sum to `1e6`, and the update will not be possible. - -#### Proof of Concept - -The following test can be dropped into a file in `src/test` to demonstrate that passing 400 accounts will result in a `recipientSize` of `400 - 256 = 144`: - -```solidity -// SPDX-License-Identifier: MIT -pragma solidity ^0.8.0; - -import { Test } from "forge-std/Test.sol"; -import { console } from "forge-std/console.sol"; -import { ImmutableSplitControllerFactory } from "src/controllers/ImmutableSplitControllerFactory.sol"; -import { ImmutableSplitController } from "src/controllers/ImmutableSplitController.sol"; - -interface ISplitsMain { - function createSplit(address[] calldata accounts, uint32[] calldata percentAllocations, uint32 distributorFee, address controller) external returns (address); -} - -contract ZachTest is Test { - function testZach_RecipientSizeCappedAt256Accounts() public { - vm.createSelectFork("https://mainnet.infura.io/v3/fb419f740b7e401bad5bec77d0d285a5"); - - ImmutableSplitControllerFactory factory = new ImmutableSplitControllerFactory(address(9999)); - bytes32 deploymentSalt = keccak256(abi.encodePacked(uint256(1102))); - address owner = address(this); - - address[] memory bigAccounts = new address[](400); - uint32[] memory bigPercentAllocations = new uint32[](400); - - for (uint i = 0; i < 400; i++) { - bigAccounts[i] = address(uint160(i)); - bigPercentAllocations[i] = 2500; - } - - // confirmation that 0xSplits will allow creating a split with this many accounts - // dummy acct passed as controller, but doesn't matter for these purposes - address split = ISplitsMain(0x2ed6c4B5dA6378c7897AC67Ba9e43102Feb694EE).createSplit(bigAccounts, bigPercentAllocations, 0, address(8888)); - - ImmutableSplitController controller = factory.createController(split, owner, bigAccounts, bigPercentAllocations, 0, deploymentSalt); - - // added a public function to controller to read recipient size directly - uint savedRecipientSize = controller.ZachTest__recipientSize(); - assert(savedRecipientSize < 400); - console.log(savedRecipientSize); // 144 - } -} -``` - -#### Recommendation - -When packing the data in `_packSplitControllerData()`, check `recipientsSize` before downcasting to a uint8: - -```diff -function _packSplitControllerData( - address owner, - address[] calldata accounts, - uint32[] calldata percentAllocations, - uint32 distributorFee -) internal view returns (bytes memory data) { - uint256 recipientsSize = accounts.length; -+ if (recipientsSize > 256) revert InvalidSplit__TooManyAccounts(recipientSize); - ... -} -``` - -#### Review - -Fixed as recommended in [PR 86](https://github.com/ObolNetwork/obol-manager-contracts/pull/86). - -### [M-03] In a mass slashing event, node operators are incentivized to get slashed - -When the `OptimisticWithdrawalRecipient` receives funds from the beacon chain, it uses the following rule to determine the allocation: - -> If the amount of funds to be distributed is greater than or equal to 16 ether, it is assumed that it is a withdrawal (to be returned to the principal, with a cap on principal withdrawals of the total amount they deposited). - -> Otherwise, it is assumed that the funds are rewards. - -This value being as low as 16 ether protects against any predictable attack the node operator could perform. For example, due to the effect of hysteresis in updating effective balances, it does not seem to be possible for node operators to predictably bleed a withdrawal down to be below 16 ether (even if they timed a slashing perfectly). - -However, in the event of a mass slashing event, slashing punishments can be much more severe than they otherwise would be. To calculate the size of a slash, we: - -- take the total percentage of validator stake slashed in the 18 days preceding and following a user's slash -- multiply this percentage by 3 (capped at 100%) -- the full slashing penalty for a given validator equals 1/32 of their stake, plus the resulting percentage above applied to the remaining 31/32 of their stake - -In order for such penalties to bring the withdrawal balance below 16 ether (assuming a full 32 ether to start), we would need the percentage taken to be greater than `15 / 31 = 48.3%`, which implies that `48.3 / 3 = 16.1%` of validators would need to be slashed. - -Because the measurement is taken from the 18 days before and after the incident, node operators would have the opportunity to see a mass slashing event unfold, and later decide that they would like to be slashed along with it. - -In the event that they observed that greater than 16.1% of validators were slashed, Obol node operators would be able to get themselves slashed, be exited with a withdrawal of less than 16 ether, and claim that withdrawal as rewards, effectively stealing from the principal recipient. - -#### Recommendations - -Find a solution that provides a higher level of guarantee that the funds withdrawn are actually rewards, and not a withdrawal. - -#### Review - -Acknowledged. We believe this is a black swan event. It would require a major ETH client to be compromised, and would be a betrayal of trust, so likely not EV+ for doxxed operators. Users of this contract with unknown operators should be wary of such a risk. - -### [L-01] Obol fees will be applied retroactively to all non-distributed funds in the Splitter - -When Obol decides to turn on fees, a call will be made to `ImmutableSplitController::updateSplit()`, which will take the predefined split parameters (the original user specified split with Obol's fees added in) and call `updateSplit()` to implement the change. - -```solidity -function updateSplit() external payable { - if (msg.sender != owner()) revert Unauthorized(); - - (address[] memory accounts, uint32[] memory percentAllocations) = getNewSplitConfiguration(); - - ISplitMain(splitMain()).updateSplit(split, accounts, percentAllocations, uint32(distributorFee())); -} -``` - -If we look at the code on `SplitsMain`, we can see that this `updateSplit()` function is applied retroactively to all funds that are already in the split, because it updates the parameters without performing a distribution first: - -```solidity -function updateSplit( - address split, - address[] calldata accounts, - uint32[] calldata percentAllocations, - uint32 distributorFee -) - external - override - onlySplitController(split) - validSplit(accounts, percentAllocations, distributorFee) -{ - _updateSplit(split, accounts, percentAllocations, distributorFee); -} -``` - -This means that any funds that have been sent to the split but have not yet be distributed will be subject to the Obol fee. Since these splitters will be accumulating all execution layer fees, it is possible that some of them may have received large MEV bribes, where this after-the-fact fee could be quite expensive. - -#### Recommendation - -The most strict solution would be for the `ImmutableSplitController` to store both the old split parameters and the new parameters. The old parameters could first be used to call `distributeETH()` on the split, and then `updateSplit()` could be called with the new parameters. - -If storing both sets of values seems too complex, the alternative would be to require that `split.balance <= 1` to update the split. Then the Obol team could simply store the old parameters off chain to call `distributeETH()` on each split to "unlock" it to update the fees. - -(Note that for the second solution, the ETH balance should be less than or equal to 1, not 0, because 0xSplits stores empty balances as `1` for gas savings.) - -#### Review - -Fixed as recommended in [PR 86](https://github.com/ObolNetwork/obol-manager-contracts/pull/86). - -### [L-02] If OWR is used with rebase tokens and there's a negative rebase, principal can be lost - -The `OptimisticWithdrawalRecipient` is deployed with a specific token immutably set on the clone. It is presumed that that token will usually be ETH, but it can also be an ERC20 to account for future integrations with tokenized versions of ETH. - -In the event that one of these integrations used a rebasing version of ETH (like `stETH`), the architecture would need to be set up as follows: - -`OptimisticWithdrawalRecipient => rewards to something like LidoSplit.sol => Split Wallet` - -In this case, the OWR would need to be able to handle rebasing tokens. - -In the event that rebasing tokens are used, there is the risk that slashing or inactivity leads to a period with a negative rebase. In this case, the following chain of events could happen: - -- `distribute(PULL)` is called, setting `fundsPendingWithdrawal == balance` -- rebasing causes the balance to decrease slightly -- `distribute(PULL)` is called again, so when `fundsToBeDistributed = balance - fundsPendingWithdrawal` is calculated in an unchecked block, it ends up being near `type(uint256).max` -- since this is more than `16 ether`, the first `amountOfPrincipalStake - _claimedPrincipalFunds` will be allocated to the principal recipient, and the rest to the reward recipient -- we check that `endingDistributedFunds <= type(uint128).max`, but unfortunately this check misses the issue, because only `fundsToBeDistributed` underflows, not `endingDistributedFunds` -- `_claimedPrincipalFunds` is set to `amountOfPrincipalStake`, so all future claims will go to the reward recipient -- the `pullBalances` for both recipients will be set higher than the balance of the contract, and so will be unusable - -In this situation, the only way for the principal to get their funds back would be for the full `amountOfPrincipalStake` to hit the contract at once, and for them to call `withdraw()` before anyone called `distribute(PUSH)`. If anyone was to be able to call `distribute(PUSH)` before them, all principal would be sent to the reward recipient instead. - -#### Recommendation - -Similar to #74, I would recommend removing the ability for the `OptimisticWithdrawalRecipient` to accept non-ETH tokens. - -Otherwise, I would recommend two changes for redundant safety: - -1) Do not allow the OWR to be used with rebasing tokens. - -2) Move the `_fundsToBeDistributed = _endingDistributedFunds - _startingDistributedFunds;` out of the unchecked block. The case where `_endingDistributedFunds` underflows is already handled by a later check, so this one change should be sufficient to prevent any risk of this issue. - -#### Review - -Fixed in [PR 85](https://github.com/ObolNetwork/obol-manager-contracts/pull/85) by removing the ability to use non-ETH tokens. - -### [L-03] LidoSplit can receive ETH, which will be locked in contract - -Each new `LidoSplit` is deployed as a clone, which comes with a `receive()` function for receiving ETH. - -However, the only function on `LidoSplit` is `distribute()`, which converts `stETH` to `wstETH` and transfers it to the `splitWallet`. - -While this contract should only be used for Lido to pay out rewards (which will come in `stETH`), it seems possible that users may accidentally use the same contract to receive other validator rewards (in ETH), or that Lido governance may introduce ETH payments in the future, which would cause the funds to be locked. - -#### Proof of Concept - -The following test can be dropped into `LidoSplit.t.sol` to confirm that the clones can currently receive ETH: - -```solidity -function testZach_CanReceiveEth() public { - uint before = address(lidoSplit).balance; - payable(address(lidoSplit)).transfer(1 ether); - assertEq(address(lidoSplit).balance, before + 1 ether); -} -``` - -#### Recommendation - -Introduce an additional function to `LidoSplit.sol` which wraps ETH into stETH before calling `distribute()`, in order to rescue any ETH accidentally sent to the contract. - -#### Review - -Fixed in [PR 87](https://github.com/ObolNetwork/obol-manager-contracts/pull/87/files) by adding a `rescueFunds()` function that can send ETH or any ERC20 (except `stETH` or `wstETH`) to the `splitWallet`. - -### [L-04] Upgrade to latest version of Solady to fix LibClone bug - -In the recent [Solady audit](https://github.com/Vectorized/solady/blob/main/audits/cantina-solady-report.pdf), an issue was found the affects LibClone. - -In short, LibClone assumes that the length of the immutable arguments on the clone will fit in 2 bytes. If it's larger, it overlaps other op codes and can lead to strange behaviors, including causing the deployment to fail or causing the deployment to succeed with no resulting bytecode. - -Because the `ImmutableSplitControllerFactory` allows the user to input arrays of any length that will be encoded as immutable arguments on the Clone, we can manipulate the length to accomplish these goals. - -Fortunately, failed deployments or empty bytecode (which causes a revert when `init()` is called) are not problems in this case, as the transactions will fail, and it can only happen with unrealistically long arrays that would only be used by malicious users. - -However, it is difficult to be sure how else this risk might be exploited by using the overflow to jump to later op codes, and it is recommended to update to a newer version of Solady where the issue has been resolved. - -#### Proof of Concept - -If we comment out the `init()` call in the `createController()` call, we can see that the following test "successfully" deploys the controller, but the result is that there is no bytecode: - -```solidity -function testZach__CreateControllerSoladyBug() public { - ImmutableSplitControllerFactory factory = new ImmutableSplitControllerFactory(address(9999)); - bytes32 deploymentSalt = keccak256(abi.encodePacked(uint256(1102))); - address owner = address(this); - - address[] memory bigAccounts = new address[](28672); - uint32[] memory bigPercentAllocations = new uint32[](28672); - - for (uint i = 0; i < 28672; i++) { - bigAccounts[i] = address(uint160(i)); - if (i < 32) bigPercentAllocations[i] = 820; - else bigPercentAllocations[i] = 34; - } - - ImmutableSplitController controller = factory.createController(address(8888), owner, bigAccounts, bigPercentAllocations, 0, deploymentSalt); - assert(address(controller) != address(0)); - assert(address(controller).code.length == 0); -} -``` - -#### Recommendation - -Delete Solady and clone it from the most recent commit, or any commit after the fixes from [PR #548](https://github.com/Vectorized/solady/pull/548/files#diff-27a3ba4730de4b778ecba4697ab7dfb9b4f30f9e3666d1e5665b194fe6c9ae45) were merged. - -#### Review - -Solady has been updated to v.0.0.123 in [PR 88](https://github.com/ObolNetwork/obol-manager-contracts/pull/88). - -### [G-01] stETH and wstETH addresses can be saved on implementation to save gas - -The `LidoSplitFactory` contract holds two immutable values for the addresses of the `stETH` and `wstETH` tokens. - -When new clones are deployed, these values are encoded as immutable args. This adds the values to the contract code of the clone, so that each time a call is made, they are passed as calldata along to the implementation, which reads the values from the calldata for use. - -Since these values will be consistent across all clones on the same chain, it would be more gas efficient to store them in the implementation directly, which can be done with `immutable` storage values, set in the constructor. - -This would save 40 bytes of calldata on each call to the clone, which leads to a savings of approximately 640 gas on each call. - -#### Recommendation - -1) Add the following to `LidoSplit.sol`: - -```solidity -address immutable public stETH; -address immutable public wstETH; -``` - -2) Add a constructor to `LidoSplit.sol` which sets these immutable values. Solidity treats immutable values as constants and stores them directly in the contract bytecode, so they will be accessible from the clones. - -3) Remove `stETH` and `wstETH` from `LidoSplitFactory.sol`, both as storage values, arguments to the constructor, and arguments to `clone()`. - -4) Adjust the `distribute()` function in `LidoSplit.sol` to read the storage values for these two addresses, and remove the helper functions to read the clone's immutable arguments for these two values. - -#### Review - -Fixed as recommended in [PR 87](https://github.com/ObolNetwork/obol-manager-contracts/pull/87). - -### [G-02] OWR can be simplified and save gas by not tracking distributedFunds - -Currently, the `OptimisticWithdrawalRecipient` contract tracks four variables: - -- distributedFunds: total amount of the token distributed via push or pull -- fundsPendingWithdrawal: total balance distributed via pull that haven't been claimed yet -- claimedPrincipalFunds: total amount of funds claimed by the principal recipient -- pullBalances: individual pull balances that haven't been claimed yet - -When `_distributeFunds()` is called, we perform the following math (simplified to only include relevant updates): - -```solidity -endingDistributedFunds = distributedFunds - fundsPendingWithdrawal + currentBalance; -fundsToBeDistributed = endingDistributedFunds - distributedFunds; -distributedFunds = endingDistributedFunds; -``` - -As we can see, `distributedFunds` is added to the `endingDistributedFunds` variable and then removed when calculating `fundsToBeDistributed`, having no impact on the resulting `fundsToBeDistributed` value. - -The `distributedFunds` variable is not read or used anywhere else on the contract. - -#### Recommendation - -We can simplify the math and save substantial gas (a storage write plus additional operations) by not tracking this value at all. - -This would allow us to calculate `fundsToBeDistributed` directly, as follows: - -```solidity -fundsToBeDistributed = currentBalance - fundsPendingWithdrawal; -``` - -#### Review - -Fixed as recommended in [PR 85](https://github.com/ObolNetwork/obol-manager-contracts/pull/85). - -### [I-01] Strong trust assumptions between validators and node operators - -It is assumed that validators and node operators will always act in the best interest of the group, rather than in their selfish best interest. - -It is important to make clear to users that there are strong trust assumptions between the various parties involved in the DVT. - -Here are a select few examples of attacks that a malicious set of node operators could perform: - -1) Since there is currently no mechanism for withdrawals besides the consensus of the node operators, a minority of them sufficient to withhold consensus could blackmail the principal for a payment of up to 16 ether in order to allow them to withdraw. Otherwise, they could turn off their node operators and force the principal to bleed down to a final withdrawn balance of just over 16 ether. - -2) Node operators are all able to propose blocks within the P2P network, which are then propogated out to the rest of the network. Node software is accustomed to signing for blocks built by block builders based on the metadata including quantity of fees and the address they'll be sent to. This is enforced by social consensus, with block builders not wanting to harm validators in order to have their blocks accepted in the future. However, node operators in a DVT are not concerned with the social consensus of the network, and could therefore build blocks that include large MEV payments to their personal address (instead of the DVT's 0xSplit), add fictious metadata to the block header, have their fellow node operators accept the block, and take the MEV for themselves. - -3) While the withdrawal address is immutably set on the beacon chain to the OWR, the fee address is added by the nodes to each block. Any majority of node operators sufficient to reach consensus could create a new 0xSplit with only themselves on it, and use that for all execution layer fees. The principal (and other node operators) would not be able to stop them or withdraw their principal, and would be stuck with staked funds paying fees to the malicious node operators. - -Note that there are likely many other possible attacks that malicious node operators could perform. This report is intended to demonstrate some examples of the trust level that is needed between validators and node operators, and to emphasize the importance of making these assumptions clear to users. - -#### Review - -Acknowledged. We believe EIP 7002 will reduce this trust assumption as it would enable the validator exit via the execution layer withdrawal key. - -### [I-02] Provide node operator checklist to validate setup - -There are a number of ways that the user setting up the DVT could plant backdoors to harm the other users involved in the DVT. - -Each of these risks is possible to check before signing off on the setup, but some are rather hidden, so it would be useful for the protocol to provide a list of checks that node operators should do before signing off on the setup parameters (or, even better, provide these checks for them through the front end). - -1) Confirm that `SplitsMain.getHash(split)` matches the hash of the parameters that the user is expecting to be used. - -2) Confirm that the controller clone delegates to the correct implementation. If not, it could be pointed to delegate to `SplitMain` and then called to `transferControl()` to a user's own address, allowing them to update the split arbitrarily. - -3) `OptimisticWithdrawalRecipient.getTranches()` should be called to check that `amountOfPrincipalStake` is equal to the amount that they will actually be providing. - -4) The controller's `owner` and future split including Obol fees should be provided to the user. They should be able to check that `ImmutableSplitControllerFactory.predictSplitControllerAddress()`, with those parameters inputted, results in the controller that is actually listed on `SplitsMain.getController(split)`. - -#### Review - -Acknowledged. We do some of these already (will add the remainder) automatically in the launchpad UI during the cluster confirmation phase by the node operator. We will also add it in markdown to the repo. diff --git a/adv/troubleshooting/_category_.json b/adv/troubleshooting/_category_.json deleted file mode 100644 index 9b64570..0000000 --- a/adv/troubleshooting/_category_.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "label": "Troubleshooting", - "position": 2, - "collapsed": true -} diff --git a/advanced-and-troubleshooting/advanced/assign-ovm-roles.md b/advanced-and-troubleshooting/advanced/assign-ovm-roles.md index a9adbd6..fd5bbc6 100644 --- a/advanced-and-troubleshooting/advanced/assign-ovm-roles.md +++ b/advanced-and-troubleshooting/advanced/assign-ovm-roles.md @@ -50,17 +50,17 @@ Roles are assigned using the **`grantRoles`** function on the OVM smart contract - **`roles (uint256)`:** Input the **Decimal Value** (e.g., `17`) or **Hex Value** (e.g., `0x11`) copied from the calculator. - Click **"Write"** and approve the transaction. -
+
Screenshot of the OVM contract on a block explorer with the role-assignment transaction prepared.
## 3. Review the Roles 1. Assigned roles will show up in the Launchpad to the designated address. -
+
Screenshot of the DV Launchpad showing newly assigned OVM roles.
2. Sometimes the Launchpad may take a short time to reflect role updates due to RPC issues.. Try refreshing if this occurs. You can also use Etherscan directly to confirm roles. -
+
Screenshot of the DV Launchpad reflecting updated OVM role assignments.
## 4. Security and Recommendations 🔒 @@ -79,7 +79,7 @@ The security of the cluster relies entirely on the assignment and control of the - **Cluster Creation Timing:** It is recommended to **grant final roles before sharing cluster invites** with external invitees. This ensures the security model is locked down before the cluster scales. - **Renounce Ownership (Conditional):** If the cluster's roles are intended to be fixed forever (e.g., in a fully immutable system), you can **renounce ownership** after setting the final roles. However, if any role needs to be modifiable later (like changing the fee recipient), the owner must retain the ability to execute `grantRoles`. -
+
Screenshot of the DV Launchpad showing the OVM ownership and role-renouncement options.
## 5. Miscellaneous: How does Bitwise Logic Work? diff --git a/advanced-and-troubleshooting/advanced/ovm-predeploy.md b/advanced-and-troubleshooting/advanced/ovm-predeploy.md index 4b4be0c..25283fa 100644 --- a/advanced-and-troubleshooting/advanced/ovm-predeploy.md +++ b/advanced-and-troubleshooting/advanced/ovm-predeploy.md @@ -16,7 +16,7 @@ This guide will demonstrate the key steps in preparing a DV cluster for this typ The Hoodi testnet will be used for all examples. -
+
Diagram of the on-demand Obol Validator Manager pre-deploy workflow.
{% hint style="warning" %} The following code snippets are minimal examples for the purpose of achieving the desired functionality. These should not be run in production without thorough testing and review. @@ -1075,7 +1075,7 @@ console.log("Funds distributed to beneficiary and reward recipient");