Smart Contract Audit
Last updated
Last updated
Prepared by: Zach Obront, Independent Security Researcher
Date: Sept 18 to 22, 2023
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.
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 .
The repository was audited at commit .
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
Future fees may be skirted by setting a non-ETH reward token
Medium
✓
Splits with 256 or more node operators will not be able to switch on fees
Medium
✓
In a mass slashing event, node operators are incentivized to get slashed
Medium
Obol fees will be applied retroactively to all non-distributed funds in the Splitter
Low
✓
If OWR is used with rebase tokens and there's a negative rebase, principal can be lost
Low
✓
LidoSplit can receive ETH, which will be locked in contract
Low
✓
Upgrade to latest version of Solady to fix LibClone bug
Low
✓
stETH and wstETH addresses can be saved on implementation to save gas
Gas
✓
OWR can be simplified and save gas by not tracking distributedFunds
Gas
✓
Strong trust assumptions between validators and node operators
Informational
Provide node operator checklist to validate setup
Informational
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.
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.
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:
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.
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.
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
:
When packing the data in _packSplitControllerData()
, check recipientsSize
before downcasting to a uint8:
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.
Find a solution that provides a higher level of guarantee that the funds withdrawn are actually rewards, and not a withdrawal.
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.
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.
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:
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.
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.)
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.
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:
Do not allow the OWR to be used with rebasing tokens.
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.
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.
The following test can be dropped into LidoSplit.t.sol
to confirm that the clones can currently receive ETH:
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.
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.
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:
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.
Add the following to LidoSplit.sol
:
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.
Remove stETH
and wstETH
from LidoSplitFactory.sol
, both as storage values, arguments to the constructor, and arguments to clone()
.
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.
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):
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.
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:
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:
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.
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.
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.
Acknowledged. We believe EIP 7002 will reduce this trust assumption as it would enable the validator exit via the execution layer withdrawal key.
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).
Confirm that SplitsMain.getHash(split)
matches the hash of the parameters that the user is expecting to be used.
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.
OptimisticWithdrawalRecipient.getTranches()
should be called to check that amountOfPrincipalStake
is equal to the amount that they will actually be providing.
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)
.
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.
After completion of the fixes, the commit was reviewed. After this review, the updated Lido fee share system in (at commit ) was reviewed.
Recommendation
Review
Fixed in by removing the ability to use non-ETH tokens.
Proof of Concept
Recommendation
Review
Fixed as recommended in .
Recommendations
Review
Recommendation
Review
Fixed as recommended in .
Recommendation
Review
Fixed in by removing the ability to use non-ETH tokens.
Proof of Concept
Recommendation
Review
Fixed in by adding a rescueFunds()
function that can send ETH or any ERC20 (except stETH
or wstETH
) to the splitWallet
.
In the recent , an issue was found the affects LibClone.
Proof of Concept
Recommendation
Delete Solady and clone it from the most recent commit, or any commit after the fixes from were merged.
Review
Solady has been updated to v.0.0.123 in .
Recommendation
Review
Fixed as recommended in .
Recommendation
Review
Fixed as recommended in .
Review
Review