New Users gain some rewards of old users

[hats-bug-reporter[bot]]

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

Description: Description\ As mentioned in the docs:

• "Registering new validators in the Beacon Chain can take several weeks during times when many new staking deposits are being made. ETH staked in a Vault and used to create a validator will not start producing staking rewards until the validator passes the deposit queue and becomes active. This may cause a dilution in Vault's APY. "

But in the current implementation users can deposit ETH and start earning rewards even if their deposited ETH isn't used in Beacon Chain yet

Impact\

When someone deposits, it shouldn't take rewards until Registering new validators in the Beacon Chain which takes several weeks, but in the current implementation, users get some share of rewards.

Attachments

Scenario\

• User A deposits ETH
• After some time start earning rewards
• User B deposits ETH as well and starts earning from the reward of A's deposited ETH

Revised Code File (Optional)

Don't mint share until users deposit ETH used by validators in the Beacon Chain.

[0xmahdirostami]

Revised Code File (Optional)

Don't mint share until users deposit ETH used by validators in the Beacon Chain or used by the exit queue or …

I mean, if they aren't used, so user shouldn't gain rewards.

[tsudmi]

Yeah, APR of the current depositors is diluted from new deposits until validators are activated. But there is no way to fix that without sacrificing UX.

[0xmahdirostami]

But it's so unfair, an attacker could watch vaults, and every time a vault gains a lot of rewards stake in the vault some hours before the update state and unstake after the update state, gain a lot of rewards, and repeat these steps to all vaults. This issue is high and must be considered. Users stake in vaults, to gain rewards, if anyone could steal their rewards, what is the point of staking?

At least it should be in that way: As `Registering new validators in the Beacon Chain can take several weeks: Don't mint share for new users unless their ETH used or be in vault more than 1 or 2 weeks(depending on your decision).

Here i write simple code

uint256 mintTime = 1 weeks; // depending on your decision
// some state to track users deposited ETH
  struct unboundETH {
    uint256 _amount;
    uint256 _time;
  } 
  mapping(bytes32 => unboundETH) internal _unboundETH;
  uint256 _unbound;

Deposit function

  function _deposit(
    address to,
    uint256 assets,
    address referrer
  ) internal virtual returns (uint256 shares) {
    _checkHarvested(); 
    if (to == address(0)) revert Errors.ZeroAddress();
    if (assets == 0) revert Errors.InvalidAssets();

    _unboundETH[keccak256(abi.encode(to, block.timestamp))] = unboundETH(assets, block.timestamp);

    uint256 totalAssetsAfter;
    unchecked {
      // cannot overflow as it is capped with underlying asset total supply
      totalAssetsAfter = _totalAssets + assets + _unbound ;;
    }
    if (totalAssetsAfter > capacity()) revert Errors.CapacityExceeded();
    _unbound += assets;

function for checking ETH (used by vault or not)

  function canredeemunboundETH(uint256 deposittime) public view returns(bool){
    if (deposittime==0){revert();}
    uint256 amount = _unboundETH[keccak256(abi.encode(msg.sender, deposittime))]._amount;
    uint256 time = _unboundETH[keccak256(abi.encode(msg.sender, deposittime))]._time;
    if (time != deposittime){revert();}
    if (_vaultAssets() >= amount){return true;}
    return false;
  }

function for checking ETH (being in vault more than 1 week or not)

  function checkunboundETHTime(uint256 deposittime) public view returns(bool){
     if (deposittime==0){revert();}
    uint256 time = _unboundETH[keccak256(abi.encode(msg.sender, deposittime))]._time;
    if (time != deposittime){revert();}
    if (block.timestamp >= time + mintTime){return true;}
    return false;
  }

function to redeeming unbound ETH

 function redeemunboundETH(uint256 deposittime) public {
    if (!canredeemunboundETH(deposittime)){revert();}
    uint256 amount = _unboundETH[keccak256(abi.encode(msg.sender, deposittime))]._amount;
    delete _unboundETH[keccak256(abi.encode(msg.sender, deposittime))];
    unbound -= amount;
    msg.sender.sendValue(amount);
  }

and function for creating shares

  function ETHToshare(uint256 deposittime) public {
    if (canredeemunboundETH(deposittime) && !checkunboundETHTime(deposittime)){revert();}
    uint256 assets = _unboundETH[keccak256(abi.encode(msg.sender, deposittime))]._amount;
    shares = _convertToShares(assets, Math.Rounding.Up);
    _unbound -= assets;
    _totalAssets += assets;
    _mintShares(msg.sender, shares);
  }

[JeffCX]

looks like at least a medium based on the comment above

[tsudmi]

Hey @0xmahdirostami , The gain will be very small if an attacker will do that on a daily basis. Also, once he stakes, the operator will spin up validators and the staker is risking of locking funds for a longer period of time. So I don't think the risk/reward is in favour of an attacker. We had similar functionality in StakeWise V2 and at some point decided to pause it as it affected protocol growth: https://forum.stakewise.io/t/swip-8-remove-the-activation-queue-for-deposits/639