Using `isCollateralized` to determine the first harvest is problematic

[hats-bug-reporter[bot]]

Github username: @milotruck Submission hash (on-chain): 0x18dc25d76f3d20d5f0e2870701ae440949a830b4b27c2059229c3c6ca4388ea2 Severity: high

Description:

Bug Description

In EthGenesisVault.sol, on the first harvest, the total rewards accumulated in the legacy pool is deducted. updateState() determines if the current call is the first harvest by checking if the vault is collateralized, as shown below:

EthGenesisVault.sol#L99-L110

  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());
    }

However, the !isCollateralized check might never pass as _harvestAssets() will revert when the vault is not collateralized. _harvestAssets() eventually calls KeeperRewards.harvest(), which validates that the vault (msg.sender below) exists in the merkle tree stored in params.rewardsRoot:

KeeperRewards.sol#L177-L188

    // verify the proof
    if (
      !MerkleProof.verifyCalldata(
        params.proof,
        params.rewardsRoot,
        keccak256(
          bytes.concat(keccak256(abi.encode(msg.sender, params.reward, params.unlockedMevReward)))
        )
      )
    ) {
      revert Errors.InvalidProof();
    }

According to the protocol team in this comment, only vaults with at least one validator registered will be added to the merkle tree:

That would be an issue if all the vaults were immediately added to the Merkle tree. However, oracles only add vaults to the Merkle tree with at least one validator registered. After the vault is added to the tree, it remains there forever.

This is a problem as:

1. updateState() will always revert when it has no registered validators.
2. After a validator is registered, the vault becomes collateralized immediately.

Therefore, it is not possible for the !isCollateralized check in updateState() to pass, which means rewards accumulated in the legacy pool will never be deducted.

This issue also cannot be mitigated by adding the Genesis Vault to the merkle tree before a validator is registered, as KeeperRewards.harvest() sets the vault's reward nonce to the latest nonce:

    // update state
    rewards[msg.sender] = Reward({nonce: currentNonce, assets: params.reward});

This would make the vault collateralized when updateState() is called, allowing users to call functions such as enterExitQueue() and mintOsToken(), which should not be callable before the vault registers a validator (refer to #34 for a more in-depth explanation).

Impact

Rewards in the legacy pool will never be deducted, which causes incorrect accounting for users when migrating from V2 to V3.

More specifically, since totalPenalty in RewardEthToken.sol will not be initialized, users will gain more shares than expected when calling migrate() from V2. This leads to a loss of funds for stakers who deposit directly into the Genesis Vault, as users that are migrating from V2 will have more shares and therefore will be able to withdraw more ETH.

Recommended Mitigation

Consider using a state variable to determine if the current call to updateState() is the first harvest:

bool public totalRewardsDeducted;

EthGenesisVault.sol#L99-L110

  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) {
+   if (!totalRewardsDeducted) {
      // it's the first harvest, deduct rewards accumulated so far in legacy pool
      totalAssetsDelta -= SafeCast.toInt256(_rewardEthToken.totalRewards());
+     totalRewardsDeducted = true;
    }

This ensures rewards in the legacy pool are deducted when updateState() is called for the first time after a validator is registered.

[tsudmi]

Not sure I fully understand the issue here. The plan with EthGenesisVault is the following:

1. Deploy the vault. The updateState cannot be called as the vault is not in the merkle tree of Keeper.
2. The GenesisVault is included in the first merkle tree update as the initial validators are taken from the StakeWise v2.
3. The updateState is called for the GenesisVault that makes it collateralized and executes the line totalAssetsDelta -= SafeCast.toInt256(_rewardEthToken.totalRewards()); as it was not collateralized before.

[MiloTruck]

In that case, users will be able to call enterExitQueue() and mintOsToken() immediately after updateState() is called, even if the Genesis Vault doesn't have a registered validator.

If this is intended behavior, then I guess there isn't an issue.