diff --git a/src/CSAccounting.sol b/src/CSAccounting.sol index 20c013a7..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,10 +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(); - /// @param commonBondSize common bond size in ETH for all node operators. /// @param admin admin role member address /// @param lidoLocator lido locator contract 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,39 +451,43 @@ 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) { + // todo: can it be two functions rather than one with `from` param and condition? + 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; - // solhint-disable-next-line func-named-parameters - _lido().permit( - from, - address(this), - permit.value, - permit.deadline, - permit.v, - permit.r, - permit.s - ); + ) 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) { + // 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); } @@ -487,7 +495,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; @@ -496,40 +505,43 @@ 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) { + // todo: can it be two functions rather than one with `from` param and condition? + 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) { - // solhint-disable-next-line func-named-parameters - WSTETH.permit( - from, - address(this), - permit.value, - permit.deadline, - permit.v, - permit.r, - permit.s - ); + ) 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) { + // 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); } @@ -537,7 +549,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); @@ -547,6 +560,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. @@ -742,7 +765,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..c3815e93 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 @@ -281,7 +280,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addNodeOperatorWstETHWithPermit( - address from, uint256 keysCount, bytes calldata publicKeys, bytes calldata signatures, @@ -298,7 +296,7 @@ contract CSModule is IStakingModule, CSModuleBase { _activeNodeOperatorsCount++; accounting.depositWstETHWithPermit( - from, + msg.sender, id, accounting.getRequiredBondWstETHForKeys(keysCount), permit @@ -346,7 +344,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addValidatorKeysStETHWithPermit( - address from, uint256 nodeOperatorId, uint256 keysCount, bytes calldata publicKeys, @@ -356,7 +353,7 @@ contract CSModule is IStakingModule, CSModuleBase { // TODO sanity checks accounting.depositStETHWithPermit( - from, + msg.sender, nodeOperatorId, accounting.getRequiredBondStETH(nodeOperatorId, keysCount), permit @@ -383,7 +380,6 @@ contract CSModule is IStakingModule, CSModuleBase { } function addValidatorKeysWstETHWithPermit( - address from, uint256 nodeOperatorId, uint256 keysCount, bytes calldata publicKeys, @@ -393,7 +389,7 @@ contract CSModule is IStakingModule, CSModuleBase { // TODO sanity checks accounting.depositWstETHWithPermit( - from, + msg.sender, nodeOperatorId, accounting.getRequiredBondWstETH(nodeOperatorId, keysCount), permit 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/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 07f3bcde..b320313d 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, @@ -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.allowance.selector, + user, + address(accounting) + ), + abi.encode(32 ether) + ); + + vm.recordLogs(); + + vm.prank(user); + 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); @@ -429,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, @@ -461,11 +504,117 @@ contract CSAccountingTest is ); } - function test_deposit_RevertIfNotExistedOperator() public { + 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.allowance.selector, + user, + address(accounting) + ), + abi.encode(32 ether) + ); + + vm.recordLogs(); + + vm.prank(user); + 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_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(InvalidSender.selector); + vm.prank(stranger); + accounting.depositETH{ value: 0 }(user, 0); + } + + function test_depositStETH_RevertIfInvalidSender() public { + vm.expectRevert(InvalidSender.selector); + vm.prank(stranger); accounting.depositStETH(user, 0, 32 ether); } + function test_depositStETHWithPermit_RevertIfInvalidSender() public { + vm.expectRevert(InvalidSender.selector); + 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(InvalidSender.selector); + vm.prank(stranger); + accounting.depositWstETH(user, 0, 32 ether); + } + + function test_depositWstETHWithPermit_RevertIfInvalidSender() public { + vm.expectRevert(InvalidSender.selector); + 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); @@ -844,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); @@ -955,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); diff --git a/test/CSMAddValidator.t.sol b/test/CSMAddValidator.t.sol index b8f504e0..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, @@ -212,12 +210,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, @@ -266,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, diff --git a/test/helpers/Permit.sol b/test/helpers/Permit.sol index 0c3dbd3a..acf22006 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 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,