Splits with 256 or more node operators will not be able to switch on fees

[zobront]

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);
    ...
}

[boulder225]

Hey team! Please add your planning poker estimate with Zenhub @OisinKyne @samparsky