From 6c111e56bcf394476c140cd8767652052f06b1ca Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Tue, 4 Feb 2025 13:44:20 +0300 Subject: [PATCH 1/6] OWRv2 with deposit proxy --- script/DistributeFunds.s.sol | 2 +- script/OWRV2FactoryScript.s.sol | 10 +- script/RequestConsolidation.s.sol | 2 +- script/RequestWithdrawal.s.sol | 2 +- src/interfaces/IDepositContract.sol | 48 ++++++ src/owr/OptimisticWithdrawalRecipientV2.sol | 128 +++++++--------- ...OptimisticWithdrawalRecipientV2Factory.sol | 93 +++++------- src/test/owr/OWRV2Reentrancy.sol | 5 +- .../owr/OptimisticWithdrawalRecipientV2.t.sol | 120 +++++++-------- ...timisticWithdrawalRecipientV2Factory.t.sol | 140 +++++++++++------- src/test/owr/mocks/DepositContractMock.sol | 21 +++ .../{pectra => mocks}/SystemContractMock.sol | 0 12 files changed, 323 insertions(+), 248 deletions(-) create mode 100644 src/interfaces/IDepositContract.sol create mode 100644 src/test/owr/mocks/DepositContractMock.sol rename src/test/owr/{pectra => mocks}/SystemContractMock.sol (100%) diff --git a/script/DistributeFunds.s.sol b/script/DistributeFunds.s.sol index 21631e9..f6580af 100644 --- a/script/DistributeFunds.s.sol +++ b/script/DistributeFunds.s.sol @@ -18,7 +18,7 @@ contract DistributeFunds is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(deployedOWRV2); + OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(deployedOWRV2)); owr.distributeFunds(); vm.stopBroadcast(); diff --git a/script/OWRV2FactoryScript.s.sol b/script/OWRV2FactoryScript.s.sol index 9a44ca7..f082452 100644 --- a/script/OWRV2FactoryScript.s.sol +++ b/script/OWRV2FactoryScript.s.sol @@ -18,7 +18,8 @@ contract OWRV2FactoryScript is Script { address constant consolidationSysContract = 0x00431F263cE400f4455c2dCf564e53007Ca4bbBb; // From https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#configuration address constant withdrawalSysContract = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; - + // By default the script is aiming devnet-5 (7088110746) + address constant depositSysContract = 0x4242424242424242424242424242424242424242; // TBD address ensReverseRegistrar = address(0x0); @@ -28,6 +29,10 @@ contract OWRV2FactoryScript is Script { if (ensReverseRegistrar == address(0x0)) { revert("ensReverseRegistrar not set"); } + + if (block.chainid != 7088110746) { // devnet-5 + revert("update deposit contract address and chain id"); + } vm.startBroadcast(privKey); @@ -36,7 +41,8 @@ contract OWRV2FactoryScript is Script { ensReverseRegistrar, msg.sender, consolidationSysContract, - withdrawalSysContract + withdrawalSysContract, + depositSysContract ); vm.stopBroadcast(); diff --git a/script/RequestConsolidation.s.sol b/script/RequestConsolidation.s.sol index 4738b7f..7f481ad 100644 --- a/script/RequestConsolidation.s.sol +++ b/script/RequestConsolidation.s.sol @@ -19,7 +19,7 @@ contract RequestConsolidation is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(owrv2); + OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(owrv2)); // Call the function on the deployed contract bytes[] memory sourcePubKeys = new bytes[](1); diff --git a/script/RequestWithdrawal.s.sol b/script/RequestWithdrawal.s.sol index 76548f7..3d1cc54 100644 --- a/script/RequestWithdrawal.s.sol +++ b/script/RequestWithdrawal.s.sol @@ -19,7 +19,7 @@ contract RequestWithdrawal is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(owrv2); + OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(owrv2)); bytes[] memory pubKeys = new bytes[](1); pubKeys[0] = pubkey; diff --git a/src/interfaces/IDepositContract.sol b/src/interfaces/IDepositContract.sol new file mode 100644 index 0000000..8458d70 --- /dev/null +++ b/src/interfaces/IDepositContract.sol @@ -0,0 +1,48 @@ +// ┏━━━┓━┏┓━┏┓━━┏━━━┓━━┏━━━┓━━━━┏━━━┓━━━━━━━━━━━━━━━━━━━┏┓━━━━━┏━━━┓━━━━━━━━━┏┓━━━━━━━━━━━━━━┏┓━ +// ┃┏━━┛┏┛┗┓┃┃━━┃┏━┓┃━━┃┏━┓┃━━━━┗┓┏┓┃━━━━━━━━━━━━━━━━━━┏┛┗┓━━━━┃┏━┓┃━━━━━━━━┏┛┗┓━━━━━━━━━━━━┏┛┗┓ +// ┃┗━━┓┗┓┏┛┃┗━┓┗┛┏┛┃━━┃┃━┃┃━━━━━┃┃┃┃┏━━┓┏━━┓┏━━┓┏━━┓┏┓┗┓┏┛━━━━┃┃━┗┛┏━━┓┏━┓━┗┓┏┛┏━┓┏━━┓━┏━━┓┗┓┏┛ +// ┃┏━━┛━┃┃━┃┏┓┃┏━┛┏┛━━┃┃━┃┃━━━━━┃┃┃┃┃┏┓┃┃┏┓┃┃┏┓┃┃━━┫┣┫━┃┃━━━━━┃┃━┏┓┃┏┓┃┃┏┓┓━┃┃━┃┏┛┗━┓┃━┃┏━┛━┃┃━ +// ┃┗━━┓━┃┗┓┃┃┃┃┃┃┗━┓┏┓┃┗━┛┃━━━━┏┛┗┛┃┃┃━┫┃┗┛┃┃┗┛┃┣━━┃┃┃━┃┗┓━━━━┃┗━┛┃┃┗┛┃┃┃┃┃━┃┗┓┃┃━┃┗┛┗┓┃┗━┓━┃┗┓ +// ┗━━━┛━┗━┛┗┛┗┛┗━━━┛┗┛┗━━━┛━━━━┗━━━┛┗━━┛┃┏━┛┗━━┛┗━━┛┗┛━┗━┛━━━━┗━━━┛┗━━┛┗┛┗┛━┗━┛┗┛━┗━━━┛┗━━┛━┗━┛ +// ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┃┃━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +// ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┗┛━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + +// SPDX-License-Identifier: CC0-1.0 + +// pragma solidity 0.6.11 was the original version +pragma solidity 0.8.19; + +// This interface is designed to be compatible with the Vyper version. +/// @notice This is the Ethereum 2.0 deposit contract interface. +/// For more information see the Phase 0 specification under https://github.com/ethereum/eth2.0-specs +interface IDepositContract { + /// @notice A processed deposit event. + event DepositEvent( + bytes pubkey, + bytes withdrawal_credentials, + bytes amount, + bytes signature, + bytes index + ); + + /// @notice Submit a Phase 0 DepositData object. + /// @param pubkey A BLS12-381 public key. + /// @param withdrawal_credentials Commitment to a public key for withdrawals. + /// @param signature A BLS12-381 signature. + /// @param deposit_data_root The SHA-256 hash of the SSZ-encoded DepositData object. + /// Used as a protection against malformed input. + function deposit( + bytes calldata pubkey, + bytes calldata withdrawal_credentials, + bytes calldata signature, + bytes32 deposit_data_root + ) external payable; + + /// @notice Query the current deposit root hash. + /// @return The deposit root hash. + function get_deposit_root() external view returns (bytes32); + + /// @notice Query the current deposit count. + /// @return The deposit count encoded as a little endian 64-bit number. + function get_deposit_count() external view returns (bytes memory); +} \ No newline at end of file diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol index 0163341..539db52 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity 0.8.19; -import {Clone} from "solady/utils/Clone.sol"; import {ERC20} from "solmate/tokens/ERC20.sol"; import {OwnableRoles} from "solady/auth/OwnableRoles.sol"; import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; +import {IDepositContract} from "../interfaces/IDepositContract.sol"; /// @title OptimisticWithdrawalRecipientV2 /// @author Obol @@ -12,7 +12,7 @@ import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; /// based on threshold to it's recipients. /// @dev Only ETH can be distributed for a given deployment. There is a /// recovery method for tokens sent by accident. -contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { +contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// ----------------------------------------------------------------------- /// libraries /// ----------------------------------------------------------------------- @@ -94,30 +94,17 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { uint256 internal constant PUSH = 0; uint256 internal constant PULL = 1; - uint256 internal constant ONE_WORD = 32; - uint256 internal constant ADDRESS_BITS = 160; - - /// @dev threshold for pushing balance update as reward or principal - uint256 internal constant BALANCE_CLASSIFICATION_THRESHOLD = 16 ether; - uint256 internal constant PRINCIPAL_RECIPIENT_INDEX = 0; - uint256 internal constant REWARD_RECIPIENT_INDEX = 1; - - /// ----------------------------------------------------------------------- - /// storage - cwia offsets - /// ----------------------------------------------------------------------- - - // 0; first item - uint256 internal constant RECOVERY_ADDRESS_OFFSET = 0; - // 20 = recoveryAddress_offset (0) + recoveryAddress_size (address, 20 - // bytes) - uint256 internal constant TRANCHES_OFFSET = 20; - /// ----------------------------------------------------------------------- /// storage - immutable /// ----------------------------------------------------------------------- address public immutable consolidationSystemContract; address public immutable withdrawalSystemContract; + address public immutable depositSystemContract; + address public immutable recoveryAddress; + address public immutable principalRecipient; + address public immutable rewardRecipient; + uint256 public immutable principalThreshold; /// ----------------------------------------------------------------------- /// storage - mutables @@ -125,15 +112,13 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { /// @dev set to `true` after owner is initialized bool public initialized; + /// Amount of principal stake done via deposit() calls + uint256 public amountOfPrincipalStake; + /// Amount of active balance set aside for pulls /// @dev ERC20s with very large decimals may overflow & cause issues uint128 public fundsPendingWithdrawal; - /// Amount of distributed OWRecipient token for principal - /// @dev Would be less than or equal to amount of stake - /// @dev ERC20s with very large decimals may overflow & cause issues - uint128 public claimedPrincipalFunds; - /// Mapping to account balances for pulling mapping(address => uint256) internal pullBalances; @@ -141,10 +126,22 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { /// constructor /// ----------------------------------------------------------------------- - /// Sets the system contract addresses for withdrawals and consolidations. - constructor(address _consolidationSystemContract, address _withdrawalSystemContract) { + constructor( + address _consolidationSystemContract, + address _withdrawalSystemContract, + address _depositSystemContract, + address _recoveryAddress, + address _principalRecipient, + address _rewardRecipient, + uint256 _principalThreshold + ) { consolidationSystemContract = _consolidationSystemContract; withdrawalSystemContract = _withdrawalSystemContract; + depositSystemContract = _depositSystemContract; + recoveryAddress = _recoveryAddress; + principalRecipient = _principalRecipient; + rewardRecipient = _rewardRecipient; + principalThreshold = _principalThreshold; } /// ----------------------------------------------------------------------- @@ -163,6 +160,29 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { initialized = true; } + /// @dev Fallback function to receive ETH + /// Because we do not use Clone, we must implement this explicitly + receive() external payable {} + + /// @notice Submit a Phase 0 DepositData object. + /// @param pubkey A BLS12-381 public key. + /// @param withdrawal_credentials Commitment to a public key for withdrawals. + /// @param signature A BLS12-381 signature. + /// @param deposit_data_root The SHA-256 hash of the SSZ-encoded DepositData object. + /// Used as a protection against malformed input. + /// @dev This function is a proxy to the deposit() function on the depositSystemContract. + /// The deposited amount is added to the principal stake. + /// Any deposits made directly to the depositSystemContract will not be accounted for. + function deposit( + bytes calldata pubkey, + bytes calldata withdrawal_credentials, + bytes calldata signature, + bytes32 deposit_data_root + ) external payable { + IDepositContract(depositSystemContract).deposit{value: msg.value}(pubkey, withdrawal_credentials, signature, deposit_data_root); + amountOfPrincipalStake += msg.value; + } + /// Distributes target token inside the contract to recipients /// @dev pushes funds to recipients function distributeFunds() external payable { @@ -246,15 +266,12 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { // if recoveryAddress is set, recipient must match it // else, recipient must be one of the OWR recipients - - address _recoveryAddress = recoveryAddress(); - if (_recoveryAddress == address(0)) { + if (recoveryAddress == address(0)) { // ensure txn recipient is a valid OWR recipient - (address principalRecipient, address rewardRecipient, ) = getTranches(); if (recipient != principalRecipient && recipient != rewardRecipient) { revert InvalidTokenRecovery_InvalidRecipient(); } - } else if (recipient != _recoveryAddress) { + } else if (recipient != recoveryAddress) { revert InvalidTokenRecovery_InvalidRecipient(); } @@ -287,22 +304,6 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { /// functions - view & pure /// ----------------------------------------------------------------------- - /// Return unpacked tranches - /// @return principalRecipient Addres of principal recipient - /// @return rewardRecipient Address of reward recipient - /// @return amountOfPrincipalStake Absolute payment threshold for principal - function getTranches() - public - pure - returns (address principalRecipient, address rewardRecipient, uint256 amountOfPrincipalStake) - { - uint256 tranche = _getTranche(PRINCIPAL_RECIPIENT_INDEX); - principalRecipient = address(uint160(tranche)); - amountOfPrincipalStake = tranche >> ADDRESS_BITS; - - rewardRecipient = address(uint160(_getTranche(REWARD_RECIPIENT_INDEX))); - } - /// Returns the balance for account `account` /// @param account Account to return balance for /// @return Account's withdrawable ether balance @@ -310,12 +311,6 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { return pullBalances[account]; } - /// Address to recover non-OWR tokens to - /// @dev equivalent to address public immutable recoveryAddress; - function recoveryAddress() public pure returns (address) { - return _getArgAddress(RECOVERY_ADDRESS_OFFSET); - } - /// ----------------------------------------------------------------------- /// functions - private & internal /// ----------------------------------------------------------------------- @@ -370,28 +365,22 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { // load storage into memory uint256 currentbalance = address(this).balance; - uint256 _claimedPrincipalFunds = uint256(claimedPrincipalFunds); uint256 _memoryFundsPendingWithdrawal = uint256(fundsPendingWithdrawal); uint256 _fundsToBeDistributed = currentbalance - _memoryFundsPendingWithdrawal; - (address principalRecipient, address rewardRecipient, uint256 amountOfPrincipalStake) = getTranches(); - // determine which recipeint is getting paid based on funds to be // distributed uint256 _principalPayout = 0; uint256 _rewardPayout = 0; unchecked { - // _claimedPrincipalFunds should always be <= amountOfPrincipalStake - uint256 principalStakeRemaining = amountOfPrincipalStake - _claimedPrincipalFunds; - - if (_fundsToBeDistributed >= BALANCE_CLASSIFICATION_THRESHOLD && principalStakeRemaining > 0) { - if (_fundsToBeDistributed > principalStakeRemaining) { + if (_fundsToBeDistributed >= principalThreshold && amountOfPrincipalStake > 0) { + if (_fundsToBeDistributed > amountOfPrincipalStake) { // this means there is reward part of the funds to be // distributed - _principalPayout = principalStakeRemaining; + _principalPayout = amountOfPrincipalStake; // shouldn't underflow - _rewardPayout = _fundsToBeDistributed - principalStakeRemaining; + _rewardPayout = _fundsToBeDistributed - amountOfPrincipalStake; } else { // this means there is no reward part of the funds to be // distributed @@ -407,7 +396,7 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { // Write to storage // the principal value // it cannot overflow because _principalPayout < _fundsToBeDistributed - if (_principalPayout > 0) claimedPrincipalFunds += uint128(_principalPayout); + if (_principalPayout > 0) amountOfPrincipalStake -= uint128(_principalPayout); } /// interactions @@ -444,13 +433,4 @@ contract OptimisticWithdrawalRecipientV2 is Clone, OwnableRoles { } } } - - /// Get OWR tranche `i` - /// @dev emulates to uint256[] internal immutable tranche; - function _getTranche(uint256 i) internal pure returns (uint256) { - unchecked { - // shouldn't overflow - return _getArgUint256(TRANCHES_OFFSET + i * ONE_WORD); - } - } } diff --git a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol index f70062a..f4161d4 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol @@ -2,63 +2,52 @@ pragma solidity 0.8.19; import {OptimisticWithdrawalRecipientV2} from "./OptimisticWithdrawalRecipientV2.sol"; -import {LibClone} from "solady/utils/LibClone.sol"; import {IENSReverseRegistrar} from "../interfaces/IENSReverseRegistrar.sol"; /// @title OptimisticWithdrawalRecipientV2Factory /// @author Obol -/// @notice A factory contract for cheaply deploying -/// OptimisticWithdrawalRecipientV2. +/// @notice A factory contract for deploying OptimisticWithdrawalRecipientV2. contract OptimisticWithdrawalRecipientV2Factory { /// ----------------------------------------------------------------------- /// errors /// ----------------------------------------------------------------------- - /// Invalid number of recipients, must be 2 + /// Some recipients are address(0) error Invalid__Recipients(); - /// Thresholds must be positive + /// Threshold must be positive error Invalid__ZeroThreshold(); - /// Invalid threshold at `index`; must be < 2^96 - /// @param threshold threshold of too-large threshold - error Invalid__ThresholdTooLarge(uint256 threshold); - - /// ----------------------------------------------------------------------- - /// libraries - /// ----------------------------------------------------------------------- - - using LibClone for address; + /// Threshold must be below 2048 ether + error Invalid__ThresholdTooLarge(); /// ----------------------------------------------------------------------- /// events /// ----------------------------------------------------------------------- /// Emitted after a new OptimisticWithdrawalRecipientV2 module is deployed - /// @param owr Address of newly created OptimisticWithdrawalRecipientV2 clone - /// @param owner Owner of newly created OptimisticWithdrawalRecipientV2 clone + /// @param owr Address of newly created OptimisticWithdrawalRecipientV2 instance + /// @param owner Owner of newly created OptimisticWithdrawalRecipientV2 instance /// @param recoveryAddress Address to recover non-OWR tokens to /// @param principalRecipient Address to distribute principal payment to /// @param rewardRecipient Address to distribute reward payment to - /// @param threshold Absolute payment threshold for OWR first recipient - /// (reward recipient has no threshold & receives all residual flows) + /// @param principalThreshold Principal vs rewards classification threshold event CreateOWRecipient( address indexed owr, address indexed owner, address recoveryAddress, address principalRecipient, address rewardRecipient, - uint256 threshold + uint256 principalThreshold ); /// ----------------------------------------------------------------------- - /// storage + /// storage - immutable /// ----------------------------------------------------------------------- - uint256 internal constant ADDRESS_BITS = 160; - - /// OptimisticWithdrawalRecipientV2 implementation address - OptimisticWithdrawalRecipientV2 public immutable owrImpl; + address public immutable consolidationSystemContract; + address public immutable withdrawalSystemContract; + address public immutable depositSystemContract; /// ----------------------------------------------------------------------- /// constructor @@ -69,20 +58,26 @@ contract OptimisticWithdrawalRecipientV2Factory { /// @param _ensOwner ENS owner address /// @param _consolidationSystemContract Consolidation system contract address /// @param _withdrawalSystemContract Withdrawal system contract address + /// @param _depositSystemContract Deposit system contract address /// @dev System contracts are expected to be deployed at: /// Consolidation: 0x00431F263cE400f4455c2dCf564e53007Ca4bbBb /// https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7251.md#constants /// Withdrawal: 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA - // https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#configuration + /// https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#configuration + /// Deposit Holesky/Devnet: 0x4242424242424242424242424242424242424242 + /// Deposit Sepolia: 0x7f02C3E3c98b133055B8B348B2Ac625669Ed295D + /// Deposit Mainnet: 0x00000000219ab540356cBB839Cbe05303d7705Fa constructor( string memory _ensName, address _ensReverseRegistrar, address _ensOwner, address _consolidationSystemContract, - address _withdrawalSystemContract + address _withdrawalSystemContract, + address _depositSystemContract ) { - owrImpl = new OptimisticWithdrawalRecipientV2(_consolidationSystemContract, _withdrawalSystemContract); - owrImpl.initialize(address(this)); + consolidationSystemContract = _consolidationSystemContract; + withdrawalSystemContract = _withdrawalSystemContract; + depositSystemContract = _depositSystemContract; IENSReverseRegistrar(_ensReverseRegistrar).setName(_ensName); IENSReverseRegistrar(_ensReverseRegistrar).claim(_ensOwner); @@ -96,42 +91,36 @@ contract OptimisticWithdrawalRecipientV2Factory { /// functions - public & external /// ----------------------------------------------------------------------- - /// Create a new OptimisticWithdrawalRecipientV2 clone + /// Create a new OptimisticWithdrawalRecipientV2 instance /// @param recoveryAddress Address to recover tokens to /// If this address is 0x0, recovery of unrelated tokens can be completed by /// either the principal or reward recipients. If this address is set, only /// this address can recover ERC20 tokens allocated to the OWRV2 contract /// @param principalRecipient Address to distribute principal payments to /// @param rewardRecipient Address to distribute reward payments to - /// @param amountOfPrincipalStake Absolute amount of stake to be paid to - /// principal recipient (reward recipient has no threshold & - /// receives all residual flows) it cannot be greater than uint96 - /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 clone - /// @return owr Address of new OptimisticWithdrawalRecipientV2 clone + /// @param principalThreshold Principal vs rewards classification threshold + /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 instance + /// @return owr Address of new OptimisticWithdrawalRecipientV2 instance function createOWRecipient( address recoveryAddress, address principalRecipient, address rewardRecipient, - uint256 amountOfPrincipalStake, + uint256 principalThreshold, address owner ) external returns (OptimisticWithdrawalRecipientV2 owr) { - /// checks - - // ensure doesn't have address(0) if (principalRecipient == address(0) || rewardRecipient == address(0)) revert Invalid__Recipients(); - // ensure threshold isn't zero - if (amountOfPrincipalStake == 0) revert Invalid__ZeroThreshold(); - // ensure threshold isn't too large - if (amountOfPrincipalStake > type(uint96).max) revert Invalid__ThresholdTooLarge(amountOfPrincipalStake); - - /// effects - uint256 principalData = (amountOfPrincipalStake << ADDRESS_BITS) | uint256(uint160(principalRecipient)); - uint256 rewardData = uint256(uint160(rewardRecipient)); - - // would not exceed contract size limits - // important to not reorder - bytes memory data = abi.encodePacked(recoveryAddress, principalData, rewardData); - owr = OptimisticWithdrawalRecipientV2(address(owrImpl).clone(data)); + if (principalThreshold == 0) revert Invalid__ZeroThreshold(); + if (principalThreshold > 2048 ether) revert Invalid__ThresholdTooLarge(); + + owr = new OptimisticWithdrawalRecipientV2( + consolidationSystemContract, + withdrawalSystemContract, + depositSystemContract, + recoveryAddress, + principalRecipient, + rewardRecipient, + principalThreshold + ); owr.initialize(owner); emit CreateOWRecipient( @@ -140,7 +129,7 @@ contract OptimisticWithdrawalRecipientV2Factory { recoveryAddress, principalRecipient, rewardRecipient, - amountOfPrincipalStake + principalThreshold ); } } diff --git a/src/test/owr/OWRV2Reentrancy.sol b/src/test/owr/OWRV2Reentrancy.sol index 6e10bae..e181d1d 100644 --- a/src/test/owr/OWRV2Reentrancy.sol +++ b/src/test/owr/OWRV2Reentrancy.sol @@ -2,10 +2,13 @@ pragma solidity ^0.8.19; import "forge-std/Test.sol"; +import "forge-std/console.sol"; import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; contract OWRV2Reentrancy is Test { receive() external payable { - if (address(this).balance <= 1 ether) OptimisticWithdrawalRecipientV2(msg.sender).distributeFunds(); + console.log("OWRV2Reentrancy::receive() with value", msg.value, "and balance", address(this).balance); + + if (address(this).balance <= 1 ether) OptimisticWithdrawalRecipientV2(payable(msg.sender)).distributeFunds(); } } diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol index 9ec7573..969bd9b 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol @@ -2,16 +2,17 @@ pragma solidity ^0.8.19; import "forge-std/Test.sol"; +import "forge-std/console.sol"; import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; import {OptimisticWithdrawalRecipientV2Factory} from "src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; import {MockERC20} from "../utils/mocks/MockERC20.sol"; import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; -import {OWRTestHelper} from "./OWRTestHelper.t.sol"; import {OWRV2Reentrancy} from "./OWRV2Reentrancy.sol"; -import {SystemContractMock} from "./pectra/SystemContractMock.sol"; +import {SystemContractMock} from "./mocks/SystemContractMock.sol"; +import {DepositContractMock} from "./mocks/DepositContractMock.sol"; import {IENSReverseRegistrar} from "../../interfaces/IENSReverseRegistrar.sol"; -contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { +contract OptimisticWithdrawalRecipientV2Test is Test { using SafeTransferLib for address; event DistributeFunds(uint256 principalPayout, uint256 rewardPayout, uint256 pullFlowFlag); @@ -21,21 +22,23 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { address public ENS_REVERSE_REGISTRAR = 0x084b1c3C81545d370f3634392De611CaaBFf8148; - OptimisticWithdrawalRecipientV2 public owrModule; - OptimisticWithdrawalRecipientV2Factory public owrFactory; - address internal recoveryAddress; + uint256 public constant BALANCE_CLASSIFICATION_THRESHOLD = 16 ether; + uint256 public constant INITIAL_DEPOSIT_AMOUNT = 32 ether; + OptimisticWithdrawalRecipientV2Factory public owrFactory; OptimisticWithdrawalRecipientV2 owrETH; OptimisticWithdrawalRecipientV2 owrETH_OR; SystemContractMock consolidationMock; SystemContractMock withdrawalMock; + DepositContractMock depositMock; MockERC20 mERC20; + address internal recoveryAddress; address internal principalRecipient; address internal rewardsRecipient; - uint256 internal trancheThreshold; + uint256 internal principalThreshold; function setUp() public { vm.mockCall( @@ -51,49 +54,52 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { consolidationMock = new SystemContractMock(48 + 48); withdrawalMock = new SystemContractMock(48 + 8); + depositMock = new DepositContractMock(); + owrFactory = new OptimisticWithdrawalRecipientV2Factory( "demo.obol.eth", ENS_REVERSE_REGISTRAR, address(this), address(consolidationMock), - address(withdrawalMock) + address(withdrawalMock), + address(depositMock) ); mERC20 = new MockERC20("demo", "DMT", 18); mERC20.mint(type(uint256).max); - (principalRecipient, rewardsRecipient) = generateTrancheRecipients(uint256(uint160(makeAddr("tranche")))); - // use 1 validator as default tranche threshold - trancheThreshold = ETH_STAKE; - recoveryAddress = makeAddr("recoveryAddress"); + principalRecipient = makeAddr("principalRecipient"); + rewardsRecipient = makeAddr("rewardsRecipient"); + principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD; owrETH = owrFactory.createOWRecipient( recoveryAddress, principalRecipient, rewardsRecipient, - trancheThreshold, + principalThreshold, address(this) ); owrETH_OR = owrFactory.createOWRecipient( address(0), principalRecipient, rewardsRecipient, - trancheThreshold, + principalThreshold, address(this) ); - } - function testGetTranches() public { - // eth - (address _principalRecipient, address _rewardsRecipient, uint256 wtrancheThreshold) = owrETH.getTranches(); + owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); + owrETH_OR.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); + } - assertEq(_principalRecipient, principalRecipient, "invalid principal recipient"); - assertEq(_rewardsRecipient, rewardsRecipient, "invalid rewards recipient"); - assertEq(wtrancheThreshold, ETH_STAKE, "invalid eth tranche threshold"); + function testDefaultParameters() public { + assertEq(owrETH.recoveryAddress(), recoveryAddress, "invalid recovery address"); + assertEq(owrETH.principalRecipient(), principalRecipient, "invalid principal recipient"); + assertEq(owrETH.rewardRecipient(), rewardsRecipient, "invalid rewards recipient"); + assertEq(owrETH.principalThreshold(), BALANCE_CLASSIFICATION_THRESHOLD, "invalid principal threshold"); } - function testOWRPectraInitialization() public { + function testInitialization() public { assertTrue(owrETH.initialized()); assertEq(owrETH.owner(), address(this)); } @@ -456,6 +462,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { OWRV2Reentrancy wr = new OWRV2Reentrancy(); owrETH = owrFactory.createOWRecipient(recoveryAddress, address(wr), rewardsRecipient, 1 ether, address(this)); + owrETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); address(owrETH).safeTransferETH(33 ether); vm.expectRevert(SafeTransferLib.ETHTransferFailed.selector); @@ -594,8 +601,7 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { } function testFuzzCan_distributeDepositsToRecipients( - uint256 _recipientsSeed, - uint256 _thresholdsSeed, + uint96 _threshold, uint8 _numDeposits, uint256 _ethAmount, uint256 _erc20Amount @@ -603,18 +609,17 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { _ethAmount = uint256(bound(_ethAmount, 0.01 ether, 34 ether)); _erc20Amount = uint256(bound(_erc20Amount, 0.01 ether, 34 ether)); vm.assume(_numDeposits > 0); - (address _principalRecipient, address _rewardsRecipient, uint256 _trancheThreshold) = generateTranches( - _recipientsSeed, - _thresholdsSeed - ); + vm.assume(_threshold > 0 && _threshold < 2048 ether); + principalThreshold = _threshold; owrETH = owrFactory.createOWRecipient( recoveryAddress, - _principalRecipient, - _rewardsRecipient, - _trancheThreshold, + principalRecipient, + rewardsRecipient, + principalThreshold, address(this) ); + owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); /// test eth for (uint256 i = 0; i < _numDeposits; i++) { @@ -630,35 +635,34 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { if (BALANCE_CLASSIFICATION_THRESHOLD > _totalETHAmount) { // then all of the deposit should be classified as reward - assertEq(_principalRecipient.balance, 0, "should not classify reward as principal"); + assertEq(principalRecipient.balance, 0, "should not classify reward as principal"); - assertEq(_rewardsRecipient.balance, _totalETHAmount, "invalid amount"); + assertEq(rewardsRecipient.balance, _totalETHAmount, "invalid amount"); } if (_ethAmount > BALANCE_CLASSIFICATION_THRESHOLD) { // then all of reward classified as principal // but check if _totalETHAmount > first threshold - if (_totalETHAmount > _trancheThreshold) { + if (_totalETHAmount > principalThreshold) { // there is reward - assertEq(_principalRecipient.balance, _trancheThreshold, "invalid amount"); + assertEq(principalRecipient.balance, principalThreshold, "invalid amount"); assertEq( - _rewardsRecipient.balance, - _totalETHAmount - _trancheThreshold, + rewardsRecipient.balance, + _totalETHAmount - principalThreshold, "should not classify principal as reward" ); } else { // eelse no rewards - assertEq(_principalRecipient.balance, _totalETHAmount, "invalid amount"); + assertEq(principalRecipient.balance, _totalETHAmount, "invalid amount"); - assertEq(_rewardsRecipient.balance, 0, "should not classify principal as reward"); + assertEq(rewardsRecipient.balance, 0, "should not classify principal as reward"); } } } function testFuzzCan_distributePullDepositsToRecipients( - uint256 _recipientsSeed, - uint256 _thresholdsSeed, + uint96 _threshold, uint8 _numDeposits, uint256 _ethAmount, uint256 _erc20Amount @@ -666,19 +670,17 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { _ethAmount = uint256(bound(_ethAmount, 0.01 ether, 40 ether)); _erc20Amount = uint256(bound(_erc20Amount, 0.01 ether, 40 ether)); vm.assume(_numDeposits > 0); - - (address _principalRecipient, address _rewardsRecipient, uint256 _trancheThreshold) = generateTranches( - _recipientsSeed, - _thresholdsSeed - ); + vm.assume(_threshold > 0 && _threshold < 2048 ether); + principalThreshold = _threshold; owrETH = owrFactory.createOWRecipient( recoveryAddress, - _principalRecipient, - _rewardsRecipient, - _trancheThreshold, + principalRecipient, + rewardsRecipient, + principalThreshold, address(this) ); + owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); /// test eth @@ -692,31 +694,31 @@ contract OptimisticWithdrawalRecipientV2Test is OWRTestHelper, Test { // assertEq(owrETH.distributedFunds(), _totalETHAmount); assertEq(owrETH.fundsPendingWithdrawal(), _totalETHAmount); - uint256 principal = owrETH.getPullBalance(_principalRecipient); + uint256 principal = owrETH.getPullBalance(principalRecipient); assertEq( - owrETH.getPullBalance(_principalRecipient), + owrETH.getPullBalance(principalRecipient), (_ethAmount >= BALANCE_CLASSIFICATION_THRESHOLD) - ? _trancheThreshold > _totalETHAmount ? _totalETHAmount : _trancheThreshold + ? principalThreshold > _totalETHAmount ? _totalETHAmount : principalThreshold : 0, "5/invalid recipient balance" ); - uint256 reward = owrETH.getPullBalance(_rewardsRecipient); + uint256 reward = owrETH.getPullBalance(rewardsRecipient); assertEq( - owrETH.getPullBalance(_rewardsRecipient), + owrETH.getPullBalance(rewardsRecipient), (_ethAmount >= BALANCE_CLASSIFICATION_THRESHOLD) - ? _totalETHAmount > _trancheThreshold ? (_totalETHAmount - _trancheThreshold) : 0 + ? _totalETHAmount > principalThreshold ? (_totalETHAmount - principalThreshold) : 0 : _totalETHAmount, "6/invalid recipient balance" ); - owrETH.withdraw(_principalRecipient); - owrETH.withdraw(_rewardsRecipient); + owrETH.withdraw(principalRecipient); + owrETH.withdraw(rewardsRecipient); assertEq(address(owrETH).balance, 0); assertEq(owrETH.fundsPendingWithdrawal(), 0); - assertEq(_principalRecipient.balance, principal, "10/invalid principal balance"); - assertEq(_rewardsRecipient.balance, reward, "11/invalid reward balance"); + assertEq(principalRecipient.balance, principal, "10/invalid principal balance"); + assertEq(rewardsRecipient.balance, reward, "11/invalid reward balance"); } } diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol index 62ab05c..2ac8905 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol @@ -5,30 +5,33 @@ import "forge-std/Test.sol"; import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; import {OptimisticWithdrawalRecipientV2Factory} from "src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; import {MockERC20} from "../utils/mocks/MockERC20.sol"; -import {OWRTestHelper} from "../owr/OWRTestHelper.t.sol"; -import {SystemContractMock} from "./pectra/SystemContractMock.sol"; +import {SystemContractMock} from "./mocks/SystemContractMock.sol"; +import {DepositContractMock} from "./mocks/DepositContractMock.sol"; import {IENSReverseRegistrar} from "../../interfaces/IENSReverseRegistrar.sol"; -contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { +contract OptimisticWithdrawalRecipientV2FactoryTest is Test { event CreateOWRecipient( address indexed owr, address indexed owner, address recoveryAddress, address principalRecipient, address rewardRecipient, - uint256 threshold + uint256 principalThreshold ); address public ENS_REVERSE_REGISTRAR = 0x084b1c3C81545d370f3634392De611CaaBFf8148; + uint256 public constant BALANCE_CLASSIFICATION_THRESHOLD = 16 ether; + SystemContractMock consolidationMock; SystemContractMock withdrawalMock; + DepositContractMock depositMock; OptimisticWithdrawalRecipientV2Factory owrFactory; address public recoveryAddress; address public principalRecipient; - address public rewardRecipient; - uint256 public threshold; + address public rewardsRecipient; + uint256 public principalThreshold; function setUp() public { vm.mockCall( @@ -42,28 +45,45 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { bytes.concat(bytes32(0)) ); - consolidationMock = new SystemContractMock(48+48); - withdrawalMock = new SystemContractMock(48+8); + consolidationMock = new SystemContractMock(48 + 48); + withdrawalMock = new SystemContractMock(48 + 8); + depositMock = new DepositContractMock(); + owrFactory = new OptimisticWithdrawalRecipientV2Factory( "demo.obol.eth", ENS_REVERSE_REGISTRAR, address(this), address(consolidationMock), - address(withdrawalMock) + address(withdrawalMock), + address(depositMock) ); + recoveryAddress = makeAddr("recoveryAddress"); - (principalRecipient, rewardRecipient) = generateTrancheRecipients(10); - threshold = ETH_STAKE; + principalRecipient = makeAddr("principalRecipient"); + rewardsRecipient = makeAddr("rewardsRecipient"); + principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD; } function testCan_createOWRecipient() public { - OptimisticWithdrawalRecipientV2 owr = owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); + OptimisticWithdrawalRecipientV2 owr = owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + rewardsRecipient, + principalThreshold, + address(this) + ); assertEq(owr.owner(), address(this)); assertEq(address(owr.consolidationSystemContract()), address(consolidationMock)); assertEq(address(owr.withdrawalSystemContract()), address(withdrawalMock)); recoveryAddress = address(0); - owr = owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); + owr = owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + rewardsRecipient, + principalThreshold, + address(this) + ); assertEq(owr.recoveryAddress(), address(0)); } @@ -76,10 +96,16 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { address(this), recoveryAddress, principalRecipient, - rewardRecipient, - threshold + rewardsRecipient, + principalThreshold + ); + owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + rewardsRecipient, + principalThreshold, + address(this) ); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); recoveryAddress = address(0); @@ -90,42 +116,46 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { address(this), recoveryAddress, principalRecipient, - rewardRecipient, - threshold + rewardsRecipient, + principalThreshold + ); + owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + rewardsRecipient, + principalThreshold, + address(this) ); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); } function testCannot_createWithInvalidRecipients() public { - (principalRecipient, rewardRecipient, threshold) = generateTranches(1, 1); - // eth vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, address(0), rewardRecipient, threshold, address(this)); + owrFactory.createOWRecipient(recoveryAddress, address(0), rewardsRecipient, principalThreshold, address(this)); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, address(0), address(0), threshold, address(this)); + owrFactory.createOWRecipient(recoveryAddress, address(0), address(0), principalThreshold, address(this)); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, address(0), threshold, address(this)); + owrFactory.createOWRecipient(recoveryAddress, principalRecipient, address(0), principalThreshold, address(this)); } function testCannot_createWithInvalidThreshold() public { - (principalRecipient, rewardRecipient) = generateTrancheRecipients(2); - threshold = 0; + principalThreshold = 0; vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); - - vm.expectRevert( - abi.encodeWithSelector( - OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector, - type(uint128).max - ) + owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + rewardsRecipient, + principalThreshold, + address(this) ); + + vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); owrFactory.createOWRecipient( recoveryAddress, principalRecipient, - rewardRecipient, + rewardsRecipient, type(uint128).max, address(this) ); @@ -135,14 +165,8 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { /// Fuzzing Tests /// ---------------------------------------------------------------------- - function testFuzzCan_createOWRecipient( - address _recoveryAddress, - uint256 recipientsSeed, - uint256 thresholdSeed - ) public { - recoveryAddress = _recoveryAddress; - - (principalRecipient, rewardRecipient, threshold) = generateTranches(recipientsSeed, thresholdSeed); + function testFuzzCan_createOWRecipient(uint96 _threshold) public { + vm.assume(_threshold > 0 && _threshold < 2048 ether); vm.expectEmit(false, true, true, true); emit CreateOWRecipient( @@ -150,30 +174,32 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is OWRTestHelper, Test { address(this), recoveryAddress, principalRecipient, - rewardRecipient, - threshold + rewardsRecipient, + _threshold ); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); + owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardsRecipient, _threshold, address(this)); } - function testFuzzCannot_CreateWithZeroThreshold(uint256 _receipientSeed) public { - threshold = 0; - (principalRecipient, rewardRecipient) = generateTrancheRecipients(_receipientSeed); + function testFuzzCannot_CreateWithZeroThreshold(address _rewardsRecipient) public { + vm.assume(_rewardsRecipient != address(0)); + principalThreshold = 0; // eth vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); + owrFactory.createOWRecipient( + recoveryAddress, + principalRecipient, + _rewardsRecipient, + principalThreshold, + address(this) + ); } - function testFuzzCannot_CreateWithLargeThreshold(uint256 _receipientSeed, uint256 _threshold) public { + function testFuzzCannot_CreateWithLargeThreshold(address _rewardsRecipient, uint256 _threshold) public { vm.assume(_threshold > type(uint96).max); + vm.assume(_rewardsRecipient != address(0)); - threshold = _threshold; - (principalRecipient, rewardRecipient) = generateTrancheRecipients(_receipientSeed); - - vm.expectRevert( - abi.encodeWithSelector(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector, _threshold) - ); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardRecipient, threshold, address(this)); + vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); + owrFactory.createOWRecipient(recoveryAddress, principalRecipient, _rewardsRecipient, _threshold, address(this)); } } diff --git a/src/test/owr/mocks/DepositContractMock.sol b/src/test/owr/mocks/DepositContractMock.sol new file mode 100644 index 0000000..c67e628 --- /dev/null +++ b/src/test/owr/mocks/DepositContractMock.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity ^0.8.19; + +import "forge-std/console.sol"; +import {IDepositContract} from "../../../interfaces/IDepositContract.sol"; + +/// @title DepositContractMock +/// @notice This contract mocks the standard Deposit Contract. +contract DepositContractMock is IDepositContract { + function deposit(bytes calldata, bytes calldata, bytes calldata, bytes32) external payable override { + console.log("DepositContractMock.deposit called with value", msg.value); + } + + function get_deposit_root() external pure override returns (bytes32) { + revert("DepositContractMock.get_deposit_root not implemented"); + } + + function get_deposit_count() external pure override returns (bytes memory) { + revert("DepositContractMock.get_deposit_count not implemented"); + } +} diff --git a/src/test/owr/pectra/SystemContractMock.sol b/src/test/owr/mocks/SystemContractMock.sol similarity index 100% rename from src/test/owr/pectra/SystemContractMock.sol rename to src/test/owr/mocks/SystemContractMock.sol From 7b22a91f646140a3f0c82b718cc32fa0903d0005 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Wed, 5 Feb 2025 09:56:28 +0300 Subject: [PATCH 2/6] Limit withdraw amount and better tests --- src/owr/OptimisticWithdrawalRecipientV2.sol | 37 +++++++---- ...OptimisticWithdrawalRecipientV2Factory.sol | 6 +- .../owr/OptimisticWithdrawalRecipientV2.t.sol | 63 +++++++++++++++---- 3 files changed, 77 insertions(+), 29 deletions(-) diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol index 539db52..8d71d52 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2.sol @@ -171,7 +171,8 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// @param deposit_data_root The SHA-256 hash of the SSZ-encoded DepositData object. /// Used as a protection against malformed input. /// @dev This function is a proxy to the deposit() function on the depositSystemContract. - /// The deposited amount is added to the principal stake. + /// The deposited amount is accounted for in the amountOfPrincipalStake, which is used + /// to determine the principalRecipient's share of the funds to be distributed. /// Any deposits made directly to the depositSystemContract will not be accounted for. function deposit( bytes calldata pubkey, @@ -179,7 +180,12 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { bytes calldata signature, bytes32 deposit_data_root ) external payable { - IDepositContract(depositSystemContract).deposit{value: msg.value}(pubkey, withdrawal_credentials, signature, deposit_data_root); + IDepositContract(depositSystemContract).deposit{value: msg.value}( + pubkey, + withdrawal_credentials, + signature, + deposit_data_root + ); amountOfPrincipalStake += msg.value; } @@ -232,7 +238,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// excess amount will be refunded /// withdrawals that leave a validator with (0..32) ether will cause the transaction to fail /// @param pubKeys validator public keys - /// @param amounts withdrawal amounts in gwei + /// @param amounts withdrawal amounts in gwei (must be >= principalThreshold) function requestWithdrawal( bytes[] calldata pubKeys, uint64[] calldata amounts @@ -243,6 +249,9 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { uint256 len = pubKeys.length; for (uint256 i; i < len; ) { + uint256 amountWei = amounts[i] * 1 gwei; + if (amountWei < principalThreshold) revert InvalidRequest_Params(); + uint256 _currentFee = _computeSystemContractFee(withdrawalSystemContract); if (_currentFee > remainingFee) revert InvalidRequest_NotEnoughFee(); @@ -286,7 +295,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { emit RecoverNonOWRecipientFunds(nonOWRToken, recipient, amount); } - /// Withdraw token balance for account + /// Withdraw token balance for an account /// @param account Address to withdraw on behalf of function withdraw(address account) external { uint256 amount = pullBalances[account]; @@ -304,7 +313,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// functions - view & pure /// ----------------------------------------------------------------------- - /// Returns the balance for account `account` + /// Returns the balance for the account `account` /// @param account Account to return balance for /// @return Account's withdrawable ether balance function getPullBalance(address account) external view returns (uint256) { @@ -315,7 +324,9 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// functions - private & internal /// ----------------------------------------------------------------------- - /// Compute system contracts fee + /// Compute system contract's fee + /// @param systemContractAddress Address of the consolidation system contract + /// @return The computed fee function _computeSystemContractFee(address systemContractAddress) internal view returns (uint256) { (bool ok, bytes memory result) = systemContractAddress.staticcall(""); if (!ok) revert InvalidRequest_SystemGetFee(); @@ -323,7 +334,10 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { return uint256(bytes32(result)); } - /// Executes single consolidation request + /// Execute a single consolidation request + /// @param source Source validator public key + /// @param target Target validator public key + /// @param fee Fee for the consolidation request function _requestConsolidation(bytes calldata source, bytes calldata target, uint256 fee) private { if (source.length != 48 || target.length != 48) revert InvalidRequest_Params(); @@ -368,22 +382,19 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { uint256 _memoryFundsPendingWithdrawal = uint256(fundsPendingWithdrawal); uint256 _fundsToBeDistributed = currentbalance - _memoryFundsPendingWithdrawal; - // determine which recipeint is getting paid based on funds to be - // distributed + // determine which recipeint is getting paid based on funds to be distributed uint256 _principalPayout = 0; uint256 _rewardPayout = 0; unchecked { if (_fundsToBeDistributed >= principalThreshold && amountOfPrincipalStake > 0) { if (_fundsToBeDistributed > amountOfPrincipalStake) { - // this means there is reward part of the funds to be - // distributed + // this means there is reward part of the funds to be distributed _principalPayout = amountOfPrincipalStake; // shouldn't underflow _rewardPayout = _fundsToBeDistributed - amountOfPrincipalStake; } else { - // this means there is no reward part of the funds to be - // distributed + // this means there is no reward part of the funds to be distributed _principalPayout = _fundsToBeDistributed; } } else { diff --git a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol index f4161d4..5ef857f 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol @@ -94,13 +94,13 @@ contract OptimisticWithdrawalRecipientV2Factory { /// Create a new OptimisticWithdrawalRecipientV2 instance /// @param recoveryAddress Address to recover tokens to /// If this address is 0x0, recovery of unrelated tokens can be completed by - /// either the principal or reward recipients. If this address is set, only - /// this address can recover ERC20 tokens allocated to the OWRV2 contract + /// either the principal or reward recipients. If this address is set, only + /// this address can recover ERC20 tokens allocated to the OWRV2 contract. /// @param principalRecipient Address to distribute principal payments to /// @param rewardRecipient Address to distribute reward payments to /// @param principalThreshold Principal vs rewards classification threshold /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 instance - /// @return owr Address of new OptimisticWithdrawalRecipientV2 instance + /// @return owr Address of the new OptimisticWithdrawalRecipientV2 instance function createOWRecipient( address recoveryAddress, address principalRecipient, diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol index 969bd9b..025072e 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol @@ -110,6 +110,16 @@ contract OptimisticWithdrawalRecipientV2Test is Test { owrETH.initialize(address(this)); } + function testDeposit() public { + // Initial deposit is done in setUp() + assertEq(address(owrETH).balance, INITIAL_DEPOSIT_AMOUNT); + + uint256 depositAmount = 1 ether; + owrETH.deposit{value: depositAmount}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); + assertEq(address(depositMock).balance, INITIAL_DEPOSIT_AMOUNT + depositAmount); + assertEq(owrETH.amountOfPrincipalStake(), INITIAL_DEPOSIT_AMOUNT + depositAmount); + } + function testCannot_requestConsolidation() public { // Unauthorized address _user = vm.addr(0x2); @@ -220,28 +230,39 @@ contract OptimisticWithdrawalRecipientV2Test is Test { owrETH.requestWithdrawal{value: 1 ether}(new bytes[](1), new uint64[](1)); vm.stopPrank(); + uint64[] memory amounts = new uint64[](1); + bytes[] memory single = new bytes[](1); + single[0] = new bytes(48); + // Inequal array lengths vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); bytes[] memory empty = new bytes[](0); - owrETH.requestWithdrawal{value: 1 ether}(empty, new uint64[](1)); + owrETH.requestWithdrawal{value: 1 ether}(empty, amounts); // Not enough fee (1 wei is the minimum fee) + uint256 validAmount = principalThreshold / 1 gwei; + amounts[0] = uint64(validAmount); vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector); - bytes[] memory single = new bytes[](1); - single[0] = new bytes(48); - owrETH.requestWithdrawal{value: 0}(single, new uint64[](1)); + owrETH.requestWithdrawal{value: 0}(single, amounts); + + // Amount below principalThreshold + uint256 lowAmount = (principalThreshold - 1 wei) / 1 gwei; + amounts[0] = uint64(lowAmount); + vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); + owrETH.requestWithdrawal{value: 100 wei}(single, amounts); // Failed get_fee() request uint256 realFee = withdrawalMock.fakeExponential(0); + amounts[0] = uint64(validAmount); withdrawalMock.setFailNextFeeRequest(true); vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_SystemGetFee.selector); - owrETH.requestWithdrawal{value: realFee}(single, new uint64[](1)); + owrETH.requestWithdrawal{value: realFee}(single, amounts); withdrawalMock.setFailNextFeeRequest(false); // Failed add_request() request withdrawalMock.setFailNextAddRequest(true); vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidWithdrawal_Failed.selector); - owrETH.requestWithdrawal{value: realFee}(single, new uint64[](1)); + owrETH.requestWithdrawal{value: realFee}(single, amounts); withdrawalMock.setFailNextAddRequest(false); } @@ -249,7 +270,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { bytes[] memory pubkeys = new bytes[](1); uint64[] memory amounts = new uint64[](1); bytes memory pubkey = new bytes(48); - uint64 amount = 1234; // gwei + uint64 amount = uint64(principalThreshold / 1 gwei); for (uint256 i = 0; i < 48; i++) { pubkey[i] = bytes1(0xAB); } @@ -290,7 +311,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { pubkey[i] = bytes1(i + 1); } pubkeys[i] = pubkey; - amounts[i] = uint64(1 << i); + amounts[i] = uint64(principalThreshold / 1 gwei + i); } address _user = vm.addr(0x1); @@ -420,17 +441,33 @@ contract OptimisticWithdrawalRecipientV2Test is Test { } function testCan_distributeToBothRecipients() public { - address(owrETH).safeTransferETH(36 ether); + // First deposit of 32eth is done in setUp() + uint256 secondDeposit = 64 ether; + owrETH.deposit{value: secondDeposit}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); + uint256 rewardPayout = 4 ether; + address(owrETH).safeTransferETH(INITIAL_DEPOSIT_AMOUNT + secondDeposit + rewardPayout); + + vm.expectEmit(true, true, true, true); + emit DistributeFunds(INITIAL_DEPOSIT_AMOUNT + secondDeposit, rewardPayout, 0); + owrETH.distributeFunds(); + assertEq(address(owrETH).balance, 0 ether); + assertEq(principalRecipient.balance, INITIAL_DEPOSIT_AMOUNT + secondDeposit); + assertEq(rewardsRecipient.balance, rewardPayout); + } - uint256 principalPayout = 32 ether; + function testCan_distributeDirectDepositsAsReward() public { + // First deposit of 32eth is done in setUp() + uint256 secondDeposit = 64 ether; uint256 rewardPayout = 4 ether; + address(owrETH).safeTransferETH(INITIAL_DEPOSIT_AMOUNT + secondDeposit + rewardPayout); vm.expectEmit(true, true, true, true); - emit DistributeFunds(principalPayout, rewardPayout, 0); + // Second deposit is classified as reward, because we did not call OWR.deposit() + emit DistributeFunds(INITIAL_DEPOSIT_AMOUNT, secondDeposit + rewardPayout, 0); owrETH.distributeFunds(); assertEq(address(owrETH).balance, 0 ether); - assertEq(principalRecipient.balance, 32 ether); - assertEq(rewardsRecipient.balance, 4 ether); + assertEq(principalRecipient.balance, INITIAL_DEPOSIT_AMOUNT); + assertEq(rewardsRecipient.balance, rewardPayout + secondDeposit); } function testCan_distributeMultipleDepositsToPrincipalRecipient() public { From a58cad608bb59b2c76232d190a99aef23a9aada2 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Wed, 5 Feb 2025 10:49:10 +0300 Subject: [PATCH 3/6] Max src consolidation entires is 63 --- src/owr/OptimisticWithdrawalRecipientV2.sol | 1 + src/test/owr/OptimisticWithdrawalRecipientV2.t.sol | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol index 8d71d52..2a8e908 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2.sol @@ -213,6 +213,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { bytes calldata targetPubKey ) external payable onlyOwnerOrRoles(CONSOLIDATION_ROLE) { if (sourcePubKeys.length == 0 || targetPubKey.length != 48) revert InvalidRequest_Params(); + if (sourcePubKeys.length > 63) revert InvalidRequest_Params(); uint256 remainingFee = msg.value; uint256 len = sourcePubKeys.length; diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol index 025072e..a8f1d16 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol @@ -153,6 +153,11 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidConsolidation_Failed.selector); owrETH.requestConsolidation{value: realFee}(single, new bytes(48)); consolidationMock.setFailNextAddRequest(false); + + // Maximum number of source pubkeys is 63 + vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); + bytes[] memory batch64 = new bytes[](64); + owrETH.requestConsolidation{value: realFee}(batch64, new bytes(48)); } function testRequestSingleConsolidation() public { From a396e6ef6b1261c7e2370cfadce705bcc9e535f4 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Thu, 6 Feb 2025 11:20:28 +0300 Subject: [PATCH 4/6] Addressed PR feedback --- script/CreateOWRV2.s.sol | 10 +-- script/OWRV2FactoryScript.s.sol | 12 +-- src/owr/OptimisticWithdrawalRecipientV2.sol | 28 ++++--- ...OptimisticWithdrawalRecipientV2Factory.sol | 41 +++++----- .../owr/OptimisticWithdrawalRecipientV2.t.sol | 80 +++++++++---------- ...timisticWithdrawalRecipientV2Factory.t.sol | 76 +++++++++--------- 6 files changed, 125 insertions(+), 122 deletions(-) diff --git a/script/CreateOWRV2.s.sol b/script/CreateOWRV2.s.sol index b94efd1..65b4ff4 100644 --- a/script/CreateOWRV2.s.sol +++ b/script/CreateOWRV2.s.sol @@ -20,19 +20,19 @@ contract CreateOWRecipientScript is Script { OptimisticWithdrawalRecipientV2Factory factory = OptimisticWithdrawalRecipientV2Factory(deployedOWRV2Factory); + address owner = msg.sender; address recoveryAddress = msg.sender; address principalRecipient = msg.sender; address rewardsRecipient = msg.sender; - uint256 trancheThreshold = 16 ether; - address owner = msg.sender; + uint64 principalThreshold = 16 ether / 1 gwei; // Call the createOWRecipient function factory.createOWRecipient( - recoveryAddress, + owner, principalRecipient, rewardsRecipient, - trancheThreshold, - owner + recoveryAddress, + principalThreshold ); vm.stopBroadcast(); diff --git a/script/OWRV2FactoryScript.s.sol b/script/OWRV2FactoryScript.s.sol index f082452..bf23410 100644 --- a/script/OWRV2FactoryScript.s.sol +++ b/script/OWRV2FactoryScript.s.sol @@ -18,7 +18,7 @@ contract OWRV2FactoryScript is Script { address constant consolidationSysContract = 0x00431F263cE400f4455c2dCf564e53007Ca4bbBb; // From https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#configuration address constant withdrawalSysContract = 0x0c15F14308530b7CDB8460094BbB9cC28b9AaaAA; - // By default the script is aiming devnet-5 (7088110746) + // By default the script is aiming devnet-6 (7072151312) address constant depositSysContract = 0x4242424242424242424242424242424242424242; // TBD address ensReverseRegistrar = address(0x0); @@ -30,19 +30,19 @@ contract OWRV2FactoryScript is Script { revert("ensReverseRegistrar not set"); } - if (block.chainid != 7088110746) { // devnet-5 + if (block.chainid != 7072151312) { // devnet-6 revert("update deposit contract address and chain id"); } vm.startBroadcast(privKey); new OptimisticWithdrawalRecipientV2Factory{salt: keccak256(bytes(name))}( - name, - ensReverseRegistrar, - msg.sender, consolidationSysContract, withdrawalSysContract, - depositSysContract + depositSysContract, + name, + ensReverseRegistrar, + msg.sender ); vm.stopBroadcast(); diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol index 2a8e908..c67caf3 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2.sol @@ -104,7 +104,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { address public immutable recoveryAddress; address public immutable principalRecipient; address public immutable rewardRecipient; - uint256 public immutable principalThreshold; + uint64 public immutable principalThreshold; /// ----------------------------------------------------------------------- /// storage - mutables @@ -112,7 +112,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// @dev set to `true` after owner is initialized bool public initialized; - /// Amount of principal stake done via deposit() calls + /// Amount of principal stake (wei) done via deposit() calls uint256 public amountOfPrincipalStake; /// Amount of active balance set aside for pulls @@ -130,17 +130,17 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { address _consolidationSystemContract, address _withdrawalSystemContract, address _depositSystemContract, - address _recoveryAddress, address _principalRecipient, address _rewardRecipient, - uint256 _principalThreshold + address _recoveryAddress, + uint64 _principalThreshold ) { consolidationSystemContract = _consolidationSystemContract; withdrawalSystemContract = _withdrawalSystemContract; depositSystemContract = _depositSystemContract; - recoveryAddress = _recoveryAddress; principalRecipient = _principalRecipient; rewardRecipient = _rewardRecipient; + recoveryAddress = _recoveryAddress; principalThreshold = _principalThreshold; } @@ -173,20 +173,21 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// @dev This function is a proxy to the deposit() function on the depositSystemContract. /// The deposited amount is accounted for in the amountOfPrincipalStake, which is used /// to determine the principalRecipient's share of the funds to be distributed. - /// Any deposits made directly to the depositSystemContract will not be accounted for. + /// Any deposits made directly to the depositSystemContract will not be accounted for + /// and will be sent to the rewardRecipient address. function deposit( bytes calldata pubkey, bytes calldata withdrawal_credentials, bytes calldata signature, bytes32 deposit_data_root ) external payable { + amountOfPrincipalStake += msg.value; IDepositContract(depositSystemContract).deposit{value: msg.value}( pubkey, withdrawal_credentials, signature, deposit_data_root ); - amountOfPrincipalStake += msg.value; } /// Distributes target token inside the contract to recipients @@ -212,8 +213,8 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { bytes[] calldata sourcePubKeys, bytes calldata targetPubKey ) external payable onlyOwnerOrRoles(CONSOLIDATION_ROLE) { - if (sourcePubKeys.length == 0 || targetPubKey.length != 48) revert InvalidRequest_Params(); - if (sourcePubKeys.length > 63) revert InvalidRequest_Params(); + if (sourcePubKeys.length == 0 || sourcePubKeys.length > 63 || targetPubKey.length != 48) + revert InvalidRequest_Params(); uint256 remainingFee = msg.value; uint256 len = sourcePubKeys.length; @@ -237,7 +238,8 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// Request partial/full withdrawal from the EIP7002 system contract /// @dev the caller must compute the fee before calling and send a sufficient msg.value amount /// excess amount will be refunded - /// withdrawals that leave a validator with (0..32) ether will cause the transaction to fail + /// withdrawals that leave a validator with (0..32) ether, + // will only withdraw an amount that leaves the validator at 32 ether /// @param pubKeys validator public keys /// @param amounts withdrawal amounts in gwei (must be >= principalThreshold) function requestWithdrawal( @@ -250,8 +252,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { uint256 len = pubKeys.length; for (uint256 i; i < len; ) { - uint256 amountWei = amounts[i] * 1 gwei; - if (amountWei < principalThreshold) revert InvalidRequest_Params(); + if (amounts[i] < principalThreshold) revert InvalidRequest_Params(); uint256 _currentFee = _computeSystemContractFee(withdrawalSystemContract); if (_currentFee > remainingFee) revert InvalidRequest_NotEnoughFee(); @@ -382,13 +383,14 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { uint256 currentbalance = address(this).balance; uint256 _memoryFundsPendingWithdrawal = uint256(fundsPendingWithdrawal); uint256 _fundsToBeDistributed = currentbalance - _memoryFundsPendingWithdrawal; + uint256 principalThresholdWei = uint256(principalThreshold) * 1e9; // determine which recipeint is getting paid based on funds to be distributed uint256 _principalPayout = 0; uint256 _rewardPayout = 0; unchecked { - if (_fundsToBeDistributed >= principalThreshold && amountOfPrincipalStake > 0) { + if (_fundsToBeDistributed >= principalThresholdWei && amountOfPrincipalStake > 0) { if (_fundsToBeDistributed > amountOfPrincipalStake) { // this means there is reward part of the funds to be distributed _principalPayout = amountOfPrincipalStake; diff --git a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol index 5ef857f..b795da9 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol @@ -31,14 +31,14 @@ contract OptimisticWithdrawalRecipientV2Factory { /// @param recoveryAddress Address to recover non-OWR tokens to /// @param principalRecipient Address to distribute principal payment to /// @param rewardRecipient Address to distribute reward payment to - /// @param principalThreshold Principal vs rewards classification threshold + /// @param principalThreshold Principal vs rewards classification threshold (gwei) event CreateOWRecipient( address indexed owr, address indexed owner, address recoveryAddress, address principalRecipient, address rewardRecipient, - uint256 principalThreshold + uint64 principalThreshold ); /// ----------------------------------------------------------------------- @@ -53,12 +53,12 @@ contract OptimisticWithdrawalRecipientV2Factory { /// constructor /// ----------------------------------------------------------------------- - /// @param _ensName ENS name to register - /// @param _ensReverseRegistrar ENS reverse registrar address - /// @param _ensOwner ENS owner address /// @param _consolidationSystemContract Consolidation system contract address /// @param _withdrawalSystemContract Withdrawal system contract address /// @param _depositSystemContract Deposit system contract address + /// @param _ensName ENS name to register + /// @param _ensReverseRegistrar ENS reverse registrar address + /// @param _ensOwner ENS owner address /// @dev System contracts are expected to be deployed at: /// Consolidation: 0x00431F263cE400f4455c2dCf564e53007Ca4bbBb /// https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7251.md#constants @@ -68,12 +68,12 @@ contract OptimisticWithdrawalRecipientV2Factory { /// Deposit Sepolia: 0x7f02C3E3c98b133055B8B348B2Ac625669Ed295D /// Deposit Mainnet: 0x00000000219ab540356cBB839Cbe05303d7705Fa constructor( - string memory _ensName, - address _ensReverseRegistrar, - address _ensOwner, address _consolidationSystemContract, address _withdrawalSystemContract, - address _depositSystemContract + address _depositSystemContract, + string memory _ensName, + address _ensReverseRegistrar, + address _ensOwner ) { consolidationSystemContract = _consolidationSystemContract; withdrawalSystemContract = _withdrawalSystemContract; @@ -92,33 +92,34 @@ contract OptimisticWithdrawalRecipientV2Factory { /// ----------------------------------------------------------------------- /// Create a new OptimisticWithdrawalRecipientV2 instance - /// @param recoveryAddress Address to recover tokens to - /// If this address is 0x0, recovery of unrelated tokens can be completed by - /// either the principal or reward recipients. If this address is set, only - /// this address can recover ERC20 tokens allocated to the OWRV2 contract. + /// @param recoveryAddress Address to receive ERC20 tokens transferred to this contract. + /// If this address is 0x0, ERC20 tokens owned by this contract can be permissionlessly + /// transferred to *either* the principal or reward recipient addresses. If this address is set, + /// ERC20 tokens can only be pushed to the recoveryAddress set. + /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 instance /// @param principalRecipient Address to distribute principal payments to /// @param rewardRecipient Address to distribute reward payments to - /// @param principalThreshold Principal vs rewards classification threshold - /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 instance + /// @param recoveryAddress Address to recover non-OWR tokens to + /// @param principalThreshold Principal vs rewards classification threshold (gwei) /// @return owr Address of the new OptimisticWithdrawalRecipientV2 instance function createOWRecipient( - address recoveryAddress, + address owner, address principalRecipient, address rewardRecipient, - uint256 principalThreshold, - address owner + address recoveryAddress, + uint64 principalThreshold ) external returns (OptimisticWithdrawalRecipientV2 owr) { if (principalRecipient == address(0) || rewardRecipient == address(0)) revert Invalid__Recipients(); if (principalThreshold == 0) revert Invalid__ZeroThreshold(); - if (principalThreshold > 2048 ether) revert Invalid__ThresholdTooLarge(); + if (principalThreshold > 2048 * 1e9) revert Invalid__ThresholdTooLarge(); owr = new OptimisticWithdrawalRecipientV2( consolidationSystemContract, withdrawalSystemContract, depositSystemContract, - recoveryAddress, principalRecipient, rewardRecipient, + recoveryAddress, principalThreshold ); owr.initialize(owner); diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol index a8f1d16..e5106bb 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol @@ -22,7 +22,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { address public ENS_REVERSE_REGISTRAR = 0x084b1c3C81545d370f3634392De611CaaBFf8148; - uint256 public constant BALANCE_CLASSIFICATION_THRESHOLD = 16 ether; + uint64 public constant BALANCE_CLASSIFICATION_THRESHOLD_GWEI = 16 ether / 1 gwei; uint256 public constant INITIAL_DEPOSIT_AMOUNT = 32 ether; OptimisticWithdrawalRecipientV2Factory public owrFactory; @@ -38,7 +38,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { address internal recoveryAddress; address internal principalRecipient; address internal rewardsRecipient; - uint256 internal principalThreshold; + uint64 internal principalThreshold; function setUp() public { vm.mockCall( @@ -57,12 +57,12 @@ contract OptimisticWithdrawalRecipientV2Test is Test { depositMock = new DepositContractMock(); owrFactory = new OptimisticWithdrawalRecipientV2Factory( - "demo.obol.eth", - ENS_REVERSE_REGISTRAR, - address(this), address(consolidationMock), address(withdrawalMock), - address(depositMock) + address(depositMock), + "demo.obol.eth", + ENS_REVERSE_REGISTRAR, + address(this) ); mERC20 = new MockERC20("demo", "DMT", 18); @@ -71,21 +71,21 @@ contract OptimisticWithdrawalRecipientV2Test is Test { recoveryAddress = makeAddr("recoveryAddress"); principalRecipient = makeAddr("principalRecipient"); rewardsRecipient = makeAddr("rewardsRecipient"); - principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD; + principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD_GWEI; owrETH = owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); owrETH_OR = owrFactory.createOWRecipient( - address(0), + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + address(0), + principalThreshold ); owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); @@ -96,7 +96,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { assertEq(owrETH.recoveryAddress(), recoveryAddress, "invalid recovery address"); assertEq(owrETH.principalRecipient(), principalRecipient, "invalid principal recipient"); assertEq(owrETH.rewardRecipient(), rewardsRecipient, "invalid rewards recipient"); - assertEq(owrETH.principalThreshold(), BALANCE_CLASSIFICATION_THRESHOLD, "invalid principal threshold"); + assertEq(owrETH.principalThreshold(), BALANCE_CLASSIFICATION_THRESHOLD_GWEI, "invalid principal threshold"); } function testInitialization() public { @@ -245,13 +245,13 @@ contract OptimisticWithdrawalRecipientV2Test is Test { owrETH.requestWithdrawal{value: 1 ether}(empty, amounts); // Not enough fee (1 wei is the minimum fee) - uint256 validAmount = principalThreshold / 1 gwei; + uint256 validAmount = principalThreshold; amounts[0] = uint64(validAmount); vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector); owrETH.requestWithdrawal{value: 0}(single, amounts); // Amount below principalThreshold - uint256 lowAmount = (principalThreshold - 1 wei) / 1 gwei; + uint256 lowAmount = principalThreshold / 2; amounts[0] = uint64(lowAmount); vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); owrETH.requestWithdrawal{value: 100 wei}(single, amounts); @@ -275,7 +275,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { bytes[] memory pubkeys = new bytes[](1); uint64[] memory amounts = new uint64[](1); bytes memory pubkey = new bytes(48); - uint64 amount = uint64(principalThreshold / 1 gwei); + uint64 amount = uint64(principalThreshold); for (uint256 i = 0; i < 48; i++) { pubkey[i] = bytes1(0xAB); } @@ -316,7 +316,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { pubkey[i] = bytes1(i + 1); } pubkeys[i] = pubkey; - amounts[i] = uint64(principalThreshold / 1 gwei + i); + amounts[i] = uint64(principalThreshold + i); } address _user = vm.addr(0x1); @@ -503,7 +503,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { function testCannot_reenterOWR() public { OWRV2Reentrancy wr = new OWRV2Reentrancy(); - owrETH = owrFactory.createOWRecipient(recoveryAddress, address(wr), rewardsRecipient, 1 ether, address(this)); + owrETH = owrFactory.createOWRecipient(address(this), address(wr), rewardsRecipient, recoveryAddress, 1e9); owrETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); address(owrETH).safeTransferETH(33 ether); @@ -643,7 +643,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { } function testFuzzCan_distributeDepositsToRecipients( - uint96 _threshold, + uint64 _threshold, uint8 _numDeposits, uint256 _ethAmount, uint256 _erc20Amount @@ -651,15 +651,15 @@ contract OptimisticWithdrawalRecipientV2Test is Test { _ethAmount = uint256(bound(_ethAmount, 0.01 ether, 34 ether)); _erc20Amount = uint256(bound(_erc20Amount, 0.01 ether, 34 ether)); vm.assume(_numDeposits > 0); - vm.assume(_threshold > 0 && _threshold < 2048 ether); - principalThreshold = _threshold; + vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); + uint256 principalThresholdWei = uint256(_threshold) * 1e9; owrETH = owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + _threshold ); owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); @@ -675,23 +675,23 @@ contract OptimisticWithdrawalRecipientV2Test is Test { // assertEq(owrETH.distributedFunds(), _totalETHAmount, "undistributed funds"); assertEq(owrETH.fundsPendingWithdrawal(), 0 ether, "funds pending withdraw"); - if (BALANCE_CLASSIFICATION_THRESHOLD > _totalETHAmount) { + if (principalThresholdWei > _totalETHAmount) { // then all of the deposit should be classified as reward assertEq(principalRecipient.balance, 0, "should not classify reward as principal"); assertEq(rewardsRecipient.balance, _totalETHAmount, "invalid amount"); } - if (_ethAmount > BALANCE_CLASSIFICATION_THRESHOLD) { + if (_ethAmount > principalThresholdWei) { // then all of reward classified as principal // but check if _totalETHAmount > first threshold - if (_totalETHAmount > principalThreshold) { + if (_totalETHAmount > principalThresholdWei) { // there is reward - assertEq(principalRecipient.balance, principalThreshold, "invalid amount"); + assertEq(principalRecipient.balance, principalThresholdWei, "invalid amount"); assertEq( rewardsRecipient.balance, - _totalETHAmount - principalThreshold, + _totalETHAmount - principalThresholdWei, "should not classify principal as reward" ); } else { @@ -704,7 +704,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { } function testFuzzCan_distributePullDepositsToRecipients( - uint96 _threshold, + uint64 _threshold, uint8 _numDeposits, uint256 _ethAmount, uint256 _erc20Amount @@ -712,15 +712,15 @@ contract OptimisticWithdrawalRecipientV2Test is Test { _ethAmount = uint256(bound(_ethAmount, 0.01 ether, 40 ether)); _erc20Amount = uint256(bound(_erc20Amount, 0.01 ether, 40 ether)); vm.assume(_numDeposits > 0); - vm.assume(_threshold > 0 && _threshold < 2048 ether); - principalThreshold = _threshold; + vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); + uint256 principalThresholdWei = uint256(_threshold) * 1e9; owrETH = owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + _threshold ); owrETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); @@ -739,8 +739,8 @@ contract OptimisticWithdrawalRecipientV2Test is Test { uint256 principal = owrETH.getPullBalance(principalRecipient); assertEq( owrETH.getPullBalance(principalRecipient), - (_ethAmount >= BALANCE_CLASSIFICATION_THRESHOLD) - ? principalThreshold > _totalETHAmount ? _totalETHAmount : principalThreshold + (_ethAmount >= principalThresholdWei) + ? principalThresholdWei > _totalETHAmount ? _totalETHAmount : principalThresholdWei : 0, "5/invalid recipient balance" ); @@ -748,8 +748,8 @@ contract OptimisticWithdrawalRecipientV2Test is Test { uint256 reward = owrETH.getPullBalance(rewardsRecipient); assertEq( owrETH.getPullBalance(rewardsRecipient), - (_ethAmount >= BALANCE_CLASSIFICATION_THRESHOLD) - ? _totalETHAmount > principalThreshold ? (_totalETHAmount - principalThreshold) : 0 + (_ethAmount >= principalThresholdWei) + ? _totalETHAmount > principalThresholdWei ? (_totalETHAmount - principalThresholdWei) : 0 : _totalETHAmount, "6/invalid recipient balance" ); diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol index 2ac8905..4b03ff5 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol @@ -16,12 +16,12 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { address recoveryAddress, address principalRecipient, address rewardRecipient, - uint256 principalThreshold + uint64 principalThreshold ); address public ENS_REVERSE_REGISTRAR = 0x084b1c3C81545d370f3634392De611CaaBFf8148; - uint256 public constant BALANCE_CLASSIFICATION_THRESHOLD = 16 ether; + uint64 public constant BALANCE_CLASSIFICATION_THRESHOLD_GWEI = 16 ether / 1 gwei; SystemContractMock consolidationMock; SystemContractMock withdrawalMock; @@ -31,7 +31,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { address public recoveryAddress; address public principalRecipient; address public rewardsRecipient; - uint256 public principalThreshold; + uint64 public principalThreshold; function setUp() public { vm.mockCall( @@ -50,27 +50,27 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { depositMock = new DepositContractMock(); owrFactory = new OptimisticWithdrawalRecipientV2Factory( - "demo.obol.eth", - ENS_REVERSE_REGISTRAR, - address(this), address(consolidationMock), address(withdrawalMock), - address(depositMock) + address(depositMock), + "demo.obol.eth", + ENS_REVERSE_REGISTRAR, + address(this) ); recoveryAddress = makeAddr("recoveryAddress"); principalRecipient = makeAddr("principalRecipient"); rewardsRecipient = makeAddr("rewardsRecipient"); - principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD; + principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD_GWEI; } function testCan_createOWRecipient() public { OptimisticWithdrawalRecipientV2 owr = owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); assertEq(owr.owner(), address(this)); assertEq(address(owr.consolidationSystemContract()), address(consolidationMock)); @@ -78,11 +78,11 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { recoveryAddress = address(0); owr = owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); assertEq(owr.recoveryAddress(), address(0)); } @@ -100,11 +100,11 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { principalThreshold ); owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); recoveryAddress = address(0); @@ -120,23 +120,23 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { principalThreshold ); owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); } function testCannot_createWithInvalidRecipients() public { vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, address(0), rewardsRecipient, principalThreshold, address(this)); + owrFactory.createOWRecipient(address(this), address(0), rewardsRecipient, recoveryAddress, principalThreshold); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, address(0), address(0), principalThreshold, address(this)); + owrFactory.createOWRecipient(address(this), address(0), address(0), recoveryAddress, principalThreshold); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, address(0), principalThreshold, address(this)); + owrFactory.createOWRecipient(address(this), principalRecipient, address(0), recoveryAddress, principalThreshold); } function testCannot_createWithInvalidThreshold() public { @@ -144,20 +144,20 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, rewardsRecipient, - type(uint128).max, - address(this) + recoveryAddress, + type(uint64).max ); } @@ -165,8 +165,8 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { /// Fuzzing Tests /// ---------------------------------------------------------------------- - function testFuzzCan_createOWRecipient(uint96 _threshold) public { - vm.assume(_threshold > 0 && _threshold < 2048 ether); + function testFuzzCan_createOWRecipient(uint64 _threshold) public { + vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); vm.expectEmit(false, true, true, true); emit CreateOWRecipient( @@ -177,7 +177,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { rewardsRecipient, _threshold ); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, rewardsRecipient, _threshold, address(this)); + owrFactory.createOWRecipient(address(this), principalRecipient, rewardsRecipient, recoveryAddress, _threshold); } function testFuzzCannot_CreateWithZeroThreshold(address _rewardsRecipient) public { @@ -187,19 +187,19 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { // eth vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); owrFactory.createOWRecipient( - recoveryAddress, + address(this), principalRecipient, _rewardsRecipient, - principalThreshold, - address(this) + recoveryAddress, + principalThreshold ); } - function testFuzzCannot_CreateWithLargeThreshold(address _rewardsRecipient, uint256 _threshold) public { - vm.assume(_threshold > type(uint96).max); + function testFuzzCannot_CreateWithLargeThreshold(address _rewardsRecipient, uint64 _threshold) public { + vm.assume(_threshold > 2048 * 1e9); vm.assume(_rewardsRecipient != address(0)); vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); - owrFactory.createOWRecipient(recoveryAddress, principalRecipient, _rewardsRecipient, _threshold, address(this)); + owrFactory.createOWRecipient(address(this), principalRecipient, _rewardsRecipient, recoveryAddress, _threshold); } } From 8c1231224f7bf5c9c93b454f204783df3b67a89d Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Thu, 6 Feb 2025 14:34:37 +0300 Subject: [PATCH 5/6] Addressed PR feedback --- src/owr/OptimisticWithdrawalRecipientV2.sol | 22 +++++-------------- ...OptimisticWithdrawalRecipientV2Factory.sol | 2 +- .../owr/OptimisticWithdrawalRecipientV2.t.sol | 19 +++------------- 3 files changed, 10 insertions(+), 33 deletions(-) diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/owr/OptimisticWithdrawalRecipientV2.sol index c67caf3..11be985 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2.sol @@ -23,9 +23,6 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// errors /// ----------------------------------------------------------------------- - // The instance is already initialized - error Invalid_AlreadyInitialized(); - // Invalid request params, e.g. empty input error InvalidRequest_Params(); @@ -109,8 +106,6 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// ----------------------------------------------------------------------- /// storage - mutables /// ----------------------------------------------------------------------- - /// @dev set to `true` after owner is initialized - bool public initialized; /// Amount of principal stake (wei) done via deposit() calls uint256 public amountOfPrincipalStake; @@ -130,6 +125,7 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { address _consolidationSystemContract, address _withdrawalSystemContract, address _depositSystemContract, + address _owner, address _principalRecipient, address _rewardRecipient, address _recoveryAddress, @@ -142,6 +138,8 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { rewardRecipient = _rewardRecipient; recoveryAddress = _recoveryAddress; principalThreshold = _principalThreshold; + + _initializeOwner(_owner); } /// ----------------------------------------------------------------------- @@ -152,14 +150,6 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// functions - public & external /// ----------------------------------------------------------------------- - /// @dev initializes the owner - /// @param _owner the owner address - function initialize(address _owner) public { - if (initialized) revert Invalid_AlreadyInitialized(); - _initializeOwner(_owner); - initialized = true; - } - /// @dev Fallback function to receive ETH /// Because we do not use Clone, we must implement this explicitly receive() external payable {} @@ -241,7 +231,9 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { /// withdrawals that leave a validator with (0..32) ether, // will only withdraw an amount that leaves the validator at 32 ether /// @param pubKeys validator public keys - /// @param amounts withdrawal amounts in gwei (must be >= principalThreshold) + /// @param amounts withdrawal amounts in gwei + /// any amount below principalThreshold will be distributed as reward + /// any amount >= principalThreshold will be distributed as principal function requestWithdrawal( bytes[] calldata pubKeys, uint64[] calldata amounts @@ -252,8 +244,6 @@ contract OptimisticWithdrawalRecipientV2 is OwnableRoles { uint256 len = pubKeys.length; for (uint256 i; i < len; ) { - if (amounts[i] < principalThreshold) revert InvalidRequest_Params(); - uint256 _currentFee = _computeSystemContractFee(withdrawalSystemContract); if (_currentFee > remainingFee) revert InvalidRequest_NotEnoughFee(); diff --git a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol index b795da9..fbe7ca8 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol +++ b/src/owr/OptimisticWithdrawalRecipientV2Factory.sol @@ -117,12 +117,12 @@ contract OptimisticWithdrawalRecipientV2Factory { consolidationSystemContract, withdrawalSystemContract, depositSystemContract, + owner, principalRecipient, rewardRecipient, recoveryAddress, principalThreshold ); - owr.initialize(owner); emit CreateOWRecipient( address(owr), diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol index e5106bb..f7872e1 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol @@ -99,17 +99,10 @@ contract OptimisticWithdrawalRecipientV2Test is Test { assertEq(owrETH.principalThreshold(), BALANCE_CLASSIFICATION_THRESHOLD_GWEI, "invalid principal threshold"); } - function testInitialization() public { - assertTrue(owrETH.initialized()); + function testOwnerInitialization() public { assertEq(owrETH.owner(), address(this)); } - function testReinitialization() public { - assertTrue(owrETH.initialized()); - vm.expectRevert(OptimisticWithdrawalRecipientV2.Invalid_AlreadyInitialized.selector); - owrETH.initialize(address(this)); - } - function testDeposit() public { // Initial deposit is done in setUp() assertEq(address(owrETH).balance, INITIAL_DEPOSIT_AMOUNT); @@ -250,12 +243,6 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector); owrETH.requestWithdrawal{value: 0}(single, amounts); - // Amount below principalThreshold - uint256 lowAmount = principalThreshold / 2; - amounts[0] = uint64(lowAmount); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); - owrETH.requestWithdrawal{value: 100 wei}(single, amounts); - // Failed get_fee() request uint256 realFee = withdrawalMock.fakeExponential(0); amounts[0] = uint64(validAmount); @@ -348,8 +335,8 @@ contract OptimisticWithdrawalRecipientV2Test is Test { } function testReceiveERC20() public { - address(mERC20).safeTransfer(address(owrETH), 1 ether); - assertEq(mERC20.balanceOf(address(owrETH)), 1 ether); + address(mERC20).safeTransfer(address(owrETH), 1e10); + assertEq(mERC20.balanceOf(address(owrETH)), 1e10); } function testCan_recoverNonOWRFundsToRecipient() public { From b22a7165709cc8aa0f20450928382aada8af8307 Mon Sep 17 00:00:00 2001 From: Andrei Smirnov Date: Thu, 6 Feb 2025 15:46:44 +0300 Subject: [PATCH 6/6] Renamed to ObolValidatorManager --- ...s.sol => CreateObolValidatorManager.s.sol} | 13 ++- script/DistributeFunds.s.sol | 8 +- ...> ObolValidatorManagerFactoryScript.s.sol} | 13 +-- script/RequestConsolidation.s.sol | 8 +- script/RequestWithdrawal.s.sol | 8 +- .../ObolValidatorManager.sol} | 4 +- .../ObolValidatorManagerFactory.sol} | 30 +++---- .../ObolValidatorManager.t.sol} | 56 ++++++------- .../ObolValidatorManagerFactory.t.sol} | 80 ++++++++++++------- .../ovm/ObolValidatorManagerReentrancy.sol | 14 ++++ .../mocks/DepositContractMock.sol | 0 .../{owr => ovm}/mocks/SystemContractMock.sol | 0 src/test/owr/OWRV2Reentrancy.sol | 14 ---- 13 files changed, 137 insertions(+), 111 deletions(-) rename script/{CreateOWRV2.s.sol => CreateObolValidatorManager.s.sol} (64%) rename script/{OWRV2FactoryScript.s.sol => ObolValidatorManagerFactoryScript.s.sol} (75%) rename src/{owr/OptimisticWithdrawalRecipientV2.sol => ovm/ObolValidatorManager.sol} (99%) rename src/{owr/OptimisticWithdrawalRecipientV2Factory.sol => ovm/ObolValidatorManagerFactory.sol} (84%) rename src/test/{owr/OptimisticWithdrawalRecipientV2.t.sol => ovm/ObolValidatorManager.t.sol} (92%) rename src/test/{owr/OptimisticWithdrawalRecipientV2Factory.t.sol => ovm/ObolValidatorManagerFactory.t.sol} (68%) create mode 100644 src/test/ovm/ObolValidatorManagerReentrancy.sol rename src/test/{owr => ovm}/mocks/DepositContractMock.sol (100%) rename src/test/{owr => ovm}/mocks/SystemContractMock.sol (100%) delete mode 100644 src/test/owr/OWRV2Reentrancy.sol diff --git a/script/CreateOWRV2.s.sol b/script/CreateObolValidatorManager.s.sol similarity index 64% rename from script/CreateOWRV2.s.sol rename to script/CreateObolValidatorManager.s.sol index 65b4ff4..3b8d9a4 100644 --- a/script/CreateOWRV2.s.sol +++ b/script/CreateObolValidatorManager.s.sol @@ -2,23 +2,23 @@ pragma solidity ^0.8.19; import "forge-std/Script.sol"; -import {OptimisticWithdrawalRecipientV2Factory} from "../src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; +import {ObolValidatorManagerFactory} from "../src/ovm/ObolValidatorManagerFactory.sol"; // -// This script creates a new OptimisticWithdrawalRecipientV2 contract. +// This script creates a new ObolValidatorManager contract. // To run this script, the following environment variables must be set: // - PRIVATE_KEY: the private key of the account that will deploy the contract // Example usage: -// forge script script/CreateOWRecipientScript.s.sol --sig "run(address)" \ +// forge script script/CreateObolValidatorManagerScript.s.sol --sig "run(address)" \ // --rpc-url https://rpc.pectra-devnet-5.ethpandaops.io/ --broadcast "" // -contract CreateOWRecipientScript is Script { +contract CreateObolValidatorManagerScript is Script { function run(address deployedOWRV2Factory) external { uint256 privKey = vm.envUint("PRIVATE_KEY"); vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2Factory factory = OptimisticWithdrawalRecipientV2Factory(deployedOWRV2Factory); + ObolValidatorManagerFactory factory = ObolValidatorManagerFactory(deployedOWRV2Factory); address owner = msg.sender; address recoveryAddress = msg.sender; @@ -26,8 +26,7 @@ contract CreateOWRecipientScript is Script { address rewardsRecipient = msg.sender; uint64 principalThreshold = 16 ether / 1 gwei; - // Call the createOWRecipient function - factory.createOWRecipient( + factory.createObolValidatorManager( owner, principalRecipient, rewardsRecipient, diff --git a/script/DistributeFunds.s.sol b/script/DistributeFunds.s.sol index f6580af..c9e39c7 100644 --- a/script/DistributeFunds.s.sol +++ b/script/DistributeFunds.s.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.19; import "forge-std/Script.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; // -// This script calls distributeFunds() for a OptimisticWithdrawalRecipientV2 contract. +// This script calls distributeFunds() for a ObolValidatorManager contract. // To run this script, the following environment variables must be set: // - PRIVATE_KEY: the private key of the account that will deploy the contract // Example usage: @@ -18,8 +18,8 @@ contract DistributeFunds is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(deployedOWRV2)); - owr.distributeFunds(); + ObolValidatorManager ovm = ObolValidatorManager(payable(deployedOWRV2)); + ovm.distributeFunds(); vm.stopBroadcast(); } diff --git a/script/OWRV2FactoryScript.s.sol b/script/ObolValidatorManagerFactoryScript.s.sol similarity index 75% rename from script/OWRV2FactoryScript.s.sol rename to script/ObolValidatorManagerFactoryScript.s.sol index bf23410..aaad850 100644 --- a/script/OWRV2FactoryScript.s.sol +++ b/script/ObolValidatorManagerFactoryScript.s.sol @@ -2,18 +2,19 @@ pragma solidity ^0.8.19; import "forge-std/Script.sol"; -import {OptimisticWithdrawalRecipientV2Factory} from "src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; +import "forge-std/console.sol"; +import {ObolValidatorManagerFactory} from "src/ovm/ObolValidatorManagerFactory.sol"; // -// This script deploys a new OptimisticWithdrawalRecipientV2Factory contract. +// This script deploys a new ObolValidatorManagerFactory contract. // To run this script, the following environment variables must be set: // - PRIVATE_KEY: the private key of the account that will deploy the contract // Please set ensReverseRegistrar before running this script. // Example usage: -// forge script script/OWRV2FactoryScript.s.sol --sig "run(string)" +// forge script script/ObolValidatorManagerFactoryScript.s.sol --sig "run(string)" // --rpc-url https://rpc.pectra-devnet-5.ethpandaops.io/ --broadcast "demo" // -contract OWRV2FactoryScript is Script { +contract ObolValidatorManagerFactoryScript is Script { // From https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7251.md#execution-layer address constant consolidationSysContract = 0x00431F263cE400f4455c2dCf564e53007Ca4bbBb; // From https://github.com/ethereum/EIPs/blob/d96625a4dcbbe2572fa006f062bd02b4582eefd5/EIPS/eip-7002.md#configuration @@ -36,7 +37,7 @@ contract OWRV2FactoryScript is Script { vm.startBroadcast(privKey); - new OptimisticWithdrawalRecipientV2Factory{salt: keccak256(bytes(name))}( + ObolValidatorManagerFactory factory = new ObolValidatorManagerFactory{salt: keccak256(bytes(name))}( consolidationSysContract, withdrawalSysContract, depositSysContract, @@ -45,6 +46,8 @@ contract OWRV2FactoryScript is Script { msg.sender ); + console.log('ObolValidatorManagerFactory deployed at: ', address(factory)); + vm.stopBroadcast(); } } diff --git a/script/RequestConsolidation.s.sol b/script/RequestConsolidation.s.sol index 7f481ad..60a52dc 100644 --- a/script/RequestConsolidation.s.sol +++ b/script/RequestConsolidation.s.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.19; import "forge-std/Script.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; // -// This script calls requestConsolidation() for a OptimisticWithdrawalRecipientV2 contract. +// This script calls requestConsolidation() for a ObolValidatorManager contract. // To run this script, the following environment variables must be set: // - PRIVATE_KEY: the private key of the account that will deploy the contract // Example usage: @@ -19,14 +19,14 @@ contract RequestConsolidation is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(owrv2)); + ObolValidatorManager ovm = ObolValidatorManager(payable(owrv2)); // Call the function on the deployed contract bytes[] memory sourcePubKeys = new bytes[](1); sourcePubKeys[0] = src; // Estimated total gas used for script: 162523 - owr.requestConsolidation{value: 100 wei}(sourcePubKeys, dst); + ovm.requestConsolidation{value: 100 wei}(sourcePubKeys, dst); vm.stopBroadcast(); } diff --git a/script/RequestWithdrawal.s.sol b/script/RequestWithdrawal.s.sol index 3d1cc54..0fb81dd 100644 --- a/script/RequestWithdrawal.s.sol +++ b/script/RequestWithdrawal.s.sol @@ -2,10 +2,10 @@ pragma solidity ^0.8.19; import "forge-std/Script.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; // -// This script calls requestWithdrawal() for a OptimisticWithdrawalRecipientV2 contract. +// This script calls requestWithdrawal() for a ObolValidatorManager contract. // To run this script, the following environment variables must be set: // - PRIVATE_KEY: the private key of the account that will deploy the contract // Example usage: @@ -19,7 +19,7 @@ contract RequestWithdrawal is Script { vm.startBroadcast(privKey); - OptimisticWithdrawalRecipientV2 owr = OptimisticWithdrawalRecipientV2(payable(owrv2)); + ObolValidatorManager ovm = ObolValidatorManager(payable(owrv2)); bytes[] memory pubKeys = new bytes[](1); pubKeys[0] = pubkey; @@ -28,7 +28,7 @@ contract RequestWithdrawal is Script { amounts[0] = amount; // Estimated total gas used for script: 219325 - owr.requestWithdrawal{value: 100 wei}(pubKeys, amounts); + ovm.requestWithdrawal{value: 100 wei}(pubKeys, amounts); vm.stopBroadcast(); } diff --git a/src/owr/OptimisticWithdrawalRecipientV2.sol b/src/ovm/ObolValidatorManager.sol similarity index 99% rename from src/owr/OptimisticWithdrawalRecipientV2.sol rename to src/ovm/ObolValidatorManager.sol index 11be985..8d1fbcd 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2.sol +++ b/src/ovm/ObolValidatorManager.sol @@ -6,13 +6,13 @@ import {OwnableRoles} from "solady/auth/OwnableRoles.sol"; import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; import {IDepositContract} from "../interfaces/IDepositContract.sol"; -/// @title OptimisticWithdrawalRecipientV2 +/// @title ObolValidatorManager /// @author Obol /// @notice A maximally-composable contract that distributes payments /// based on threshold to it's recipients. /// @dev Only ETH can be distributed for a given deployment. There is a /// recovery method for tokens sent by accident. -contract OptimisticWithdrawalRecipientV2 is OwnableRoles { +contract ObolValidatorManager is OwnableRoles { /// ----------------------------------------------------------------------- /// libraries /// ----------------------------------------------------------------------- diff --git a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol b/src/ovm/ObolValidatorManagerFactory.sol similarity index 84% rename from src/owr/OptimisticWithdrawalRecipientV2Factory.sol rename to src/ovm/ObolValidatorManagerFactory.sol index fbe7ca8..18e6878 100644 --- a/src/owr/OptimisticWithdrawalRecipientV2Factory.sol +++ b/src/ovm/ObolValidatorManagerFactory.sol @@ -1,13 +1,13 @@ // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity 0.8.19; -import {OptimisticWithdrawalRecipientV2} from "./OptimisticWithdrawalRecipientV2.sol"; +import {ObolValidatorManager} from "./ObolValidatorManager.sol"; import {IENSReverseRegistrar} from "../interfaces/IENSReverseRegistrar.sol"; -/// @title OptimisticWithdrawalRecipientV2Factory +/// @title ObolValidatorManagerFactory /// @author Obol -/// @notice A factory contract for deploying OptimisticWithdrawalRecipientV2. -contract OptimisticWithdrawalRecipientV2Factory { +/// @notice A factory contract for deploying ObolValidatorManager. +contract ObolValidatorManagerFactory { /// ----------------------------------------------------------------------- /// errors /// ----------------------------------------------------------------------- @@ -25,14 +25,14 @@ contract OptimisticWithdrawalRecipientV2Factory { /// events /// ----------------------------------------------------------------------- - /// Emitted after a new OptimisticWithdrawalRecipientV2 module is deployed - /// @param owr Address of newly created OptimisticWithdrawalRecipientV2 instance - /// @param owner Owner of newly created OptimisticWithdrawalRecipientV2 instance + /// Emitted after a new ObolValidatorManager instance is deployed + /// @param owr Address of newly created ObolValidatorManager instance + /// @param owner Owner of newly created ObolValidatorManager instance /// @param recoveryAddress Address to recover non-OWR tokens to /// @param principalRecipient Address to distribute principal payment to /// @param rewardRecipient Address to distribute reward payment to /// @param principalThreshold Principal vs rewards classification threshold (gwei) - event CreateOWRecipient( + event CreateObolValidatorManager( address indexed owr, address indexed owner, address recoveryAddress, @@ -91,29 +91,29 @@ contract OptimisticWithdrawalRecipientV2Factory { /// functions - public & external /// ----------------------------------------------------------------------- - /// Create a new OptimisticWithdrawalRecipientV2 instance + /// Create a new ObolValidatorManager instance /// @param recoveryAddress Address to receive ERC20 tokens transferred to this contract. /// If this address is 0x0, ERC20 tokens owned by this contract can be permissionlessly /// transferred to *either* the principal or reward recipient addresses. If this address is set, /// ERC20 tokens can only be pushed to the recoveryAddress set. - /// @param owner Owner of the new OptimisticWithdrawalRecipientV2 instance + /// @param owner Owner of the new ObolValidatorManager instance /// @param principalRecipient Address to distribute principal payments to /// @param rewardRecipient Address to distribute reward payments to /// @param recoveryAddress Address to recover non-OWR tokens to /// @param principalThreshold Principal vs rewards classification threshold (gwei) - /// @return owr Address of the new OptimisticWithdrawalRecipientV2 instance - function createOWRecipient( + /// @return owr Address of the new ObolValidatorManager instance + function createObolValidatorManager( address owner, address principalRecipient, address rewardRecipient, address recoveryAddress, uint64 principalThreshold - ) external returns (OptimisticWithdrawalRecipientV2 owr) { + ) external returns (ObolValidatorManager owr) { if (principalRecipient == address(0) || rewardRecipient == address(0)) revert Invalid__Recipients(); if (principalThreshold == 0) revert Invalid__ZeroThreshold(); if (principalThreshold > 2048 * 1e9) revert Invalid__ThresholdTooLarge(); - owr = new OptimisticWithdrawalRecipientV2( + owr = new ObolValidatorManager( consolidationSystemContract, withdrawalSystemContract, depositSystemContract, @@ -124,7 +124,7 @@ contract OptimisticWithdrawalRecipientV2Factory { principalThreshold ); - emit CreateOWRecipient( + emit CreateObolValidatorManager( address(owr), owner, recoveryAddress, diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol b/src/test/ovm/ObolValidatorManager.t.sol similarity index 92% rename from src/test/owr/OptimisticWithdrawalRecipientV2.t.sol rename to src/test/ovm/ObolValidatorManager.t.sol index f7872e1..b19d4d5 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2.t.sol +++ b/src/test/ovm/ObolValidatorManager.t.sol @@ -3,16 +3,16 @@ pragma solidity ^0.8.19; import "forge-std/Test.sol"; import "forge-std/console.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; -import {OptimisticWithdrawalRecipientV2Factory} from "src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; +import {ObolValidatorManagerFactory} from "src/ovm/ObolValidatorManagerFactory.sol"; import {MockERC20} from "../utils/mocks/MockERC20.sol"; import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol"; -import {OWRV2Reentrancy} from "./OWRV2Reentrancy.sol"; +import {ObolValidatorManagerReentrancy} from "./ObolValidatorManagerReentrancy.sol"; import {SystemContractMock} from "./mocks/SystemContractMock.sol"; import {DepositContractMock} from "./mocks/DepositContractMock.sol"; import {IENSReverseRegistrar} from "../../interfaces/IENSReverseRegistrar.sol"; -contract OptimisticWithdrawalRecipientV2Test is Test { +contract ObolValidatorManagerTest is Test { using SafeTransferLib for address; event DistributeFunds(uint256 principalPayout, uint256 rewardPayout, uint256 pullFlowFlag); @@ -25,9 +25,9 @@ contract OptimisticWithdrawalRecipientV2Test is Test { uint64 public constant BALANCE_CLASSIFICATION_THRESHOLD_GWEI = 16 ether / 1 gwei; uint256 public constant INITIAL_DEPOSIT_AMOUNT = 32 ether; - OptimisticWithdrawalRecipientV2Factory public owrFactory; - OptimisticWithdrawalRecipientV2 owrETH; - OptimisticWithdrawalRecipientV2 owrETH_OR; + ObolValidatorManagerFactory public owrFactory; + ObolValidatorManager owrETH; + ObolValidatorManager owrETH_OR; SystemContractMock consolidationMock; SystemContractMock withdrawalMock; @@ -56,7 +56,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { withdrawalMock = new SystemContractMock(48 + 8); depositMock = new DepositContractMock(); - owrFactory = new OptimisticWithdrawalRecipientV2Factory( + owrFactory = new ObolValidatorManagerFactory( address(consolidationMock), address(withdrawalMock), address(depositMock), @@ -73,14 +73,14 @@ contract OptimisticWithdrawalRecipientV2Test is Test { rewardsRecipient = makeAddr("rewardsRecipient"); principalThreshold = BALANCE_CLASSIFICATION_THRESHOLD_GWEI; - owrETH = owrFactory.createOWRecipient( + owrETH = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, recoveryAddress, principalThreshold ); - owrETH_OR = owrFactory.createOWRecipient( + owrETH_OR = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -124,12 +124,12 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.stopPrank(); // Empty source array - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_Params.selector); bytes[] memory empty = new bytes[](0); owrETH.requestConsolidation{value: 1 ether}(empty, new bytes(48)); // Not enough fee (1 wei is the minimum fee) - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_NotEnoughFee.selector); bytes[] memory single = new bytes[](1); single[0] = new bytes(48); owrETH.requestConsolidation{value: 0}(single, new bytes(48)); @@ -137,18 +137,18 @@ contract OptimisticWithdrawalRecipientV2Test is Test { // Failed get_fee() request uint256 realFee = consolidationMock.fakeExponential(0); consolidationMock.setFailNextFeeRequest(true); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_SystemGetFee.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_SystemGetFee.selector); owrETH.requestConsolidation{value: realFee}(single, new bytes(48)); consolidationMock.setFailNextFeeRequest(false); // Failed add_request() request consolidationMock.setFailNextAddRequest(true); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidConsolidation_Failed.selector); + vm.expectRevert(ObolValidatorManager.InvalidConsolidation_Failed.selector); owrETH.requestConsolidation{value: realFee}(single, new bytes(48)); consolidationMock.setFailNextAddRequest(false); // Maximum number of source pubkeys is 63 - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_Params.selector); bytes[] memory batch64 = new bytes[](64); owrETH.requestConsolidation{value: realFee}(batch64, new bytes(48)); } @@ -233,27 +233,27 @@ contract OptimisticWithdrawalRecipientV2Test is Test { single[0] = new bytes(48); // Inequal array lengths - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_Params.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_Params.selector); bytes[] memory empty = new bytes[](0); owrETH.requestWithdrawal{value: 1 ether}(empty, amounts); // Not enough fee (1 wei is the minimum fee) uint256 validAmount = principalThreshold; amounts[0] = uint64(validAmount); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_NotEnoughFee.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_NotEnoughFee.selector); owrETH.requestWithdrawal{value: 0}(single, amounts); // Failed get_fee() request uint256 realFee = withdrawalMock.fakeExponential(0); amounts[0] = uint64(validAmount); withdrawalMock.setFailNextFeeRequest(true); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidRequest_SystemGetFee.selector); + vm.expectRevert(ObolValidatorManager.InvalidRequest_SystemGetFee.selector); owrETH.requestWithdrawal{value: realFee}(single, amounts); withdrawalMock.setFailNextFeeRequest(false); // Failed add_request() request withdrawalMock.setFailNextAddRequest(true); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidWithdrawal_Failed.selector); + vm.expectRevert(ObolValidatorManager.InvalidWithdrawal_Failed.selector); owrETH.requestWithdrawal{value: realFee}(single, amounts); withdrawalMock.setFailNextAddRequest(false); } @@ -370,10 +370,10 @@ contract OptimisticWithdrawalRecipientV2Test is Test { } function testCannot_recoverFundsToNonRecipient() public { - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidTokenRecovery_InvalidRecipient.selector); + vm.expectRevert(ObolValidatorManager.InvalidTokenRecovery_InvalidRecipient.selector); owrETH.recoverFunds(address(mERC20), address(1)); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidTokenRecovery_InvalidRecipient.selector); + vm.expectRevert(ObolValidatorManager.InvalidTokenRecovery_InvalidRecipient.selector); owrETH_OR.recoverFunds(address(mERC20), address(2)); } @@ -480,17 +480,17 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.deal(address(owrETH), 1); vm.deal(address(owrETH), type(uint136).max); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidDistribution_TooLarge.selector); + vm.expectRevert(ObolValidatorManager.InvalidDistribution_TooLarge.selector); owrETH.distributeFunds(); - vm.expectRevert(OptimisticWithdrawalRecipientV2.InvalidDistribution_TooLarge.selector); + vm.expectRevert(ObolValidatorManager.InvalidDistribution_TooLarge.selector); owrETH.distributeFundsPull(); } function testCannot_reenterOWR() public { - OWRV2Reentrancy wr = new OWRV2Reentrancy(); + ObolValidatorManagerReentrancy re = new ObolValidatorManagerReentrancy(); - owrETH = owrFactory.createOWRecipient(address(this), address(wr), rewardsRecipient, recoveryAddress, 1e9); + owrETH = owrFactory.createObolValidatorManager(address(this), address(re), rewardsRecipient, recoveryAddress, 1e9); owrETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0)); address(owrETH).safeTransferETH(33 ether); @@ -498,7 +498,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { owrETH.distributeFunds(); assertEq(address(owrETH).balance, 33 ether); - assertEq(address(wr).balance, 0 ether); + assertEq(address(re).balance, 0 ether); assertEq(address(0).balance, 0 ether); } @@ -641,7 +641,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); uint256 principalThresholdWei = uint256(_threshold) * 1e9; - owrETH = owrFactory.createOWRecipient( + owrETH = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -702,7 +702,7 @@ contract OptimisticWithdrawalRecipientV2Test is Test { vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); uint256 principalThresholdWei = uint256(_threshold) * 1e9; - owrETH = owrFactory.createOWRecipient( + owrETH = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, diff --git a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol b/src/test/ovm/ObolValidatorManagerFactory.t.sol similarity index 68% rename from src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol rename to src/test/ovm/ObolValidatorManagerFactory.t.sol index 4b03ff5..ca98b59 100644 --- a/src/test/owr/OptimisticWithdrawalRecipientV2Factory.t.sol +++ b/src/test/ovm/ObolValidatorManagerFactory.t.sol @@ -2,15 +2,15 @@ pragma solidity ^0.8.19; import "forge-std/Test.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; -import {OptimisticWithdrawalRecipientV2Factory} from "src/owr/OptimisticWithdrawalRecipientV2Factory.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; +import {ObolValidatorManagerFactory} from "src/ovm/ObolValidatorManagerFactory.sol"; import {MockERC20} from "../utils/mocks/MockERC20.sol"; import {SystemContractMock} from "./mocks/SystemContractMock.sol"; import {DepositContractMock} from "./mocks/DepositContractMock.sol"; import {IENSReverseRegistrar} from "../../interfaces/IENSReverseRegistrar.sol"; -contract OptimisticWithdrawalRecipientV2FactoryTest is Test { - event CreateOWRecipient( +contract ObolValidatorManagerFactoryTest is Test { + event CreateObolValidatorManager( address indexed owr, address indexed owner, address recoveryAddress, @@ -26,7 +26,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { SystemContractMock consolidationMock; SystemContractMock withdrawalMock; DepositContractMock depositMock; - OptimisticWithdrawalRecipientV2Factory owrFactory; + ObolValidatorManagerFactory owrFactory; address public recoveryAddress; address public principalRecipient; @@ -49,7 +49,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { withdrawalMock = new SystemContractMock(48 + 8); depositMock = new DepositContractMock(); - owrFactory = new OptimisticWithdrawalRecipientV2Factory( + owrFactory = new ObolValidatorManagerFactory( address(consolidationMock), address(withdrawalMock), address(depositMock), @@ -65,7 +65,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { } function testCan_createOWRecipient() public { - OptimisticWithdrawalRecipientV2 owr = owrFactory.createOWRecipient( + ObolValidatorManager owr = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -77,7 +77,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { assertEq(address(owr.withdrawalSystemContract()), address(withdrawalMock)); recoveryAddress = address(0); - owr = owrFactory.createOWRecipient( + owr = owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -91,7 +91,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { // don't check deploy address vm.expectEmit(false, true, true, true); - emit CreateOWRecipient( + emit CreateObolValidatorManager( address(0xdead), address(this), recoveryAddress, @@ -99,7 +99,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { rewardsRecipient, principalThreshold ); - owrFactory.createOWRecipient( + owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -111,7 +111,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { // don't check deploy address vm.expectEmit(false, true, true, true); - emit CreateOWRecipient( + emit CreateObolValidatorManager( address(0xdead), address(this), recoveryAddress, @@ -119,7 +119,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { rewardsRecipient, principalThreshold ); - owrFactory.createOWRecipient( + owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -129,21 +129,33 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { } function testCannot_createWithInvalidRecipients() public { - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(address(this), address(0), rewardsRecipient, recoveryAddress, principalThreshold); + vm.expectRevert(ObolValidatorManagerFactory.Invalid__Recipients.selector); + owrFactory.createObolValidatorManager( + address(this), + address(0), + rewardsRecipient, + recoveryAddress, + principalThreshold + ); - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(address(this), address(0), address(0), recoveryAddress, principalThreshold); + vm.expectRevert(ObolValidatorManagerFactory.Invalid__Recipients.selector); + owrFactory.createObolValidatorManager(address(this), address(0), address(0), recoveryAddress, principalThreshold); - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__Recipients.selector); - owrFactory.createOWRecipient(address(this), principalRecipient, address(0), recoveryAddress, principalThreshold); + vm.expectRevert(ObolValidatorManagerFactory.Invalid__Recipients.selector); + owrFactory.createObolValidatorManager( + address(this), + principalRecipient, + address(0), + recoveryAddress, + principalThreshold + ); } function testCannot_createWithInvalidThreshold() public { principalThreshold = 0; - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); - owrFactory.createOWRecipient( + vm.expectRevert(ObolValidatorManagerFactory.Invalid__ZeroThreshold.selector); + owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -151,8 +163,8 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { principalThreshold ); - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); - owrFactory.createOWRecipient( + vm.expectRevert(ObolValidatorManagerFactory.Invalid__ThresholdTooLarge.selector); + owrFactory.createObolValidatorManager( address(this), principalRecipient, rewardsRecipient, @@ -169,7 +181,7 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { vm.assume(_threshold > 0 && _threshold < 2048 * 1e9); vm.expectEmit(false, true, true, true); - emit CreateOWRecipient( + emit CreateObolValidatorManager( address(0xdead), address(this), recoveryAddress, @@ -177,7 +189,13 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { rewardsRecipient, _threshold ); - owrFactory.createOWRecipient(address(this), principalRecipient, rewardsRecipient, recoveryAddress, _threshold); + owrFactory.createObolValidatorManager( + address(this), + principalRecipient, + rewardsRecipient, + recoveryAddress, + _threshold + ); } function testFuzzCannot_CreateWithZeroThreshold(address _rewardsRecipient) public { @@ -185,8 +203,8 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { principalThreshold = 0; // eth - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ZeroThreshold.selector); - owrFactory.createOWRecipient( + vm.expectRevert(ObolValidatorManagerFactory.Invalid__ZeroThreshold.selector); + owrFactory.createObolValidatorManager( address(this), principalRecipient, _rewardsRecipient, @@ -199,7 +217,13 @@ contract OptimisticWithdrawalRecipientV2FactoryTest is Test { vm.assume(_threshold > 2048 * 1e9); vm.assume(_rewardsRecipient != address(0)); - vm.expectRevert(OptimisticWithdrawalRecipientV2Factory.Invalid__ThresholdTooLarge.selector); - owrFactory.createOWRecipient(address(this), principalRecipient, _rewardsRecipient, recoveryAddress, _threshold); + vm.expectRevert(ObolValidatorManagerFactory.Invalid__ThresholdTooLarge.selector); + owrFactory.createObolValidatorManager( + address(this), + principalRecipient, + _rewardsRecipient, + recoveryAddress, + _threshold + ); } } diff --git a/src/test/ovm/ObolValidatorManagerReentrancy.sol b/src/test/ovm/ObolValidatorManagerReentrancy.sol new file mode 100644 index 0000000..d07e613 --- /dev/null +++ b/src/test/ovm/ObolValidatorManagerReentrancy.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity ^0.8.19; + +import "forge-std/Test.sol"; +import "forge-std/console.sol"; +import {ObolValidatorManager} from "src/ovm/ObolValidatorManager.sol"; + +contract ObolValidatorManagerReentrancy is Test { + receive() external payable { + console.log("receive() with value", msg.value, "and balance", address(this).balance); + + if (address(this).balance <= 1 ether) ObolValidatorManager(payable(msg.sender)).distributeFunds(); + } +} diff --git a/src/test/owr/mocks/DepositContractMock.sol b/src/test/ovm/mocks/DepositContractMock.sol similarity index 100% rename from src/test/owr/mocks/DepositContractMock.sol rename to src/test/ovm/mocks/DepositContractMock.sol diff --git a/src/test/owr/mocks/SystemContractMock.sol b/src/test/ovm/mocks/SystemContractMock.sol similarity index 100% rename from src/test/owr/mocks/SystemContractMock.sol rename to src/test/ovm/mocks/SystemContractMock.sol diff --git a/src/test/owr/OWRV2Reentrancy.sol b/src/test/owr/OWRV2Reentrancy.sol deleted file mode 100644 index e181d1d..0000000 --- a/src/test/owr/OWRV2Reentrancy.sol +++ /dev/null @@ -1,14 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0-or-later -pragma solidity ^0.8.19; - -import "forge-std/Test.sol"; -import "forge-std/console.sol"; -import {OptimisticWithdrawalRecipientV2} from "src/owr/OptimisticWithdrawalRecipientV2.sol"; - -contract OWRV2Reentrancy is Test { - receive() external payable { - console.log("OWRV2Reentrancy::receive() with value", msg.value, "and balance", address(this).balance); - - if (address(this).balance <= 1 ether) OptimisticWithdrawalRecipientV2(payable(msg.sender)).distributeFunds(); - } -}