Skip to content

Commit

Permalink
fix: statemind - Open access variables may lead to DOS
Browse files Browse the repository at this point in the history
  • Loading branch information
alrxy committed Dec 17, 2024
1 parent a387792 commit ea9dc68
Show file tree
Hide file tree
Showing 12 changed files with 287 additions and 252 deletions.
9 changes: 4 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ This repository provides a framework for developing middleware in a modular and

- **Operators**: Manages operator registration and operator's vault.

- **KeyManager**: Manages operator keys. Variants include `KeyManager256`, `KeyManagerBytes`, and `NoKeyManager`.
- **KeyManager**: Manages operator keys. Variants include `KeyManagerAddress`, `KeyManager256`, `KeyManagerBytes`, and `NoKeyManager`.

- **AccessManager**: Controls access to restricted functions. Implementations include `OwnableAccessManager`, `OzAccessControl`, `OzAccessManaged`, and `NoAccessManager`.

Expand Down Expand Up @@ -63,7 +63,7 @@ Features:
#### SelfRegisterMiddleware

```solidity
contract SelfRegisterMiddleware is SharedVaults, SelfRegisterOperators, KeyManager256, ECDSASig, NoAccessManager, TimestampCapture, EqualStakePower {
contract SelfRegisterMiddleware is SharedVaults, SelfRegisterOperators, KeyManagerAddress, ECDSASig, NoAccessManager, TimestampCapture, EqualStakePower {
// Implementation details...
}
```
Expand All @@ -89,7 +89,7 @@ Features:
#### SelfRegisterSqrtTaskMiddleware

```solidity
contract SelfRegisterSqrtTaskMiddleware is SharedVaults, SelfRegisterOperators, KeyManager256, ECDSASig, OwnableAccessManager, TimestampCapture, EqualStakePower {
contract SelfRegisterSqrtTaskMiddleware is SharedVaults, SelfRegisterOperators, KeyManagerAddress, ECDSASig, OwnableAccessManager, TimestampCapture, EqualStakePower {
// Implementation details...
}
```
Expand Down Expand Up @@ -209,10 +209,9 @@ contract MyCustomMiddleware is BaseMiddleware, Operators, KeyStorage256, Ownable
1. Granting roles to addresses using `grantRole(bytes32 role, address account)`
2. Setting role admins with `_setRoleAdmin(bytes32 role, bytes32 adminRole)`
3. Assigning roles to function selectors via `_setSelectorRole(bytes4 selector, bytes32 role)`
- `OzAccessManaged`: Wraps OpenZeppelin's AccessManaged contract to integrate with external access control systems
- `OzAccessManaged`: Wraps OpenZeppelin's AccessManaged contract to integrate with external access control systems. This allows for more complex access control scenarios where permissions are managed externally, providing flexibility and scalability in managing roles and permissions.

- **Key Manager**: Choose a `KeyManager` implementation that suits your key management needs. Use `KeyManager256` for managing 256-bit keys, `KeyManagerBytes` for handling arbitrary-length keys, or `NoKeyManager` if key management is not required.
- **Key Manager**: Choose a `KeyManager` implementation that suits your key management needs. Use `KeyManagerAddress` for managing address keys, `KeyManager256` for managing 256-bit keys, `KeyManagerBytes` for handling arbitrary-length keys, or `NoKeyManager` if key management is not required.

