Fix event values

contracts/contracts/strategies/NativeStaking/NativeStakingSSVStrategy.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.8.0;
 
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import "@openzeppelin/contracts/utils/math/Math.sol";
 
 import { InitializableAbstractStrategy } from "../../utils/InitializableAbstractStrategy.sol";
 import { IWETH9 } from "../../interfaces/IWETH9.sol";
@@ -18,6 +19,26 @@ struct ValidatorStakeData {
 /// @title Native Staking SSV Strategy
 /// @notice Strategy to deploy funds into DVT validators powered by the SSV Network
 /// @author Origin Protocol Inc
+/// @dev This contract handles WETH and ETH and in some operations interchanges between the two. Any WETH that
+/// is on the contract across multiple blocks (and not just transitory within a transaction) is considered an
+/// asset. Meaning deposits increase the balance of the asset and withdrawal decrease it. As opposed to all
+/// our other strategies the WETH doesn't immediately get deposited into an underlying strategy and can be present
+/// across multiple blocks waiting to be unwrapped to ETH and staked to validators. This separation of WETH and ETH is
+/// required since the rewards (reward token) is also in ETH.
+///
+/// To simplify the accounting of WETH there is another difference in behavior compared to the other strategies.
+/// To withdraw WETH asset - exit message is posted to validators and the ETH hits this contract with multiple days
+/// delay. In order to simplify the WETH accounting upon detection of such an event the ValidatorAccountant
+/// immediately wraps ETH to WETH and sends it to the Vault.
+///
+/// On the other hand any ETH on the contract (across multiple blocks) is there either:
+///  - as a result of already accounted for consensus rewards
+///  - as a result of not yet accounted for consensus rewards
+///  - as a results of not yet accounted for full validator withdrawals (or validator slashes)
+///
+/// Even though the strategy assets and rewards are a very similar asset the consensus layer rewards and the
+/// execution layer rewards are considered rewards and those are dripped to the Vault over a configurable time
+/// interval and not immediately.
 contract NativeStakingSSVStrategy is
     ValidatorAccountant,
     InitializableAbstractStrategy
@@ -33,8 +54,20 @@ contract NativeStakingSSVStrategy is
     /// (rewards for arranging transactions in a way that benefits the validator).
     address payable public immutable FEE_ACCUMULATOR_ADDRESS;
 
+    /// @dev This contract receives WETH as the deposit asset, but unlike other strategies doesn't immediately
+    /// deposit it to an underlying platform. Rather a special privilege account stakes it to the validators.
+    /// For that reason calling WETH.balanceOf(this) in a deposit function can contain WETH that has just been
+    /// deposited and also WETH that has previously been deposited. To keep a correct count we need to keep track
+    /// of WETH that has already been accounted for.
+    /// This value represents the amount of WETH balance of this contract that has already been accounted for by the
+    /// deposit events.
+    /// It is important to note that this variable is not concerned with WETH that is a result of full/partial
+    /// withdrawal of the validators. It is strictly concerned with WETH that has been deposited and is waiting to
+    /// be staked.
+    uint256 depositedWethAccountedFor;
+
     // For future use
-    uint256[50] private __gap;
+    uint256[49] private __gap;
 
     /// @param _baseConfig Base strategy config with platformAddress (ERC-4626 Vault contract), eg sfrxETH or sDAI,
     /// and vaultAddress (OToken Vault contract), eg VaultProxy or OETHVaultProxy
@@ -126,6 +159,7 @@ contract NativeStakingSSVStrategy is
         nonReentrant
     {
         require(_asset == WETH_TOKEN_ADDRESS, "Unsupported asset");
+        depositedWethAccountedFor += _amount;
         _deposit(_asset, _amount);
     }
 
@@ -154,8 +188,12 @@ contract NativeStakingSSVStrategy is
         uint256 wethBalance = IERC20(WETH_TOKEN_ADDRESS).balanceOf(
             address(this)
         );
-        if (wethBalance > 0) {
-            _deposit(WETH_TOKEN_ADDRESS, wethBalance);
+        uint256 newWeth = wethBalance - depositedWethAccountedFor;
+
+        if (newWeth > 0) {
+            depositedWethAccountedFor = wethBalance;
+
+            _deposit(WETH_TOKEN_ADDRESS, newWeth);
         }
     }
 
@@ -262,7 +300,20 @@ contract NativeStakingSSVStrategy is
         );
     }
 
-    function wethWithdrawnToVault(uint256 _amount) internal override {
+    function _wethWithdrawnToVault(uint256 _amount) internal override {
         emit Withdrawal(WETH_TOKEN_ADDRESS, address(0), _amount);
     }
+
+    function _wethWithdrawnAndStaked(uint256 _amount) internal override {
+        /* In an ideal world we wouldn't need to reduce the deduction amount when the
+         * depositedWethAccountedFor is smaller than the _amount.
+         *
+         * The reason this is required is that a malicious actor could sent WETH direclty
+         * to this contract and that would circumvent the increase of depositedWethAccountedFor
+         * property. When the ETH would be staked the depositedWethAccountedFor amount could
+         * be deducted so much that it would be negative.
+         */
+        uint256 deductAmount = Math.min(_amount, depositedWethAccountedFor);
+        depositedWethAccountedFor -= deductAmount;
+    }
 }

