From 668032909e13734db340303aec3bef052da8b1e9 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Wed, 8 Nov 2023 16:47:17 +0400 Subject: [PATCH 01/10] Add permit check for stETH and WstETH deposits --- src/CSAccounting.sol | 45 +++++++++++---------- test/CSAccounting.t.sol | 88 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 20 deletions(-) diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol index 20c013a7..a9a59484 100644 --- a/src/CSAccounting.sol +++ b/src/CSAccounting.sol @@ -470,16 +470,18 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { PermitInput calldata permit ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { from = (from == address(0)) ? msg.sender : from; - // solhint-disable-next-line func-named-parameters - _lido().permit( - from, - address(this), - permit.value, - permit.deadline, - permit.v, - permit.r, - permit.s - ); + if (_lido().allowance(from, address(this)) < permit.value) { + // solhint-disable-next-line func-named-parameters + _lido().permit( + from, + address(this), + permit.value, + permit.deadline, + permit.v, + permit.r, + permit.s + ); + } return _depositStETH(from, nodeOperatorId, stETHAmount); } @@ -520,16 +522,19 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { uint256 wstETHAmount, PermitInput calldata permit ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { - // solhint-disable-next-line func-named-parameters - WSTETH.permit( - from, - address(this), - permit.value, - permit.deadline, - permit.v, - permit.r, - permit.s - ); + from = (from == address(0)) ? msg.sender : from; + if (WSTETH.allowance(from, address(this)) < permit.value) { + // solhint-disable-next-line func-named-parameters + WSTETH.permit( + from, + address(this), + permit.value, + permit.deadline, + permit.v, + permit.r, + permit.s + ); + } return _depositWstETH(from, nodeOperatorId, wstETHAmount); } diff --git a/test/CSAccounting.t.sol b/test/CSAccounting.t.sol index 07f3bcde..0003d643 100644 --- a/test/CSAccounting.t.sol +++ b/test/CSAccounting.t.sol @@ -413,6 +413,49 @@ contract CSAccountingTest is ); } + function test_depositStETHWithPermit_alreadyPermitted() public { + _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); + vm.deal(user, 32 ether); + vm.prank(user); + stETH.submit{ value: 32 ether }({ _referal: address(0) }); + + vm.expectEmit(true, true, true, true, address(accounting)); + emit StETHBondDeposited(0, user, 32 ether); + + vm.mockCall( + address(stETH), + abi.encodeWithSelector( + stETH.permit.selector, + user, + address(accounting) + ), + abi.encode(32 ether) + ); + + vm.recordLogs(); + + vm.prank(stranger); + accounting.depositStETHWithPermit( + user, + 0, + 32 ether, + CSAccounting.PermitInput({ + value: 32 ether, + deadline: type(uint256).max, + // mock permit signature + v: 0, + r: 0, + s: 0 + }) + ); + + assertEq( + vm.getRecordedLogs().length, + 1, + "should emit only one event about deposit" + ); + } + function test_depositWstETHWithPermit() public { _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); vm.deal(user, 32 ether); @@ -461,6 +504,51 @@ contract CSAccountingTest is ); } + function test_depositWstETHWithPermit_alreadyPermitted() public { + _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); + vm.deal(user, 32 ether); + vm.startPrank(user); + stETH.submit{ value: 32 ether }({ _referal: address(0) }); + uint256 wstETHAmount = wstETH.wrap(32 ether); + vm.stopPrank(); + + vm.expectEmit(true, true, true, true, address(accounting)); + emit WstETHBondDeposited(0, user, wstETHAmount); + + vm.mockCall( + address(wstETH), + abi.encodeWithSelector( + wstETH.permit.selector, + user, + address(accounting) + ), + abi.encode(32 ether) + ); + + vm.recordLogs(); + + vm.prank(stranger); + accounting.depositWstETHWithPermit( + user, + 0, + wstETHAmount, + CSAccounting.PermitInput({ + value: 32 ether, + deadline: type(uint256).max, + // mock permit signature + v: 0, + r: 0, + s: 0 + }) + ); + + assertEq( + vm.getRecordedLogs().length, + 1, + "should emit only one event about deposit" + ); + } + function test_deposit_RevertIfNotExistedOperator() public { vm.expectRevert("node operator does not exist"); accounting.depositStETH(user, 0, 32 ether); From 2d4aeee8c099fec8abdf2f095fcc55d26619e370 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Wed, 8 Nov 2023 16:49:54 +0400 Subject: [PATCH 02/10] Add allowance function to IStETH and IWstETH interfaces --- src/interfaces/IStETH.sol | 5 +++++ src/interfaces/IWstETH.sol | 5 +++++ test/helpers/Permit.sol | 7 +++++++ 3 files changed, 17 insertions(+) diff --git a/src/interfaces/IStETH.sol b/src/interfaces/IStETH.sol index 1c098937..1d081802 100644 --- a/src/interfaces/IStETH.sol +++ b/src/interfaces/IStETH.sol @@ -78,4 +78,9 @@ interface IStETH { bytes32 r, bytes32 s ) external; + + function allowance( + address _owner, + address _spender + ) external view returns (uint256); } diff --git a/src/interfaces/IWstETH.sol b/src/interfaces/IWstETH.sol index 65ef5403..15e1a0ca 100644 --- a/src/interfaces/IWstETH.sol +++ b/src/interfaces/IWstETH.sol @@ -35,4 +35,9 @@ interface IWstETH { bytes32 r, bytes32 s ) external; + + function allowance( + address _owner, + address _spender + ) external view returns (uint256); } diff --git a/test/helpers/Permit.sol b/test/helpers/Permit.sol index 0c3dbd3a..1655de80 100644 --- a/test/helpers/Permit.sol +++ b/test/helpers/Permit.sol @@ -24,6 +24,13 @@ contract PermitTokenBase { ) external { emit Approval(owner, spender, value); } + + function allowance( + address owner, + address spender + ) external view returns (uint256) { + return 0; + } } // https://eips.ethereum.org/EIPS/eip-2612 From de43ee6b4dcb684c3f730dfbbbc5f7f99a7ef7a6 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Wed, 8 Nov 2023 16:56:31 +0400 Subject: [PATCH 03/10] Fix allowance function calls in CSAccountingTest. --- test/CSAccounting.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/CSAccounting.t.sol b/test/CSAccounting.t.sol index 0003d643..fbd4735f 100644 --- a/test/CSAccounting.t.sol +++ b/test/CSAccounting.t.sol @@ -425,7 +425,7 @@ contract CSAccountingTest is vm.mockCall( address(stETH), abi.encodeWithSelector( - stETH.permit.selector, + stETH.allowance.selector, user, address(accounting) ), @@ -518,7 +518,7 @@ contract CSAccountingTest is vm.mockCall( address(wstETH), abi.encodeWithSelector( - wstETH.permit.selector, + wstETH.allowance.selector, user, address(accounting) ), From 2cba2e243d925ff4f207c98cb9f257876b646016 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Wed, 8 Nov 2023 16:58:51 +0400 Subject: [PATCH 04/10] Refactor allowance function parameter names. --- test/helpers/Permit.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/helpers/Permit.sol b/test/helpers/Permit.sol index 1655de80..acf22006 100644 --- a/test/helpers/Permit.sol +++ b/test/helpers/Permit.sol @@ -26,8 +26,8 @@ contract PermitTokenBase { } function allowance( - address owner, - address spender + address _owner, + address _spender ) external view returns (uint256) { return 0; } From 97e82d788b7b6a09d4e14846b7880f74ad223fa1 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Thu, 9 Nov 2023 11:31:08 +0400 Subject: [PATCH 05/10] fix: deposit mechanics --- src/CSAccounting.sol | 83 +++++++++++++++++++++----------------- src/CSModule.sol | 3 +- test/CSAccounting.t.sol | 71 +++++++++++++++++++++++++++++--- test/CSMAddValidator.t.sol | 5 +-- 4 files changed, 116 insertions(+), 46 deletions(-) diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol index a9a59484..992d60db 100644 --- a/src/CSAccounting.sol +++ b/src/CSAccounting.sol @@ -112,6 +112,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { error NotOwnerToClaim(address msgSender, address owner); error InvalidBlockedBondRetentionPeriod(); error InvalidStolenAmount(); + error InvalidSender(); /// @param commonBondSize common bond size in ETH for all node operators. /// @param admin admin role member address @@ -428,18 +429,21 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { return wstETHAmount / getRequiredBondWstETHForKeys(1); } - /// @notice Deposits ETH to the bond for the given node operator. - /// @param nodeOperatorId id of the node operator to deposit bond for. + /// @notice Stake user's ETH to Lido and make deposit in stETH to the bond + /// @return stETH shares amount + /// @dev if `from` is not the same as `msg.sender`, then `msg.sender` should be CSM function depositETH( address from, uint256 nodeOperatorId - ) - external - payable - onlyExistingNodeOperator(nodeOperatorId) - returns (uint256) - { - from = (from == address(0)) ? msg.sender : from; + ) external payable returns (uint256) { + from = _validateDepositSender(from); + return _depositETH(from, nodeOperatorId); + } + + function _depositETH( + address from, + uint256 nodeOperatorId + ) internal onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { uint256 shares = _lido().submit{ value: msg.value }(address(0)); _bondShares[nodeOperatorId] += shares; totalBondShares += shares; @@ -447,29 +451,28 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { return shares; } - /// @notice Deposits stETH to the bond for the given node operator. - /// @param nodeOperatorId id of the node operator to deposit bond for. - /// @param stETHAmount amount of stETH to deposit. + /// @notice Deposit user's stETH to the bond for the given Node Operator + /// @return stETH shares amount + /// @dev if `from` is not the same as `msg.sender`, then `msg.sender` should be CSM function depositStETH( address from, uint256 nodeOperatorId, uint256 stETHAmount - ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { - from = (from == address(0)) ? msg.sender : from; + ) external returns (uint256) { + from = _validateDepositSender(from); return _depositStETH(from, nodeOperatorId, stETHAmount); } - /// @notice Deposits stETH to the bond for the given node operator. - /// @param nodeOperatorId id of the node operator to deposit bond for. - /// @param stETHAmount amount of stETH to deposit. - /// @param permit permit to spend stETH. + /// @notice Deposit user's stETH to the bond for the given Node Operator using the proper permit for the contract + /// @return stETH shares amount + /// @dev if `from` is not the same as `msg.sender`, then `msg.sender` should be CSM function depositStETHWithPermit( address from, uint256 nodeOperatorId, uint256 stETHAmount, PermitInput calldata permit - ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { - from = (from == address(0)) ? msg.sender : from; + ) external returns (uint256) { + from = _validateDepositSender(from); if (_lido().allowance(from, address(this)) < permit.value) { // solhint-disable-next-line func-named-parameters _lido().permit( @@ -489,7 +492,8 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { address from, uint256 nodeOperatorId, uint256 stETHAmount - ) internal returns (uint256) { + ) internal onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { + // todo: should we check that `from` is manager\reward address ??? uint256 shares = _sharesByEth(stETHAmount); _lido().transferSharesFrom(from, address(this), shares); _bondShares[nodeOperatorId] += shares; @@ -498,31 +502,28 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { return shares; } - /// @notice Deposits wstETH to the bond for the given node operator. - /// @param from address to deposit wstETH from. - /// @param nodeOperatorId id of the node operator to deposit bond for. - /// @param wstETHAmount amount of wstETH to deposit. + /// @notice Unwrap user's wstETH and make deposit in stETH to the bond for the given Node Operator + /// @return stETH shares amount + /// @dev if `from` is not the same as `msg.sender`, then `msg.sender` should be CSM function depositWstETH( address from, uint256 nodeOperatorId, uint256 wstETHAmount - ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { - from = (from == address(0)) ? msg.sender : from; + ) external returns (uint256) { + from = _validateDepositSender(from); return _depositWstETH(from, nodeOperatorId, wstETHAmount); } - /// @notice Deposits wstETH to the bond for the given node operator. - /// @param from address to deposit wstETH from. - /// @param nodeOperatorId id of the node operator to deposit bond for. - /// @param wstETHAmount amount of wstETH to deposit. - /// @param permit permit to spend wstETH. + /// @notice Unwrap user's wstETH and make deposit in stETH to the bond for the given Node Operator using the proper permit for the contract + /// @return stETH shares amount + /// @dev if `from` is not the same as `msg.sender`, then `msg.sender` should be CSM function depositWstETHWithPermit( address from, uint256 nodeOperatorId, uint256 wstETHAmount, PermitInput calldata permit - ) external onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { - from = (from == address(0)) ? msg.sender : from; + ) external returns (uint256) { + from = _validateDepositSender(from); if (WSTETH.allowance(from, address(this)) < permit.value) { // solhint-disable-next-line func-named-parameters WSTETH.permit( @@ -542,7 +543,8 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { address from, uint256 nodeOperatorId, uint256 wstETHAmount - ) internal returns (uint256) { + ) internal onlyExistingNodeOperator(nodeOperatorId) returns (uint256) { + // todo: should we check that `from` is manager\reward address ??? WSTETH.transferFrom(from, address(this), wstETHAmount); uint256 stETHAmount = WSTETH.unwrap(wstETHAmount); uint256 shares = _sharesByEth(stETHAmount); @@ -552,6 +554,16 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { return shares; } + /// @dev only CSM can pass `from` != `msg.sender` + function _validateDepositSender( + address from + ) internal view returns (address) { + if (from == address(0)) from = msg.sender; + if (from != msg.sender && msg.sender != address(CSM)) + revert InvalidSender(); + return from; + } + /// @notice Claims full reward (fee + bond) for the given node operator with desirable value /// @param rewardsProof merkle proof of the rewards. /// @param nodeOperatorId id of the node operator to claim rewards for. @@ -747,7 +759,6 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { function settleBlockedBondETH( uint256[] memory nodeOperatorIds ) external onlyRole(EL_REWARDS_STEALING_PENALTY_SETTLE_ROLE) { - uint256 nosCount = CSM.getNodeOperatorsCount(); for (uint256 i; i < nodeOperatorIds.length; ++i) { uint256 nodeOperatorId = nodeOperatorIds[i]; BlockedBond storage blockedBond = _blockedBondEther[nodeOperatorId]; diff --git a/src/CSModule.sol b/src/CSModule.sol index 9f571657..82a8d49d 100644 --- a/src/CSModule.sol +++ b/src/CSModule.sol @@ -225,7 +225,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addNodeOperatorStETHWithPermit( - address from, uint256 keysCount, bytes calldata publicKeys, bytes calldata signatures, @@ -242,7 +241,7 @@ contract CSModule is IStakingModule, CSModuleBase { _activeNodeOperatorsCount++; accounting.depositStETHWithPermit( - from, + msg.sender, id, accounting.getRequiredBondStETHForKeys(keysCount), permit diff --git a/test/CSAccounting.t.sol b/test/CSAccounting.t.sol index fbd4735f..d3db8dfc 100644 --- a/test/CSAccounting.t.sol +++ b/test/CSAccounting.t.sol @@ -381,7 +381,7 @@ contract CSAccountingTest is vm.expectEmit(true, true, true, true, address(accounting)); emit StETHBondDeposited(0, user, 32 ether); - vm.prank(stranger); + vm.prank(user); accounting.depositStETHWithPermit( user, 0, @@ -434,7 +434,7 @@ contract CSAccountingTest is vm.recordLogs(); - vm.prank(stranger); + vm.prank(user); accounting.depositStETHWithPermit( user, 0, @@ -472,7 +472,7 @@ contract CSAccountingTest is vm.expectEmit(true, true, true, true, address(accounting)); emit WstETHBondDeposited(0, user, wstETHAmount); - vm.prank(stranger); + vm.prank(user); accounting.depositWstETHWithPermit( user, 0, @@ -527,7 +527,7 @@ contract CSAccountingTest is vm.recordLogs(); - vm.prank(stranger); + vm.prank(user); accounting.depositWstETHWithPermit( user, 0, @@ -549,11 +549,72 @@ contract CSAccountingTest is ); } - function test_deposit_RevertIfNotExistedOperator() public { + function test_depositETH_RevertIfNotExistedOperator() public { vm.expectRevert("node operator does not exist"); + vm.prank(user); + accounting.depositETH{ value: 0 }(user, 0); + } + + function test_depositStETH_RevertIfNotExistedOperator() public { + vm.expectRevert("node operator does not exist"); + vm.prank(user); + accounting.depositStETH(user, 0, 32 ether); + } + + function test_depositETH_RevertIfInvalidSender() public { + vm.expectRevert(0xddb5de5e); + vm.prank(stranger); + accounting.depositETH{ value: 0 }(user, 0); + } + + function test_depositStETH_RevertIfInvalidSender() public { + vm.expectRevert(0xddb5de5e); + vm.prank(stranger); accounting.depositStETH(user, 0, 32 ether); } + function test_depositStETHWithPermit_RevertIfInvalidSender() public { + vm.expectRevert(0xddb5de5e); + vm.prank(stranger); + accounting.depositStETHWithPermit( + user, + 0, + 32 ether, + CSAccounting.PermitInput({ + value: 32 ether, + deadline: type(uint256).max, + // mock permit signature + v: 0, + r: 0, + s: 0 + }) + ); + } + + function test_depositWstETH_RevertIfInvalidSender() public { + vm.expectRevert(0xddb5de5e); + vm.prank(stranger); + accounting.depositWstETH(user, 0, 32 ether); + } + + function test_depositWstETHWithPermit_RevertIfInvalidSender() public { + vm.expectRevert(0xddb5de5e); + vm.prank(stranger); + accounting.depositWstETHWithPermit( + user, + 0, + 32 ether, + CSAccounting.PermitInput({ + value: 32 ether, + deadline: type(uint256).max, + // mock permit signature + v: 0, + r: 0, + s: 0 + }) + ); + } + function test_getTotalRewardsETH() public { _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); vm.deal(address(feeDistributor), 0.1 ether); diff --git a/test/CSMAddValidator.t.sol b/test/CSMAddValidator.t.sol index b8f504e0..9b19a484 100644 --- a/test/CSMAddValidator.t.sol +++ b/test/CSMAddValidator.t.sol @@ -212,12 +212,11 @@ contract CSMAddNodeOperator is CSMCommon, PermitTokenBase { vm.expectEmit(true, true, false, true, address(csm)); emit TotalSigningKeysCountChanged(0, 1); vm.expectEmit(true, true, false, true, address(csm)); - emit NodeOperatorAdded(0, stranger); + emit NodeOperatorAdded(0, nodeOperator); } - vm.prank(stranger); + vm.prank(nodeOperator); csm.addNodeOperatorStETHWithPermit( - nodeOperator, 1, keys, signatures, From b5a791a86b7cc73ee15ddd41007050d866fabf89 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Thu, 9 Nov 2023 11:34:06 +0400 Subject: [PATCH 06/10] chore: comment --- src/CSAccounting.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol index 992d60db..a9abe070 100644 --- a/src/CSAccounting.sol +++ b/src/CSAccounting.sol @@ -473,6 +473,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { PermitInput calldata permit ) external returns (uint256) { from = _validateDepositSender(from); + // preventing revert for already used permit if (_lido().allowance(from, address(this)) < permit.value) { // solhint-disable-next-line func-named-parameters _lido().permit( @@ -524,6 +525,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { PermitInput calldata permit ) external returns (uint256) { from = _validateDepositSender(from); + // preventing revert for already used permit if (WSTETH.allowance(from, address(this)) < permit.value) { // solhint-disable-next-line func-named-parameters WSTETH.permit( From dbd56cedb80495a0ba66e27d21150593c1898cd3 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Thu, 9 Nov 2023 11:36:07 +0400 Subject: [PATCH 07/10] fix: integration tests --- test/integration/DepositInTokens.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/DepositInTokens.t.sol b/test/integration/DepositInTokens.t.sol index 3a370a3a..2b4854f3 100644 --- a/test/integration/DepositInTokens.t.sol +++ b/test/integration/DepositInTokens.t.sol @@ -134,7 +134,7 @@ contract DepositIntegrationTest is }); vm.stopPrank(); - vm.prank(stranger); + vm.prank(user); accounting.depositStETHWithPermit( user, 0, @@ -173,7 +173,7 @@ contract DepositIntegrationTest is uint256 wstETHAmount = wstETH.wrap(32 ether); vm.stopPrank(); - vm.prank(stranger); + vm.prank(user); uint256 shares = accounting.depositWstETHWithPermit( user, 0, From b5c75b473775783ecc96034129324d642f838610 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Thu, 9 Nov 2023 11:52:54 +0400 Subject: [PATCH 08/10] fix: remove `from` --- src/CSModule.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/CSModule.sol b/src/CSModule.sol index 82a8d49d..c3815e93 100644 --- a/src/CSModule.sol +++ b/src/CSModule.sol @@ -280,7 +280,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addNodeOperatorWstETHWithPermit( - address from, uint256 keysCount, bytes calldata publicKeys, bytes calldata signatures, @@ -297,7 +296,7 @@ contract CSModule is IStakingModule, CSModuleBase { _activeNodeOperatorsCount++; accounting.depositWstETHWithPermit( - from, + msg.sender, id, accounting.getRequiredBondWstETHForKeys(keysCount), permit @@ -345,7 +344,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addValidatorKeysStETHWithPermit( - address from, uint256 nodeOperatorId, uint256 keysCount, bytes calldata publicKeys, @@ -355,7 +353,7 @@ contract CSModule is IStakingModule, CSModuleBase { // TODO sanity checks accounting.depositStETHWithPermit( - from, + msg.sender, nodeOperatorId, accounting.getRequiredBondStETH(nodeOperatorId, keysCount), permit @@ -382,7 +380,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addValidatorKeysWstETHWithPermit( - address from, uint256 nodeOperatorId, uint256 keysCount, bytes calldata publicKeys, @@ -392,7 +389,7 @@ contract CSModule is IStakingModule, CSModuleBase { // TODO sanity checks accounting.depositWstETHWithPermit( - from, + msg.sender, nodeOperatorId, accounting.getRequiredBondWstETH(nodeOperatorId, keysCount), permit From ff720613537416a6683577c5c03d92e9dae8f452 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Thu, 9 Nov 2023 13:08:47 +0400 Subject: [PATCH 09/10] fix: tests --- test/CSMAddValidator.t.sol | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/test/CSMAddValidator.t.sol b/test/CSMAddValidator.t.sol index 9b19a484..32519463 100644 --- a/test/CSMAddValidator.t.sol +++ b/test/CSMAddValidator.t.sol @@ -107,12 +107,11 @@ contract CSMAddNodeOperator is CSMCommon, PermitTokenBase { vm.expectEmit(true, true, false, true, address(csm)); emit TotalSigningKeysCountChanged(0, 1); vm.expectEmit(true, true, false, true, address(csm)); - emit NodeOperatorAdded(0, stranger); + emit NodeOperatorAdded(0, nodeOperator); } - vm.prank(stranger); + vm.prank(nodeOperator); csm.addNodeOperatorWstETHWithPermit( - nodeOperator, 1, keys, signatures, @@ -164,9 +163,8 @@ contract CSMAddNodeOperator is CSMCommon, PermitTokenBase { vm.expectEmit(true, true, false, true, address(csm)); emit TotalSigningKeysCountChanged(0, 2); } - vm.prank(stranger); + vm.prank(nodeOperator); csm.addValidatorKeysWstETHWithPermit( - nodeOperator, noId, 1, keys, @@ -265,9 +263,8 @@ contract CSMAddNodeOperator is CSMCommon, PermitTokenBase { vm.expectEmit(true, true, false, true, address(csm)); emit TotalSigningKeysCountChanged(0, 2); } - vm.prank(stranger); + vm.prank(nodeOperator); csm.addValidatorKeysStETHWithPermit( - nodeOperator, noId, 1, keys, From 2fa511167e30fd0ced874e1ed1ed7e9ff4520e54 Mon Sep 17 00:00:00 2001 From: vgorkavenko Date: Fri, 10 Nov 2023 11:49:41 +0400 Subject: [PATCH 10/10] fix: review --- src/CSAccounting.sol | 14 +++++++++----- test/CSAccounting.blockedBond.t.sol | 2 +- test/CSAccounting.t.sol | 22 +++++++--------------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol index a9abe070..fb9a9b06 100644 --- a/src/CSAccounting.sol +++ b/src/CSAccounting.sol @@ -66,6 +66,11 @@ contract CSAccountingBase { uint256 penaltyETH, uint256 coveringETH ); + + error NotOwnerToClaim(address msgSender, address owner); + error InvalidBlockedBondRetentionPeriod(); + error InvalidStolenAmount(); + error InvalidSender(); } contract CSAccounting is CSAccountingBase, AccessControlEnumerable { @@ -109,11 +114,6 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { mapping(uint256 => uint256) internal _bondShares; mapping(uint256 => BlockedBond) internal _blockedBondEther; - error NotOwnerToClaim(address msgSender, address owner); - error InvalidBlockedBondRetentionPeriod(); - error InvalidStolenAmount(); - error InvalidSender(); - /// @param commonBondSize common bond size in ETH for all node operators. /// @param admin admin role member address /// @param lidoLocator lido locator contract address @@ -459,6 +459,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { uint256 nodeOperatorId, uint256 stETHAmount ) external returns (uint256) { + // todo: can it be two functions rather than one with `from` param and condition? from = _validateDepositSender(from); return _depositStETH(from, nodeOperatorId, stETHAmount); } @@ -472,6 +473,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { uint256 stETHAmount, PermitInput calldata permit ) external returns (uint256) { + // todo: can it be two functions rather than one with `from` param and condition? from = _validateDepositSender(from); // preventing revert for already used permit if (_lido().allowance(from, address(this)) < permit.value) { @@ -511,6 +513,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { uint256 nodeOperatorId, uint256 wstETHAmount ) external returns (uint256) { + // todo: can it be two functions rather than one with `from` param and condition? from = _validateDepositSender(from); return _depositWstETH(from, nodeOperatorId, wstETHAmount); } @@ -524,6 +527,7 @@ contract CSAccounting is CSAccountingBase, AccessControlEnumerable { uint256 wstETHAmount, PermitInput calldata permit ) external returns (uint256) { + // todo: can it be two functions rather than one with `from` param and condition? from = _validateDepositSender(from); // preventing revert for already used permit if (WSTETH.allowance(from, address(this)) < permit.value) { diff --git a/test/CSAccounting.blockedBond.t.sol b/test/CSAccounting.blockedBond.t.sol index df109088..b6c5121e 100644 --- a/test/CSAccounting.blockedBond.t.sol +++ b/test/CSAccounting.blockedBond.t.sol @@ -432,7 +432,7 @@ contract CSAccountingTest is Test, Fixtures, CSAccountingBase { function test_initELRewardsStealingPenalty_revertWhenZero() public { _createNodeOperator({ ongoingVals: 1, withdrawnVals: 0 }); - vm.expectRevert(0x7edd4cfd); + vm.expectRevert(InvalidStolenAmount.selector); vm.prank(admin); accounting.initELRewardsStealingPenalty({ diff --git a/test/CSAccounting.t.sol b/test/CSAccounting.t.sol index d3db8dfc..b320313d 100644 --- a/test/CSAccounting.t.sol +++ b/test/CSAccounting.t.sol @@ -562,19 +562,19 @@ contract CSAccountingTest is } function test_depositETH_RevertIfInvalidSender() public { - vm.expectRevert(0xddb5de5e); + vm.expectRevert(InvalidSender.selector); vm.prank(stranger); accounting.depositETH{ value: 0 }(user, 0); } function test_depositStETH_RevertIfInvalidSender() public { - vm.expectRevert(0xddb5de5e); + vm.expectRevert(InvalidSender.selector); vm.prank(stranger); accounting.depositStETH(user, 0, 32 ether); } function test_depositStETHWithPermit_RevertIfInvalidSender() public { - vm.expectRevert(0xddb5de5e); + vm.expectRevert(InvalidSender.selector); vm.prank(stranger); accounting.depositStETHWithPermit( user, @@ -592,13 +592,13 @@ contract CSAccountingTest is } function test_depositWstETH_RevertIfInvalidSender() public { - vm.expectRevert(0xddb5de5e); + vm.expectRevert(InvalidSender.selector); vm.prank(stranger); accounting.depositWstETH(user, 0, 32 ether); } function test_depositWstETHWithPermit_RevertIfInvalidSender() public { - vm.expectRevert(0xddb5de5e); + vm.expectRevert(InvalidSender.selector); vm.prank(stranger); accounting.depositWstETHWithPermit( user, @@ -993,11 +993,7 @@ contract CSAccountingTest is _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); vm.expectRevert( - abi.encodeWithSelector( - CSAccounting.NotOwnerToClaim.selector, - stranger, - user - ) + abi.encodeWithSelector(NotOwnerToClaim.selector, stranger, user) ); vm.prank(stranger); accounting.claimRewardsStETH(new bytes32[](1), 0, 1, 1 ether); @@ -1104,11 +1100,7 @@ contract CSAccountingTest is _createNodeOperator({ ongoingVals: 16, withdrawnVals: 0 }); vm.expectRevert( - abi.encodeWithSelector( - CSAccounting.NotOwnerToClaim.selector, - stranger, - user - ) + abi.encodeWithSelector(NotOwnerToClaim.selector, stranger, user) ); vm.prank(stranger); accounting.claimRewardsWstETH(new bytes32[](1), 0, 1, 1 ether);