Smart Contract Audit
Obol Audit ReportObol Manager ContractsPrepared by: Zach Obront, Independent Security Researcher Date: Sept 18 to 22, 2023 |
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.
Summary & Scope
The ObolNetwork/obol-manager-contracts repository was audited at commit 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 commit was reviewed. After this review, the updated Lido fee share system in PR #96 was reviewed.
Summary of Findings
Identifier | Title | Severity | Fixed |
---|---|---|---|
M-01 | Future fees may be skirted by setting a non-ETH reward token | Medium | ✓ |
M-02 | Splits with 256 or more node operators will not be able to switch on fees | Medium | ✓ |
M-03 | In a mass slashing event, node operators are incentivized to get slashed | Medium | |
L-01 | Obol fees will be applied retroactively to all non-distributed funds in the Splitter | Low | ✓ |
L-02 | If OWR is used with rebase tokens and there's a negative rebase, principal can be lost | Low | ✓ |
L-03 | LidoSplit can receive ETH, which will be locked in contract | Low | ✓ |
L-04 | Upgrade to latest version of Solady to fix LibClone bug | Low | ✓ |
G-01 | stETH and wstETH addresses can be saved on implementation to save gas | Gas | ✓ |
G-02 | OWR can be simplified and save gas by not tracking distributedFunds | Gas | ✓ |
I-01 | Strong trust assumptions between validators and node operators | Informational | |
I-02 | 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 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:
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.
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
:
// 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:
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.
[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.
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:
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.)