Node runners can steal crowdsourced funds from each other

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/GiantSavETHVaultPool.sol#L137-L157 https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/GiantMevAndFeesPool.sol#L126-L138

Vulnerability details

Impact

Node runners, while competing for crowdsourced funds, can withdraw the funds that were deposited via Giant pools from competitor's vaults, and send them to their vaults. This will also lock node runners out of deploying a validator when the full stake amount is collected.

Proof of Concept

The Giant pools implement the bringUnusedETHBackIntoGiantPool function that allows anyone to withdraw funds from a vault and send them back to the Giant pool:

• GiantSavETHVaultPool.sol#L137-L157:

  function bringUnusedETHBackIntoGiantPool(
      address[] calldata _savETHVaults,
      LPToken[][] calldata _lpTokens,
      uint256[][] calldata _amounts
  ) external {
      uint256 numOfVaults = _savETHVaults.length;
      require(numOfVaults > 0, "Empty arrays");
      require(numOfVaults == _lpTokens.length, "Inconsistent arrays");
      require(numOfVaults == _amounts.length, "Inconsistent arrays");
      for (uint256 i; i < numOfVaults; ++i) {
          SavETHVault vault = SavETHVault(_savETHVaults[i]);
          for (uint256 j; j < _lpTokens[i].length; ++j) {
              require(
                  vault.isDETHReadyForWithdrawal(address(_lpTokens[i][j])) == false,
                  "ETH is either staked or derivatives minted"
              );
          }
  
          vault.burnLPTokens(_lpTokens[i], _amounts[i]); // @audit-issue HIGH, anyone can burn all LP tokens
      }
  }

• GiantMevAndFeesPool.sol#L126-L138:

  function bringUnusedETHBackIntoGiantPool(
      address[] calldata _stakingFundsVaults,
      LPToken[][] calldata _lpTokens,
      uint256[][] calldata _amounts
  ) external {
      uint256 numOfVaults = _stakingFundsVaults.length;
      require(numOfVaults > 0, "Empty arrays");
      require(numOfVaults == _lpTokens.length, "Inconsistent arrays");
      require(numOfVaults == _amounts.length, "Inconsistent arrays");
      for (uint256 i; i < numOfVaults; ++i) {
          StakingFundsVault(payable(_stakingFundsVaults[i])).burnLPTokensForETH(_lpTokens[i], _amounts[i]);
      }
  }

While this mechanism allows stakers to withdraw funds from stale vaults, it can be also abused by node runners to remove funds from the vaults of their competitors and lock their competitors out of deploying a validator.

Example Exploit Scenario

1. Alice and Bob are node validators in different LSD managers (thus, they'll use different savETH and MEV+fees vaults).
2. Users have deposited 24 ETH to the GiantSavETHVaultPool pool and 4 ETH to the GiantMevAndFeesPool pool (these pools are global and used by all LSD managers).
3. Bob calls the batchDepositETHForStaking functions of the Giant pools to deposit the users' funds into the vaults of his LSD manager.
4. Before Bob calls the stake function of his LSD manager (LiquidStakingManager.sol#L524) to register a validator, Alice calls the bringUnusedETHBackIntoGiantPool functions of the Giant pools and removes the users' funds from the vaults of the Bob's LSD manager.
5. Alice then calls batchDepositETHForStaking to withdraw funds from the Giant pools to her vault and calls stake to deploy a validator.

   Tools Used

   Manual Review

   Recommended Mitigation Steps

   Consider adding a function that allows node runners withdrawing funds from Giant pools and registering a validator in one transaction.

[dmvt]

Alice can only burn her own lpTokens. The call to burnLPTokensForEth leads us to a call to burnLPForETH which checks the balance of msg.sender and burns the sender's lpTokens.

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid