User assets will be affected, if EthGenesisVault, if isn't Collateralized

[hats-bug-reporter[bot]]

Github username: @0xmahdirostami Submission hash (on-chain): 0x5bf89400ff29ef77025bade7f19cc3022b7c6a28aa4a0f72646d2310d2c9720b Severity: medium

Description: Description\ In vaults, if a user deposits ETH to the vault and doesn't register the validator updateState doesn't affect the user's asset but in EthGenesisVault As the Genesis Vault is linked to the legacy Stakewise V2 system, the updateState function affects the user's asset. https://github.com/stakewise/v3-core/blob/c82fc57d013a19967576f683c5e41900cbdd0e67/contracts/vaults/ethereum/EthGenesisVault.sol#L107-L137 So if a user deposits ETH to the vault and doesn't register the validator, As the Genesis Vault is linked to the legacy Stakewise V2 system if anyone calls updatesate, it affects the user's asset.

Impact \

• affects the user's assets.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   As "The Genesis vault will become available for the users(v2 users) (added to UI) only after first harvest or validator registration happens.", if the vault isn't Collateralized, there is no need for updatestate.

[tsudmi]

What do you mean by affects assets?

[0xmahdirostami]

Make it like other vaults, and In the first updateState() just update for v2, after the first updateState(), as the vault become Collateralized, do updateState() for both v2 and v3.

  function updateState(
    IKeeperRewards.HarvestParams calldata harvestParams
  ) public override(IVaultState, VaultState) {
    bool isCollateralized = IKeeperRewards(_keeper).isCollateralized(address(this));

    // process total assets delta since last update
    int256 totalAssetsDelta = _harvestAssets(harvestParams);

    if (!isCollateralized) {
      // it's the first harvest, deduct rewards accumulated so far in legacy pool
      totalAssetsDelta -= SafeCast.toInt256(_rewardEthToken.totalRewards());
    }

    // fetch total assets controlled by legacy pool
    uint256 legacyPrincipal = _rewardEthToken.totalAssets() - _rewardEthToken.totalPenalty();

    // calculate total principal
    uint256 totalPrincipal = _totalAssets + legacyPrincipal;
    if (totalAssetsDelta < 0) {
      // calculate and update penalty for legacy pool
      int256 legacyPenalty = SafeCast.toInt256(
        Math.mulDiv(uint256(-totalAssetsDelta), legacyPrincipal, totalPrincipal)
      );
      _rewardEthToken.updateTotalRewards(-legacyPenalty);
      // deduct penalty from total assets delta
      totalAssetsDelta += legacyPenalty;
    } else { 
      // calculate and update reward for legacy pool
      int256 legacyReward = SafeCast.toInt256(
        Math.mulDiv(uint256(totalAssetsDelta), legacyPrincipal, totalPrincipal)
      );
      _rewardEthToken.updateTotalRewards(legacyReward);
      // deduct reward from total assets delta
      totalAssetsDelta -= legacyReward;
    }

+   if (isCollateralized) {

      if (totalAssetsDelta != 0) {
      super._processTotalAssetsDelta(totalAssetsDelta);
    }
    // update exit queue
    if (canUpdateExitQueue()) {
      _updateExitQueue();
    }
    }
  }

In this way, if the vault isn't collateralized, v3 users' _totalAssets doesn't change.

[tsudmi]

The EthGenesisVault is collateralized right from the start as all the validators from v2 will belong to v3. For other vaults as they don't have validators from the start, so they're not collateralized.

[0xmahdirostami]

The EthGenesisVault is collateralized right from the start as all the validators from v2 will belong to v3. For other vaults as they don't have validators from the start, so they're not collateralized. @tsudmi In the first place, there is 1e9 Wei of the asset and 1e9 shares in the vault, and in current implementation in the first updatestate, this _totalAsset could be changed,

bool isCollateralized = IKeeperRewards(_keeper).isCollateralized(address(this));

// process total assets delta since last update
int256 totalAssetsDelta = _harvestAssets(harvestParams);

if (!isCollateralized) {
  // it's the first harvest, deduct rewards accumulated so far in legacy pool
  totalAssetsDelta -= SafeCast.toInt256(_rewardEthToken.totalRewards());
}

// fetch total assets controlled by legacy pool
uint256 legacyPrincipal = _rewardEthToken.totalAssets() - _rewardEthToken.totalPenalty();

// calculate total principal
uint256 totalPrincipal = _totalAssets + legacyPrincipal;
if (totalAssetsDelta < 0) {
  // calculate and update penalty for legacy pool
  int256 legacyPenalty = SafeCast.toInt256(
    Math.mulDiv(uint256(-totalAssetsDelta), legacyPrincipal, totalPrincipal)
  );
  _rewardEthToken.updateTotalRewards(-legacyPenalty);
  // deduct penalty from total assets delta
  totalAssetsDelta += legacyPenalty;

if (totalAssetsDelta != 0) { super._processTotalAssetsDelta(totalAssetsDelta); }

so the 1:1 share to asset ratio will be changed and even if this change is too much, the vault becomes vulnerable to a share manipulation attack.
So for the first updatestate it should be better to just update for v2.
```diff
+   if (isCollateralized) {

      if (totalAssetsDelta != 0) {
      super._processTotalAssetsDelta(totalAssetsDelta);
    }
    // update exit queue
    if (canUpdateExitQueue()) {
      _updateExitQueue();
    }

[0xmahdirostami]

Please close this one, I create new issue #130 .