Skip to content

Commit 9649308

Browse files
pinebitpinebit
committed
OVM deposit() refactor
1 parent 62cec5d commit 9649308

4 files changed

Lines changed: 90 additions & 9 deletions

File tree

‎lib/forge-std‎

Submodule forge-std updated 39 files

‎src/interfaces/IObolValidatorManager.sol‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,18 @@ interface IObolValidatorManager {
9797
/// @notice Invalid distribution
9898
error InvalidDistribution_TooLarge();
9999

100+
/// @notice Deposit withdrawal credentials do not commit to this contract's address
101+
error InvalidDeposit_WithdrawalCredentials();
102+
100103
/// -----------------------------------------------------------------------
101104
/// ObolValidatorManager functions
102105
/// -----------------------------------------------------------------------
103106

104107
/// @notice Submit a Phase 0 DepositData object.
105108
/// @param pubkey A BLS12-381 public key.
106109
/// @param withdrawal_credentials Commitment to a public key for withdrawals.
110+
/// Must commit to this contract's address: a 0x01 or 0x02 prefix, 11 zero bytes,
111+
/// and this contract's address. Reverts with InvalidDeposit_WithdrawalCredentials otherwise.
107112
/// @param signature A BLS12-381 signature.
108113
/// @param deposit_data_root The SHA-256 hash of the SSZ-encoded DepositData object.
109114
/// Used as a protection against malformed input.

‎src/ovm/ObolValidatorManager.sol‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ contract ObolValidatorManager is IObolValidatorManager, OwnableRoles, Reentrancy
3939

4040
uint256 internal constant PUBLIC_KEY_LENGTH = 48;
4141

42+
uint256 internal constant WITHDRAWAL_CREDENTIALS_LENGTH = 32;
43+
bytes32 internal constant ETH1_WITHDRAWAL_PREFIX = bytes32(uint256(0x01) << 248);
44+
bytes32 internal constant COMPOUNDING_WITHDRAWAL_PREFIX = bytes32(uint256(0x02) << 248);
45+
4246
/// -----------------------------------------------------------------------
4347
/// storage - immutable
4448
/// -----------------------------------------------------------------------
@@ -117,6 +121,8 @@ contract ObolValidatorManager is IObolValidatorManager, OwnableRoles, Reentrancy
117121
bytes calldata signature,
118122
bytes32 deposit_data_root
119123
) external payable onlyOwnerOrRoles(DEPOSIT_ROLE) {
124+
_validateWithdrawalCredentials(withdrawal_credentials);
125+
120126
uint256 oldAmountOfPrincipalStake = amountOfPrincipalStake;
121127
amountOfPrincipalStake += msg.value;
122128
IDepositContract(depositSystemContract).deposit{value: msg.value}(
@@ -429,6 +435,21 @@ contract ObolValidatorManager is IObolValidatorManager, OwnableRoles, Reentrancy
429435
if (pubkey.length != PUBLIC_KEY_LENGTH) revert InvalidRequest_Params();
430436
}
431437

438+
/// Internal function to validate that withdrawal credentials commit to this contract's address,
439+
/// so that validator withdrawals cannot bypass the principal/reward accounting.
440+
/// Accepted formats: 0x01 (ETH1) or 0x02 (compounding) prefix, 11 zero bytes, this contract's address.
441+
/// @param withdrawal_credentials The withdrawal credentials to validate
442+
function _validateWithdrawalCredentials(bytes calldata withdrawal_credentials) internal view {
443+
if (withdrawal_credentials.length != WITHDRAWAL_CREDENTIALS_LENGTH) revert InvalidDeposit_WithdrawalCredentials();
444+
445+
bytes32 credentials = bytes32(withdrawal_credentials);
446+
bytes32 addressPart = bytes32(uint256(uint160(address(this))));
447+
if (
448+
credentials != (ETH1_WITHDRAWAL_PREFIX | addressPart)
449+
&& credentials != (COMPOUNDING_WITHDRAWAL_PREFIX | addressPart)
450+
) revert InvalidDeposit_WithdrawalCredentials();
451+
}
452+
432453
/// Internal function to refund the excess fee for pectra related operations.
433454
/// @param _totalValueReceived The total value received.
434455
/// @param _totalFeePaid The total fee paid.

‎src/test/ovm/ObolValidatorManager.t.sol‎

Lines changed: 63 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,18 @@ contract ObolValidatorManagerTest is Test {
9393
principalThreshold
9494
);
9595

96-
ovmETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
97-
ovmETH_OR.deposit{value: INITIAL_DEPOSIT_AMOUNT}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
96+
ovmETH.deposit{value: INITIAL_DEPOSIT_AMOUNT}(
97+
new bytes(0),
98+
_withdrawalCredentials(ovmETH, 0x02),
99+
new bytes(0),
100+
bytes32(0)
101+
);
102+
ovmETH_OR.deposit{value: INITIAL_DEPOSIT_AMOUNT}(
103+
new bytes(0),
104+
_withdrawalCredentials(ovmETH_OR, 0x02),
105+
new bytes(0),
106+
bytes32(0)
107+
);
98108
}
99109

