Loss of vault creator msg.value

[hats-bug-reporter]

Github username: @0xmahdirostami
Submission hash (on-chain): 0x7ec93058030614963b3bf5a4f2e1b97c0d34cf5a43596b9367ae9b23e29c6bb5
Severity: high

Description:
Description
As mentioned in docs:

• The security deposit of 1 gwei must be transferred with the call. This protects the vault stakers from the inflation attack described here

BUT it is not necessary for Stakewise vaults because vaults use the internal state for calculating shares not the balance of vaults:

_totalAssets is an internal state:

• Keeping track of the assets held by the vault internally removes the effect of direct transfers.
• Completely removes the inflation, removing all slippage

  function _convertToShares(
    uint256 assets,
    Math.Rounding rounding
  ) internal view returns (uint256 shares) {
    uint256 totalShares = _totalShares;
    // Will revert if assets > 0, totalShares > 0 and _totalAssets = 0.
    // That corresponds to a case where any asset would represent an infinite amount of shares.
    return
      (assets == 0 || totalShares == 0)
        ? assets
        : Math.mulDiv(assets, totalShares, _totalAssets, rounding);
  }

So it is a loss of funds for the vault creator.

Impact\

The Vault creator loses his msg.value for nothing

Attachments

Revised Code File (Optional)

As we need at least 1 wei for not reverting in the _convertToShares function, I recommend this code:

-    if (msg.value < _securityDeposit) revert Errors.InvalidSecurityDeposit();
-    _deposit(address(this), msg.value, address(0));
+    if (msg.value < 5) revert Errors.InvalidSecurityDeposit();
+    _deposit(address(this), 5, address(0));
+    _deposit(msg.sender, msg.value-5, address(0));

[0xmahdirostami]

Better Revised Code:

-  uint256 private constant _securityDeposit = 1e9;
.
.
.
  function __VaultEthStaking_init() internal onlyInitializing {
    __ReentrancyGuard_init();

     // see https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3706
-    if (msg.value < _securityDeposit) revert Errors.InvalidSecurityDeposit();
-    _deposit(address(this), msg.value, address(0));
+    if (msg.value == 0) revert Errors.InvalidSecurityDeposit();
+    _deposit(address(this), 1, address(0));
+    if (msg.value - 1 > 0){
+      _deposit(msg.sender, msg.value-1, address(0));
+      } 
+    
   }

It should be considered that Users might think that The more is deposited, the stronger is protection against inflation attack, SO users will lose more ETH.

[JeffCX]

duplicate of #12

[0xmahdirostami]

duplicate of #12
#12 is mine, and it's different from this issue, Here I describe a new issue in which Stakewise uses a security deposit of 1 gwei to prevent an inflation attack which is totally unnecessary, and it's just a loss of funds for the vault creator.

[0xmahdirostami]

@tsudmi Please close this one.