Skip to content

Commit

Permalink
fix: statemind - Incorrect address derivation through public key lead…
Browse files Browse the repository at this point in the history
…s to registration inoperability
  • Loading branch information
alrxy committed Dec 17, 2024
1 parent 4a97095 commit a387792
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 23 deletions.
8 changes: 6 additions & 2 deletions src/examples/self-register-network/SelfRegisterMiddleware.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import {ECDSASig} from "../../extensions/managers/sigs/ECDSASig.sol";
import {OwnableAccessManager} from "../../extensions/managers/access/OwnableAccessManager.sol";
import {TimestampCapture} from "../../extensions/managers/capture-timestamps/TimestampCapture.sol";
import {EqualStakePower} from "../../extensions/managers/stake-powers/EqualStakePower.sol";
import {KeyManager256} from "../../extensions/managers/keys/KeyManager256.sol";
import {KeyManagerAddress} from "../../extensions/managers/keys/KeyManagerAddress.sol";

contract SelfRegisterMiddleware is
SharedVaults,
SelfRegisterOperators,
KeyManager256,
KeyManagerAddress,
ECDSASig,
OwnableAccessManager,
TimestampCapture,
Expand Down Expand Up @@ -62,13 +62,17 @@ contract SelfRegisterMiddleware is
__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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ import {SelfRegisterOperators} from "../../extensions/operators/SelfRegisterOper

import {ECDSASig} from "../../extensions/managers/sigs/ECDSASig.sol";
import {OwnableAccessManager} from "../../extensions/managers/access/OwnableAccessManager.sol";
import {KeyManager256} from "../../extensions/managers/keys/KeyManager256.sol";
import {KeyManagerAddress} from "../../extensions/managers/keys/KeyManagerAddress.sol";
import {TimestampCapture} from "../../extensions/managers/capture-timestamps/TimestampCapture.sol";
import {EqualStakePower} from "../../extensions/managers/stake-powers/EqualStakePower.sol";

contract SelfRegisterSqrtTaskMiddleware is
SharedVaults,
SelfRegisterOperators,
ECDSASig,
KeyManager256,
KeyManagerAddress,
OwnableAccessManager,
TimestampCapture,
EqualStakePower
Expand All @@ -39,7 +39,7 @@ contract SelfRegisterSqrtTaskMiddleware is
error TooManyOperatorVaults();
error TooManySharedVaults();

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