This framework provides flexibility in building middleware by allowing you to mix and match various extensions based on your requirements. By following the modular approach and best practices outlined, you can develop robust middleware solutions that integrate seamlessly with the network.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ contract SelfRegisterEd25519Middleware is
TimestampCapture,
EqualStakePower
{
error TooManyOperators();
error TooManyOperatorVaults();

uint256 public constant MAX_OPERATORS = 100;
uint256 public constant MAX_OPERATOR_VAULTS = 40;

/**
* @notice Constructor for initializing the SelfRegisterEd25519Middleware contract
* @param network The address of the network
Expand Down Expand Up @@ -61,22 +55,4 @@ contract SelfRegisterEd25519Middleware is
__SelfRegisterOperators_init("SelfRegisterEd25519Middleware");
__OwnableAccessManager_init(owner);
}

/// @notice Prevents DOS by limiting total number of operators that can be registered
/// @dev MAX_OPERATORS constant prevents unbounded iteration when looping through operators
function _beforeRegisterOperator(address operator, bytes memory key, address vault) internal virtual override {
super._beforeRegisterOperator(operator, key, vault);
if (_operatorsLength() >= MAX_OPERATORS) {
revert TooManyOperators();
}
}

/// @notice Prevents DOS by limiting number of vaults per operator
/// @dev MAX_OPERATOR_VAULTS constant prevents unbounded iteration when looping through an operator's vaults
function _beforeRegisterOperatorVault(address operator, address vault) internal virtual override {
super._beforeRegisterOperatorVault(operator, vault);
if (_operatorVaultsLength(operator) >= MAX_OPERATOR_VAULTS) {
revert TooManyOperatorVaults();
}
}
}
24 changes: 0 additions & 24 deletions src/examples/self-register-network/SelfRegisterMiddleware.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,6 @@ contract SelfRegisterMiddleware is
TimestampCapture,
EqualStakePower
{
error TooManyOperators();
error TooManyOperatorVaults();

uint256 public constant MAX_OPERATORS = 100;
uint256 public constant MAX_OPERATOR_VAULTS = 40;

/**
* @notice Constructor for initializing the SelfRegisterMiddleware contract
* @param network The address of the network
Expand Down Expand Up @@ -61,22 +55,4 @@ contract SelfRegisterMiddleware is
__SelfRegisterOperators_init("SelfRegisterMiddleware");
__OwnableAccessManager_init(owner);
}

/// @notice Prevents DOS by limiting total number of operators that can be registered
/// @dev MAX_OPERATORS constant prevents unbounded iteration when looping through operators
function _beforeRegisterOperator(address operator, bytes memory key, address vault) internal virtual override {
super._beforeRegisterOperator(operator, key, vault);
if (_operatorsLength() >= MAX_OPERATORS) {
revert TooManyOperators();
}
}

/// @notice Prevents DOS by limiting number of vaults per operator
/// @dev MAX_OPERATOR_VAULTS constant prevents unbounded iteration when looping through an operator's vaults
function _beforeRegisterOperatorVault(address operator, address vault) internal virtual override {
super._beforeRegisterOperatorVault(operator, vault);
if (_operatorVaultsLength(operator) >= MAX_OPERATOR_VAULTS) {
revert TooManyOperatorVaults();
}
}
}
80 changes: 31 additions & 49 deletions src/examples/sqrt-task-network/SelfRegisterSqrtTaskMiddleware.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,25 +35,22 @@ contract SelfRegisterSqrtTaskMiddleware is

error InvalidHints();
error TaskCompleted();
error TooManyOperators();
error TooManyOperatorVaults();
error TooManySharedVaults();
error InactiveValidator();

event CreateTask(uint256 indexed taskIndex, address indexed operator);
event CreateTask(uint256 indexed taskIndex, address indexed validator);
event CompleteTask(uint256 indexed taskIndex, bool isValidAnswer);

struct Task {
uint48 captureTimestamp;
uint256 value;
address operator;
address validator;
bool completed;
}

bytes32 private constant COMPLETE_TASK_TYPEHASH = keccak256("CompleteTask(uint256 taskIndex,uint256 answer)");

uint256 public constant MAX_OPERATORS = 300;
uint256 public constant MAX_OPERATOR_VAULTS = 40;
uint256 public constant MAX_SHARED_VAULTS = 60;
uint256 public constant MAX_OPERATOR_VAULTS = 20;

Task[] public tasks;

Expand Down Expand Up @@ -84,22 +81,21 @@ contract SelfRegisterSqrtTaskMiddleware is
__SelfRegisterOperators_init("SelfRegisterSqrtTaskMiddleware");
}

// allow anyone to register shared vaults
function _checkAccess() internal view override(AccessManager, OwnableAccessManager) {
if (
msg.sig == this.registerSharedVault.selector || msg.sig == this.unregisterSharedVault.selector
|| msg.sig == this.pauseSharedVault.selector
) {
return;
function createTask(uint256 value, address validator) external returns (uint256 taskIndex) {
bytes memory key = abi.encode(validator);
address operator = operatorByKey(key);
if (!keyWasActiveAt(getCaptureTimestamp(), key)) {
revert InactiveValidator();
}
if (!_isOperatorRegistered(operator)) {
revert OperatorNotRegistered();
}
OwnableAccessManager._checkAccess();
}

function createTask(uint256 value, address operator) external returns (uint256 taskIndex) {
taskIndex = tasks.length;
tasks.push(Task({captureTimestamp: getCaptureTimestamp(), value: value, operator: operator, completed: false}));
tasks.push(
Task({captureTimestamp: getCaptureTimestamp(), value: value, validator: validator, completed: false})
);

emit CreateTask(taskIndex, operator);
emit CreateTask(taskIndex, validator);
}

function completeTask(
Expand Down Expand Up @@ -133,7 +129,7 @@ contract SelfRegisterSqrtTaskMiddleware is

bytes32 hash_ = _hashTypedDataV4(keccak256(abi.encode(COMPLETE_TASK_TYPEHASH, taskIndex, answer)));

if (!SignatureChecker.isValidSignatureNow(task.operator, hash_, signature)) {
if (!SignatureChecker.isValidSignatureNow(task.validator, hash_, signature)) {
revert InvalidSignature();
}
}
Expand Down Expand Up @@ -166,7 +162,17 @@ contract SelfRegisterSqrtTaskMiddleware is

function _slash(uint256 taskIndex, bytes[] calldata stakeHints, bytes[] calldata slashHints) private {
Task storage task = tasks[taskIndex];
address[] memory vaults = _activeVaultsAt(task.captureTimestamp, task.operator);
address operator = operatorByKey(abi.encode(task.validator));
_slashOperator(task.captureTimestamp, operator, stakeHints, slashHints);
}

function _slashOperator(
uint48 captureTimestamp,
address operator,
bytes[] calldata stakeHints,
bytes[] calldata slashHints
) private {
address[] memory vaults = _activeVaultsAt(captureTimestamp, operator);
uint256 vaultsLength = vaults.length;

if (stakeHints.length != slashHints.length || stakeHints.length != vaultsLength) {
Expand All @@ -176,15 +182,14 @@ contract SelfRegisterSqrtTaskMiddleware is
bytes32 subnetwork = _NETWORK().subnetwork(0);
for (uint256 i; i < vaultsLength; ++i) {
address vault = vaults[i];
uint256 slashAmount = IBaseDelegator(IVault(vault).delegator()).stakeAt(
subnetwork, task.operator, task.captureTimestamp, stakeHints[i]
);
uint256 slashAmount =
IBaseDelegator(IVault(vault).delegator()).stakeAt(subnetwork, operator, captureTimestamp, stakeHints[i]);

if (slashAmount == 0) {
continue;
}

_slashVault(task.captureTimestamp, vault, subnetwork, task.operator, slashAmount, slashHints[i]);
_slashVault(captureTimestamp, vault, subnetwork, operator, slashAmount, slashHints[i]);
}
}

Expand All @@ -199,34 +204,11 @@ contract SelfRegisterSqrtTaskMiddleware is
_slashVault(epochStart, vault, subnetwork, operator, amount, hints);
}

/// @notice Prevents DOS by limiting total number of shared vaults that can be registered
/// @dev MAX_SHARED_VAULTS constant prevents unbounded iteration when looping through shared vaults
function _beforeRegisterSharedVault(
address sharedVault
) internal override {
super._beforeRegisterSharedVault(sharedVault);
if (_sharedVaultsLength() >= MAX_SHARED_VAULTS) {
revert TooManySharedVaults();
}
IBaseDelegator(IVault(sharedVault).delegator()).setMaxNetworkLimit(DEFAULT_SUBNETWORK, type(uint256).max);
}

/// @notice Prevents DOS by limiting number of vaults per operator
/// @dev MAX_OPERATOR_VAULTS constant prevents unbounded iteration when looping through an operator's vaults
function _beforeRegisterOperatorVault(address operator, address vault) internal override {
super._beforeRegisterOperatorVault(operator, vault);
if (_operatorVaultsLength(operator) >= MAX_OPERATOR_VAULTS) {
revert TooManyOperatorVaults();
}
IBaseDelegator(IVault(vault).delegator()).setMaxNetworkLimit(DEFAULT_SUBNETWORK, type(uint256).max);
}

/// @notice Prevents DOS by limiting total number of operators that can be registered
/// @dev MAX_OPERATORS constant prevents unbounded iteration when looping through operators
function _beforeRegisterOperator(address operator, bytes memory key, address vault) internal virtual override {
super._beforeRegisterOperator(operator, key, vault);
if (_operatorsLength() >= MAX_OPERATORS) {
revert TooManyOperators();
}
}
}
4 changes: 2 additions & 2 deletions src/examples/sqrt-task-network/SqrtTaskMiddleware.sol
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ contract SqrtTaskMiddleware is
error InvalidSignature();
error TaskCompleted();

event CreateTask(uint256 indexed taskIndex);
event CreateTask(uint256 indexed taskIndex, address indexed operator);
event CompleteTask(uint256 indexed taskIndex, bool isValidAnswer);

struct Task {
Expand Down Expand Up @@ -77,7 +77,7 @@ contract SqrtTaskMiddleware is
taskIndex = tasks.length;
tasks.push(Task({captureTimestamp: getCaptureTimestamp(), value: value, operator: operator, completed: false}));

emit CreateTask(taskIndex);
emit CreateTask(taskIndex, operator);
}

function completeTask(
Expand Down
103 changes: 103 additions & 0 deletions src/extensions/operators/ApprovalRegisterOperators.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

import {SelfRegisterOperators} from "./SelfRegisterOperators.sol";
import {IApprovalRegisterOperators} from "../../interfaces/extensions/operators/IApprovalRegisterOperators.sol";
/**
* @title ApprovalRegisterOperators
* @notice Extends SelfRegisterOperators to add approval-based registration
*/
abstract contract ApprovalRegisterOperators is SelfRegisterOperators, IApprovalRegisterOperators {
uint64 public constant ApprovalRegisterOperators_VERSION = 1;

struct ApprovalRegisterOperatorsStorage {
RegistrationRequest[] requests;
}

// keccak256(abi.encode(uint256(keccak256("symbiotic.storage.ApprovalRegisterOperators")) - 1)) & ~bytes32(uint256(0xff))
bytes32 private constant ApprovalRegisterOperators_STORAGE_LOCATION =
0x8d3c0d900c3fcfbc53470fac03a90d5cf6aa7b77c3f1ed10e6c6bd4d192eaf00;

function _getApprovalRegisterOperatorsStorage() private pure returns (ApprovalRegisterOperatorsStorage storage $) {
bytes32 location = ApprovalRegisterOperators_STORAGE_LOCATION;
assembly {
$.slot := location
}
}

/**
* @inheritdoc IApprovalRegisterOperators
*/
function getRegistrationRequestCount() public view returns (uint256) {
return _getApprovalRegisterOperatorsStorage().requests.length;
}

/**
* @inheritdoc IApprovalRegisterOperators
*/
function getRegistrationRequest(
uint256 index
) public view returns (RegistrationRequest memory) {
return _getApprovalRegisterOperatorsStorage().requests[index];
}

/**
* @inheritdoc IApprovalRegisterOperators
*/
function registerOperator(
uint256 requestIndex
) public checkAccess {
RegistrationRequest memory request = getRegistrationRequest(requestIndex);
_registerOperatorImpl(request.operator, request.key, request.vault);
ApprovalRegisterOperatorsStorage storage $ = _getApprovalRegisterOperatorsStorage();
uint256 lastIndex = $.requests.length - 1;
if (requestIndex != lastIndex) {
$.requests[requestIndex] = $.requests[lastIndex];
}
$.requests.pop();
}

/**
* @notice Override to prevent direct registration
*/
function registerOperator(bytes memory, address, bytes memory) public override {
revert DirectRegistrationNotAllowed();
}

/**
* @notice Override to prevent direct registration
*/
function registerOperator(address, bytes memory, address, bytes memory, bytes memory) public override {
revert DirectRegistrationNotAllowed();
}

/**
* @inheritdoc IApprovalRegisterOperators
*/
function requestRegisterOperator(bytes memory key, address vault, bytes memory signature) public {
_verifyKey(msg.sender, key, signature);
ApprovalRegisterOperatorsStorage storage $ = _getApprovalRegisterOperatorsStorage();
$.requests.push(RegistrationRequest({operator: msg.sender, vault: vault, key: key}));
}

/**
* @inheritdoc IApprovalRegisterOperators
*/
function requestRegisterOperator(
address operator,
bytes memory key,
address vault,
bytes memory signature,
bytes memory keySignature
) public {
SelfRegisterOperatorsStorage storage s = _getSelfRegisterOperatorsStorage();
_verifyEIP712(
operator,
keccak256(abi.encode(REGISTER_OPERATOR_TYPEHASH, operator, keccak256(key), vault, s.nonces[operator]++)),
signature
);
_verifyKey(operator, key, keySignature);
ApprovalRegisterOperatorsStorage storage $ = _getApprovalRegisterOperatorsStorage();
$.requests.push(RegistrationRequest({operator: operator, vault: vault, key: key}));
}
}
Loading

0 comments on commit ea9dc68

Please sign in to comment.