Users can `migrate()` before the first harvest to gain more shares

[hats-bug-reporter[bot]]

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

Description:

Bug Description

In EthGenesisVault.sol, on the first harvest, the total rewards accumulated in the legacy pool is deducted:

EthGenesisVault.sol#L107-L110

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

Since almost all assets will still be in the legacy pool, most of the deduction penalty will be passed on to V2's RewardETHToken contract by calling updateTotalRewards():

EthGenesisVault.sol#L115-L125

    // 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 {

In the RewardEthToken contract, the penalty will be added to totalPenalty:

RewardEthToken.sol#L246-L251

        } else if (rewardsDelta < 0) {
            uint256 _totalPenalty = totalPenalty; // gas savings
            _totalPenalty = _totalPenalty.add(uint256(- rewardsDelta));
            require(_totalPenalty <= totalAssets(), "RewardEthToken: invalid penalty amount");
            totalPenalty = _totalPenalty;
        }

Therefore, after the first harvest, users from V2 that are migrating to V3 will receive shares based on only their assets in StakedEthToken, and will not receive shares for accrued rewards, due to totalPenalty:

RewardEthToken.sol#L323-L330

        uint256 _totalPenalty = totalPenalty; // gas savings
        if (_totalPenalty > 0) {
            uint256 _totalAssets = totalAssets(); // gas savings
            // apply penalty to assets
            uint256 assetsAfterPenalty = assets.mul(_totalAssets.sub(_totalPenalty)).div(_totalAssets);
            totalPenalty = _totalPenalty.add(assetsAfterPenalty).sub(assets);
            assets = assetsAfterPenalty;
        }

However, there is no check in migrate() that ensures the vault is collateralized, which means users can call migrate() before updateState() has been called once.

Therefore, users can migrate from V2 to V3 before the first harvest, which means totalPenalty would not have been initialized yet. As such, both their assets and rewards in V2 will be migrated, allowing them to unfairly gain more shares in the Genesis Vault compared to users who migrate after the first harvest.

Impact

Users can migrate from V2 before the first harvest to unfairly gain more shares in the Genesis Vault, as compared to users who migrate after the first harvest is made.

This results in a direct loss of funds for users who migrate after the first harvest, as they will gain less shares for migrating the same amount of assets and rewards from V2.

Recommended Mitigation

Consider allowing migrate() to only be called after the Genesis Vault has been collateralized:

EthGenesisVault.sol#L146-L149

  function migrate(address receiver, uint256 assets) external override returns (uint256 shares) {
    if (msg.sender != address(_rewardEthToken)) revert Errors.AccessDenied();

    _checkHarvested();
+   _checkCollateralized();

[tsudmi]

The first state update will only occur if totalPenalty is 0. That's checked by the oracles.

[MiloTruck]

@tsudmi I don't really understand what you're referring to and how that invalidates the issue, could you elaborate more in detail?

[tsudmi]

Hi @MiloTruck the oracles won't submit the first rewards root if totalPenalty > 0, so even if you migrate before the first state update, you won't get more shares than supposed. That's done to make sure the migration happens when the current validators run stable. As of today, the validators on StakeWise v2 were running with totalPenalty = 0 for the past 3 years, so the migration should happen with no delays to first reward update.

However, I think it's worth adding sanity checks for that first update in the GenesisVault as well, so marking this issue as low.

[tsudmi]

@MiloTruck Please check the fix here: https://github.com/stakewise/v3-core/pull/69/commits/1e181e3cc3ce8071fd9b4ed38e370589c5c2ceb3#diff-a8828d79f898f54dd4e294473e5a1ec5640194a61db843105e26499c2a3b85ccR107

[MiloTruck]

@tsudmi I see that you added the following check:

EthGenesisVault.sol#L107-L112

    if (!isCollateralized) {
      // it's the first harvest, deduct rewards accumulated so far in legacy pool
      totalAssetsDelta -= SafeCast.toInt256(_rewardEthToken.totalRewards());
      // the first state update must be with positive delta
      if (totalAssetsDelta < 0) revert Errors.NegativeAssetsDelta(); // @audit this check
    }

Just to confirm, totalAssetsDelta will never be negative during the first state update? From what I remember, it should always be negative, but my understanding of the Genesis Vault might be flawed.

Apart from this the fix here looks good to me, users will no longer be able to call migrate() before the Genesis vault is collateralised.

[tsudmi]

Hi @MiloTruck Yes, totalAssetsDelta will not be negative during the first state update if StakeWise validators continue to run with the same performance as now.