contracts/contracts/strategies/NativeStaking/ValidatorAccountant.sol

@@ -42,7 +42,8 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
 
     event AccountingManuallyFixed(
         int256 validatorsDelta,
-        int256 consensusRewardsDelta
+        int256 consensusRewardsDelta,
+        uint256 wethToVault
     );
 
     /// @param _wethAddress Address of the Erc20 WETH Token contract
@@ -132,8 +133,8 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
             IWETH9(WETH_TOKEN_ADDRESS).deposit{ value: wethToVault }();
             // slither-disable-next-line unchecked-transfer
             IWETH9(WETH_TOKEN_ADDRESS).transfer(VAULT_ADDRESS, wethToVault);
-            wethWithdrawnToVault(wethToVault);
-            
+            _wethWithdrawnToVault(wethToVault);
+
             emit AccountingFullyWithdrawnValidator(
                 fullyWithdrawnValidators,
                 activeDepositedValidators,
@@ -163,7 +164,7 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
             IWETH9(WETH_TOKEN_ADDRESS).transfer(VAULT_ADDRESS, ethRemaining);
             activeDepositedValidators -= 1;
 
-            wethWithdrawnToVault(ethRemaining);
+            _wethWithdrawnToVault(ethRemaining);
 
             emit AccountingValidatorSlashed(
                 activeDepositedValidators,
@@ -194,9 +195,16 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
     /// @notice Allow the Strategist to fix the accounting of this strategy and unpause.
     /// @param _validatorsDelta adjust the active validators by up to plus three or minus three
     /// @param _consensusRewardsDelta adjust the accounted for consensus rewards up or down
+    /// @param _ethToVaultAmount the amount of ETH that gets wrapped into WETH and sent to the Vault
+    /// @dev There is a case when a validator(s) gets slashed so much that the eth swept from
+    /// the beacon chain enters the fuse area and there are no consensus rewards on the contract
+    /// to "dip into"/use. To increase the amount of unaccounted ETH over the fuse end interval
+    /// we need to reduce the amount of active deposited validators and immediately send WETH
+    /// to the vault, so it doesn't interfere with further accounting.
     function manuallyFixAccounting(
         int256 _validatorsDelta,
-        int256 _consensusRewardsDelta
+        int256 _consensusRewardsDelta,
+        uint256 _ethToVaultAmount
     ) external onlyStrategist whenPaused {
         require(
             lastFixAccountingBlockNumber + MIN_FIX_ACCOUNTING_CADENCE <
@@ -217,17 +225,30 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
                 int256(consensusRewards) + _consensusRewardsDelta >= 0,
             "invalid consensusRewardsDelta"
         );
+        require(_ethToVaultAmount <= 32 ether * 3, "invalid wethToVaultAmount");
 
-        emit AccountingManuallyFixed(_validatorsDelta, _consensusRewardsDelta);
+        emit AccountingManuallyFixed(
+            _validatorsDelta,
+            _consensusRewardsDelta,
+            _ethToVaultAmount
+        );
 
         activeDepositedValidators = uint256(
             int256(activeDepositedValidators) + _validatorsDelta
         );
         consensusRewards = uint256(
             int256(consensusRewards) + _consensusRewardsDelta
         );
-
         lastFixAccountingBlockNumber = block.number;
+        if (_ethToVaultAmount > 0) {
+            IWETH9(WETH_TOKEN_ADDRESS).deposit{ value: _ethToVaultAmount }();
+            // slither-disable-next-line unchecked-transfer
+            IWETH9(WETH_TOKEN_ADDRESS).transfer(
+                VAULT_ADDRESS,
+                _ethToVaultAmount
+            );
+            _wethWithdrawnToVault(_ethToVaultAmount);
+        }
 
         // rerun the accounting to see if it has now been fixed.
         // Do not pause the accounting on failure as it is already paused
@@ -242,5 +263,5 @@ abstract contract ValidatorAccountant is ValidatorRegistrator {
     ****************************************/
 
     /// @dev allows for NativeStakingSSVStrategy contract to emit Withdrawal event
-    function wethWithdrawnToVault(uint256 _amount) internal virtual;
+    function _wethWithdrawnToVault(uint256 _amount) internal virtual;
 }

contracts/contracts/strategies/NativeStaking/ValidatorRegistrator.sol

@@ -114,6 +114,7 @@ abstract contract ValidatorRegistrator is Governable, Pausable {
 
         // Convert required ETH from WETH
         IWETH9(WETH_TOKEN_ADDRESS).withdraw(requiredETH);
+        _wethWithdrawnAndStaked(requiredETH);
 
         uint256 validatorsLength = validators.length;
         // For each validator
@@ -247,4 +248,11 @@ abstract contract ValidatorRegistrator is Governable, Pausable {
             cluster
         );
     }
+
+    /***************************************
+                 Abstract
+    ****************************************/
+
+    /// @dev allows for NativeStakingSSVStrategy contract know how much WETH had been staked
+    function _wethWithdrawnAndStaked(uint256 _amount) internal virtual;
 }

contracts/test/behaviour/ssvStrategy.js

@@ -127,16 +127,23 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
   });
 
   describe("Validator operations", function () {
-    beforeEach(async () => {
-      const { weth, domen, nativeStakingSSVStrategy } = await context();
+    const depositToStrategy = async (amount) => {
+      const { weth, domen, nativeStakingSSVStrategy, oethVault, strategist } =
+        await context();
 
-      // Add 32 WETH to the strategy so it can be staked
-      await weth
-        .connect(domen)
-        .transfer(nativeStakingSSVStrategy.address, oethUnits("32"));
-    });
+      // Add 32 WETH to the strategy via a Vualt deposit
+      await weth.connect(domen).transfer(oethVault.address, amount);
+
+      return await oethVault
+        .connect(strategist)
+        .depositToStrategy(
+          nativeStakingSSVStrategy.address,
+          [weth.address],
+          [amount]
+        );
+    };
 
-    it("Should register and staked 32 ETH by validator registrator", async () => {
+    const registerAndStakeEth = async () => {
       const {
         addresses,
         weth,
@@ -157,15 +164,15 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
         ssvNetwork: addresses.SSVNetwork,
       });
 
-      const stakeAmount = oethUnits("32");
-
       await setERC20TokenBalance(
         nativeStakingSSVStrategy.address,
         ssv,
         "1000",
         hre
       );
 
+      const stakeAmount = oethUnits("32");
+
       // Register a new validator with the SSV Network
       const regTx = await nativeStakingSSVStrategy
         .connect(validatorRegistrator)
@@ -180,7 +187,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
         .to.emit(nativeStakingSSVStrategy, "SSVValidatorRegistered")
         .withArgs(testValidator.publicKey, testValidator.operatorIds);
 
-      // Stake 32 ETH to the new validator
+      // Stake stakeAmount ETH to the new validator
       const stakeTx = await nativeStakingSSVStrategy
         .connect(validatorRegistrator)
         .stakeEth([
@@ -195,7 +202,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
         .to.emit(nativeStakingSSVStrategy, "ETHStaked")
         .withNamedArgs({
           pubkey: testValidator.publicKey,
-          amount: stakeAmount,
+          amount: oethUnits("32"),
         });
 
       expect(await weth.balanceOf(nativeStakingSSVStrategy.address)).to.equal(
@@ -204,6 +211,43 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
           "strategy WETH not decreased"
         )
       );
+    };
+
+    it("Should register and stake 32 ETH by validator registrator", async () => {
+      await depositToStrategy(oethUnits("32"));
+      await registerAndStakeEth();
+    });
+
+    it("Should emit correct values in deposit event", async () => {
+      const { weth, nativeStakingSSVStrategy } = await context();
+
+      await depositToStrategy(oethUnits("40"));
+      // at least 8 WETH has remained on the contract and a deposit all
+      // event should emit a correct amount
+      await registerAndStakeEth();
+
+      /* deposit to strategy calls depositAll on the strategy contract after sending the WETH
+       * to it. The event should contain only the amount of newly deposited WETH and not include
+       * the pre-exiting WETH.
+       */
+      const tx = await depositToStrategy(parseEther("10"));
+
+      await expect(tx)
+        .to.emit(nativeStakingSSVStrategy, "Deposit")
+        .withArgs(weth.address, AddressZero, parseEther("10"));
+    });
+
+    it("Should register and stake 32 ETH even if half supplied by a 3rd party", async () => {
+      const { weth, domen, nativeStakingSSVStrategy } = await context();
+
+      await depositToStrategy(oethUnits("16"));
+      // A malicious actor is sending WETH directly to the native staking contract hoping to
+      // mess up the accounting.
+      await weth
+        .connect(domen)
+        .transfer(nativeStakingSSVStrategy.address, oethUnits("16"));
+
+      await registerAndStakeEth();
     });
 
     it("Should exit and remove validator by validator registrator", async () => {
@@ -215,6 +259,7 @@ const shouldBehaveLikeAnSsvStrategy = (context) => {
         addresses,
         testValidator,
       } = await context();
+      await depositToStrategy(oethUnits("32"));
 
       const { cluster } = await getClusterInfo({
         ownerAddress: nativeStakingSSVStrategy.address,