100110
function testDefaultParameters() public view {
@@ -113,25 +123,70 @@ contract ObolValidatorManagerTest is Test {
113123
uint256 depositAmount = 1 ether;
114124
vm.expectEmit(true, true, true, true);
115125
emit PrincipalStakeAmountUpdated(amountOfPrincipalStake + depositAmount, amountOfPrincipalStake);
116-
ovmETH.deposit{value: depositAmount}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
126+
ovmETH.deposit{value: depositAmount}(new bytes(0), _withdrawalCredentials(ovmETH, 0x02), new bytes(0), bytes32(0));
117127
assertEq(address(depositMock).balance, depositMockBalance + depositAmount);
118128
assertEq(ovmETH.amountOfPrincipalStake(), amountOfPrincipalStake + depositAmount);
119129
}
120130

131+
function testDepositWithEth1WithdrawalCredentials() public {
132+
uint256 amountOfPrincipalStake = ovmETH.amountOfPrincipalStake();
133+
uint256 depositAmount = 1 ether;
134+
ovmETH.deposit{value: depositAmount}(new bytes(0), _withdrawalCredentials(ovmETH, 0x01), new bytes(0), bytes32(0));
135+
assertEq(ovmETH.amountOfPrincipalStake(), amountOfPrincipalStake + depositAmount);
136+
}
137+
121138
function testCannotDeposit() public {
122139
// unauthorized
123140
address _user = vm.addr(0x5);
124141
vm.deal(_user, 1 ether);
125142
ovmETH.grantRoles(_user, ovmETH.WITHDRAWAL_ROLE()); // unrelated role
126143
vm.startPrank(_user);
127144
vm.expectRevert(bytes4(0x82b42900));
128-
ovmETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
145+
ovmETH.deposit{value: 1 ether}(new bytes(0), _withdrawalCredentials(ovmETH, 0x02), new bytes(0), bytes32(0));
129146
vm.stopPrank();
130147

131148
// unauthorized for owner after renounce
132149
ovmETH.renounceOwnership();
133150
vm.expectRevert(bytes4(0x82b42900));
151+
ovmETH.deposit{value: 1 ether}(new bytes(0), _withdrawalCredentials(ovmETH, 0x02), new bytes(0), bytes32(0));
152+
}
153+
154+
function testCannotDepositWithInvalidWithdrawalCredentials() public {
155+
// empty credentials
156+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
134157
ovmETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
158+
159+
// wrong length: 31 bytes
160+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
161+
ovmETH.deposit{value: 1 ether}(new bytes(0), new bytes(31), new bytes(0), bytes32(0));
162+
163+
// wrong length: 33 bytes (valid credentials + 1 extra byte)
164+
bytes memory tooLong = abi.encodePacked(_withdrawalCredentials(ovmETH, 0x02), bytes1(0));
165+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
166+
ovmETH.deposit{value: 1 ether}(new bytes(0), tooLong, new bytes(0), bytes32(0));
167+
168+
// wrong prefix: 0x00 (BLS credentials)
169+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
170+
ovmETH.deposit{value: 1 ether}(new bytes(0), _withdrawalCredentials(ovmETH, 0x00), new bytes(0), bytes32(0));
171+
172+
// wrong prefix: 0x03
173+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
174+
ovmETH.deposit{value: 1 ether}(new bytes(0), _withdrawalCredentials(ovmETH, 0x03), new bytes(0), bytes32(0));
175+
176+
// wrong address: credentials commit to an address other than the OVM
177+
bytes memory wrongAddress = abi.encodePacked(bytes1(0x02), bytes11(0), makeAddr("attacker"));
178+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
179+
ovmETH.deposit{value: 1 ether}(new bytes(0), wrongAddress, new bytes(0), bytes32(0));
180+
181+
// non-zero padding bytes
182+
bytes memory dirtyPadding = abi.encodePacked(bytes1(0x02), bytes11(uint88(1)), address(ovmETH));
183+
vm.expectRevert(IObolValidatorManager.InvalidDeposit_WithdrawalCredentials.selector);
184+
ovmETH.deposit{value: 1 ether}(new bytes(0), dirtyPadding, new bytes(0), bytes32(0));
185+
}
186+
187+
/// Builds withdrawal credentials committing to the given OVM: prefix + 11 zero bytes + OVM address
188+
function _withdrawalCredentials(ObolValidatorManager ovm, bytes1 prefix) internal pure returns (bytes memory) {
189+
return abi.encodePacked(prefix, bytes11(0), address(ovm));
135190
}
136191

137192
function testSetBeneficiaryRecipient() public {
@@ -725,7 +780,7 @@ contract ObolValidatorManagerTest is Test {
725780
function testCan_distributeToBothRecipients() public {
726781
// First deposit of 32eth is done in setUp()
727782
uint256 secondDeposit = 64 ether;
728-
ovmETH.deposit{value: secondDeposit}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
783+
ovmETH.deposit{value: secondDeposit}(new bytes(0), _withdrawalCredentials(ovmETH, 0x02), new bytes(0), bytes32(0));
729784
uint256 rewardPayout = 4 ether;
730785
address(ovmETH).safeTransferETH(INITIAL_DEPOSIT_AMOUNT + secondDeposit + rewardPayout);
731786

@@ -783,7 +838,7 @@ contract ObolValidatorManagerTest is Test {
783838
ObolValidatorManagerReentrancy re = new ObolValidatorManagerReentrancy();
784839

785840
ovmETH = ovmFactory.createObolValidatorManager(address(this), address(re), rewardsRecipient, 1e9);
786-
ovmETH.deposit{value: 1 ether}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
841+
ovmETH.deposit{value: 1 ether}(new bytes(0), _withdrawalCredentials(ovmETH, 0x02), new bytes(0), bytes32(0));
787842
address(ovmETH).safeTransferETH(33 ether);
788843

789844
vm.expectRevert(SafeTransferLib.ETHTransferFailed.selector);
@@ -944,7 +999,7 @@ contract ObolValidatorManagerTest is Test {
944999
);
9451000

9461001
uint256 _totalETHAmount = uint256(_numDeposits) * uint256(_ethAmount);
947-
ovm.deposit{value: _totalETHAmount}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
1002+
ovm.deposit{value: _totalETHAmount}(new bytes(0), _withdrawalCredentials(ovm, 0x02), new bytes(0), bytes32(0));
9481003

9491004
/// test eth
9501005
for (uint256 i = 0; i < _numDeposits; i++) {
@@ -993,7 +1048,7 @@ contract ObolValidatorManagerTest is Test {
9931048
);
9941049

9951050
uint256 _totalETHAmount = uint256(_numDeposits) * uint256(_ethAmount);
996-
ovm.deposit{value: _totalETHAmount}(new bytes(0), new bytes(0), new bytes(0), bytes32(0));
1051+
ovm.deposit{value: _totalETHAmount}(new bytes(0), _withdrawalCredentials(ovm, 0x02), new bytes(0), bytes32(0));
9971052

9981053
for (uint256 i = 0; i < _numDeposits; i++) {
9991054
address(ovm).safeTransferETH(_ethAmount);

0 commit comments

Comments
 (0)