struct Task {
Expand All @@ -51,7 +51,7 @@ contract SelfRegisterSqrtTaskMiddleware is

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

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

Expand Down Expand Up @@ -99,7 +99,7 @@ contract SelfRegisterSqrtTaskMiddleware 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
118 changes: 118 additions & 0 deletions src/extensions/managers/keys/KeyManagerAddress.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.25;

import {KeyManager} from "../../../managers/extendable/KeyManager.sol";
import {PauseableEnumerableSet} from "../../../libraries/PauseableEnumerableSet.sol";

/**
* @title KeyManagerAddress
* @notice Manages storage and validation of operator keys using address values
* @dev Extends KeyManager to provide key management functionality
*/
abstract contract KeyManagerAddress is KeyManager {
uint64 public constant KeyManagerAddress_VERSION = 1;

using PauseableEnumerableSet for PauseableEnumerableSet.AddressSet;

error DuplicateKey();
error MaxDisabledKeysReached();

uint256 private constant MAX_DISABLED_KEYS = 1;

struct KeyManagerAddressStorage {
/// @notice Mapping from operator addresses to their keys
mapping(address => PauseableEnumerableSet.AddressSet) _keys;
/// @notice Mapping from keys to operator addresses
mapping(address => address) _keyToOperator;
}

// keccak256(abi.encode(uint256(keccak256("symbiotic.storage.KeyManagerAddress")) - 1)) & ~bytes32(uint256(0xff))
bytes32 private constant KeyManagerAddressStorageLocation =
0x3da47716e6090d5a5545e03387f4dac112d37cd069a5573bb81de8579bd9dc00;

function _getKeyManagerAddressStorage() internal pure returns (KeyManagerAddressStorage storage s) {
bytes32 location = KeyManagerAddressStorageLocation;
assembly {
s.slot := location
}
}

/**
* @notice Gets the operator address associated with a key
* @param key The key to lookup
* @return The operator address that owns the key, or zero address if none
*/
function operatorByKey(
bytes memory key
) public view override returns (address) {
KeyManagerAddressStorage storage $ = _getKeyManagerAddressStorage();
return $._keyToOperator[abi.decode(key, (address))];
}

/**
* @notice Gets an operator's active key at the current capture timestamp
* @param operator The operator address to lookup
* @return The operator's active key encoded as bytes, or encoded zero bytes if none
*/
function operatorKey(
address operator
) public view override returns (bytes memory) {
KeyManagerAddressStorage storage $ = _getKeyManagerAddressStorage();
address[] memory active = $._keys[operator].getActive(getCaptureTimestamp());
if (active.length == 0) {
return abi.encode(address(0));
}
return abi.encode(active[0]);
}

/**
* @notice Checks if a key was active at a specific timestamp
* @param timestamp The timestamp to check
* @param key_ The key to check
* @return True if the key was active at the timestamp, false otherwise
*/
function keyWasActiveAt(uint48 timestamp, bytes memory key_) public view override returns (bool) {
KeyManagerAddressStorage storage $ = _getKeyManagerAddressStorage();
address key = abi.decode(key_, (address));
return $._keys[$._keyToOperator[key]].wasActiveAt(timestamp, key);
}

/**
* @notice Updates an operator's key
* @dev Handles key rotation by disabling old key and registering new one
* @param operator The operator address to update
* @param key_ The new key to register, encoded as bytes
*/
function _updateKey(address operator, bytes memory key_) internal override {
KeyManagerAddressStorage storage $ = _getKeyManagerAddressStorage();
address key = abi.decode(key_, (address));
uint48 timestamp = _now();

if ($._keyToOperator[key] != address(0)) {
revert DuplicateKey();
}

// check if we have reached the max number of disabled keys
// this allow us to limit the number times we can change the key
if (key != address(0) && $._keys[operator].length() > MAX_DISABLED_KEYS + 1) {
revert MaxDisabledKeysReached();
}

if ($._keys[operator].length() > 0) {
// try to remove disabled keys
address prevKey = address($._keys[operator].set.array[0].value);
if ($._keys[operator].checkUnregister(timestamp, _SLASHING_WINDOW(), prevKey)) {
$._keys[operator].unregister(timestamp, _SLASHING_WINDOW(), prevKey);
delete $._keyToOperator[prevKey];
} else if ($._keys[operator].wasActiveAt(timestamp, prevKey)) {
$._keys[operator].pause(timestamp, prevKey);
}
}

if (key != address(0)) {
// register the new key
$._keys[operator].register(timestamp, key);
$._keyToOperator[key] = operator;
}
}
}
5 changes: 2 additions & 3 deletions src/extensions/managers/sigs/ECDSASig.sol
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ abstract contract ECDSASig is SigManager, IECDSASig {
bytes memory key_,
bytes memory signature
) internal pure override returns (bool) {
bytes32 key = abi.decode(key_, (bytes32));
address key = abi.decode(key_, (address));
bytes32 hash = keccak256(abi.encodePacked(operator, key));
address signer = recover(hash, signature);
address keyAddress = address(uint160(uint256(key)));
return signer == keyAddress && signer != address(0);
return signer == key && signer != address(0);
}

/**
Expand Down
19 changes: 19 additions & 0 deletions src/libraries/PauseableEnumerableSet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,25 @@ library PauseableEnumerableSet {
self.set.unpause(timestamp, immutablePeriod, uint160(addr));
}

/**
* @notice Checks if an address can be unregistered
* @param self The AddressSet to query
* @param timestamp The current timestamp
* @param immutablePeriod The required waiting period after disabling
* @param value The address to check
* @return bool Whether the address can be unregistered
*/
function checkUnregister(
AddressSet storage self,
uint48 timestamp,
uint48 immutablePeriod,
address value
) internal view returns (bool) {
uint256 pos = self.set.positions[uint160(value)];
if (pos == 0) return false;
return self.set.array[pos - 1].status.checkUnregister(timestamp, immutablePeriod);
}

/**
* @notice Unregisters an address
* @param self The AddressSet to modify
Expand Down
24 changes: 11 additions & 13 deletions test/SigTests.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ contract SigTests is POCBaseTest {
SelfRegisterEd25519Middleware internal ed25519Middleware;
uint256 internal operatorPrivateKey;
address internal operator;
bytes32 internal operatorPublicKey;
string internal constant ED25519_TEST_DATA = "test/helpers/ed25519TestData.json";
address internal ed25519Operator;
address internal vault;
Expand All @@ -37,7 +36,6 @@ contract SigTests is POCBaseTest {
super.setUp();

(operator, operatorPrivateKey) = makeAddrAndKey("operator");
operatorPublicKey = bytes32(uint256(uint160(operator)));

string memory json = vm.readFile(ED25519_TEST_DATA);
ed25519Operator = abi.decode(vm.parseJson(json, ".operator"), (address));
Expand Down Expand Up @@ -135,27 +133,27 @@ contract SigTests is POCBaseTest {

function testSelfRegisterOperator() public {
// Create registration message
bytes32 messageHash = keccak256(abi.encodePacked(operator, operatorPublicKey));
bytes32 messageHash = keccak256(abi.encodePacked(operator, operator));
// Sign message with operator's private key
(uint8 v, bytes32 r, bytes32 s) = vm.sign(operatorPrivateKey, messageHash);
bytes memory signature = abi.encodePacked(r, s, v);
// Register operator using their own signature
vm.prank(operator);
middleware.registerOperator(abi.encode(operatorPublicKey), address(vault), signature);
middleware.registerOperator(abi.encode(operator), address(vault), signature);

// Verify operator is registered correctly
assertTrue(IBaseMiddlewareReader(address(middleware)).isOperatorRegistered(operator));

assertEq(abi.decode(IBaseMiddlewareReader(address(middleware)).operatorKey(operator), (bytes32)), bytes32(0));
assertEq(abi.decode(IBaseMiddlewareReader(address(middleware)).operatorKey(operator), (address)), address(0));
vm.warp(vm.getBlockTimestamp() + 100);
assertEq(
abi.decode(IBaseMiddlewareReader(address(middleware)).operatorKey(operator), (bytes32)), operatorPublicKey
abi.decode(IBaseMiddlewareReader(address(middleware)).operatorKey(operator), (address)), operator
);
}

function testSelxfRegisterOperatorInvalidSignature() public {
// Create registration message with wrong key
bytes32 wrongKey = bytes32(uint256(1));
address wrongKey = address(uint160(uint256(1)));
bytes32 messageHash = keccak256(abi.encodePacked(operator, wrongKey));
// Sign message
(uint8 v, bytes32 r, bytes32 s) = vm.sign(operatorPrivateKey, messageHash);
Expand All @@ -164,36 +162,36 @@ contract SigTests is POCBaseTest {
// Attempt to register with mismatched key should fail
vm.prank(operator);
vm.expectRevert();
middleware.registerOperator(abi.encode(operatorPublicKey), address(vault), signature);
middleware.registerOperator(abi.encode(operator), address(vault), signature);
}

function testSelfxRegisterOperatorWrongSender() public {
// Create valid registration message
bytes32 messageHash = keccak256(abi.encodePacked(operator, operatorPublicKey));
bytes32 messageHash = keccak256(abi.encodePacked(operator, operator));
// Sign message with operator's key
(uint8 v, bytes32 r, bytes32 s) = vm.sign(operatorPrivateKey, messageHash);
bytes memory signature = abi.encodePacked(r, s, v);

// Attempt to register from different address should fail
vm.prank(alice);
vm.expectRevert();
middleware.registerOperator(abi.encode(operatorPublicKey), address(vault), signature);
middleware.registerOperator(abi.encode(operator), address(vault), signature);
}

function testSelxfRegisterOperatorAlreadyRegistered() public {
// Create registration message
bytes32 messageHash = keccak256(abi.encodePacked(operator, operatorPublicKey));
bytes32 messageHash = keccak256(abi.encodePacked(operator, operator));
// Sign message
(uint8 v, bytes32 r, bytes32 s) = vm.sign(operatorPrivateKey, messageHash);
bytes memory signature = abi.encodePacked(r, s, v);
// Register operator first time
vm.prank(operator);
middleware.registerOperator(abi.encode(operatorPublicKey), address(vault), signature);
middleware.registerOperator(abi.encode(operator), address(vault), signature);

// Attempt to register again should fail
vm.prank(operator);
vm.expectRevert();
middleware.registerOperator(abi.encode(operatorPublicKey), address(vault), signature);
middleware.registerOperator(abi.encode(operator), address(vault), signature);
}

function testEd25519RegisterOperatorMismatchedKeyAndSignature() public {
Expand Down

0 comments on commit a387792

Please sign in to comment.