LSD Network - Stakehouse contest - aphak5010's results

Lines of code

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

Vulnerability details

Impact

The StakingFundsVault is used to stake 4 ETH. The stakers of the 4 ETH then get rewarded 50% of the fees of the validator according to the amount of ETH that they have staked. The rewards are actually distributed in the SyndiacteRewardsProcessor._distributeRewardsToUserForToken function (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L51).

The problem is that in this function, the total claimed ETH of a staker is not calculated correctly.

It is currently calculated as:

claimed[_user][_token] = due;

But it should be calculated as:

claimed[_user][_token] += due;

Affected line: https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L63

Currently when the function is called, previous reward payouts are forgotten about.

This allows a staker to receive more rewards then he should be able to, causing a loss of funds for the other stakers.

Proof of Concept

The issue can be seen easily by the following scenario:

1. Bob has so far received 1 ETH rewards (claimed["Bob"][token] = 1 ETH)
2. The StakingFundsVault receives rewards such that Bob should now be able to claim another 2 ETH
3. The StakingFundsVault.claimRewards function is called by Bob which in turn calls SyndiacteRewardsProcessor._distributeRewardsToUserForToken. Bob receives 2 ETH and the wrong value is set for his claimed rewards (claimed["Bob"][token] = 2 ETH instead of claimed["Bob"][token] = 3 ETH)
4. Now Bob can claim rewards again. This can go on indefinitely.

Tools Used

VSCode

Recommended Mitigation Steps

Change:

claimed[_user][_token] = due;

to:

claimed[_user][_token] += due;

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L170 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L343

Vulnerability details

Impact

This issue exists in GiantMevAndFeesPool as well as in StakingFundsVault.

From here on I will explain the issue based on the GiantMevAndFeesPool but the explanation can be transferred easily to the StakingFundsVault.

The holders of a GiantMevAndFeesPools GiantLP tokens receive a share of the staking rewards.

In order to handle the rewards correctly in case the tokens are transferred, the GiantMevAndFeesPool implements the beforeTokenTransfer (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L146) and afterTokenTransfer (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L170) functions.

The beforeTokenTransfer function sends all pending rewards to the sender and receiver of the token transfer.

The afterTokenTransfer functions updates the amount of claimed rewards for the receiver of the token transfer. This is done to disallow the receiver to claim rewards for tokens that the sender has already claimed rewards for.

The problem is that the claimed rewards for the sender are not adapted in the afterTokenTransfer.

The claimed rewards for the sender should be lowered to reflect the sender's now lower balance.

This causes the sender to miss out on future rewards (-> loss of funds).

Proof of Concept

Consider the following scenario:

• Alice owns 5 GiantLP tokens
• Bob owns 5 GiantLP tokens
• No rewards were claimed so far and 1 ETH per GiantLP tokens is the current reward

1. Alice wants to send 1 GiantLP to Bob
2. The beforeTokenTransfer is called and rewards for Alice and Bob are claimed (5 ETH for Alice and 5 ETH for Bob)
3. The transfer is executed. Alice owns 4 GiantLP. Bob owns 6 GiantLP
4. The afterTokenTransfer function is executed. Bob's claimed amount will be set to 6 ETH, reflecting his new balance of 6 GiantLP.
5. Alice's claimed amount however will remain at 5 ETH. This means that she will miss out on future rewards. She will only start earning rewards when the reward per GiantLP reaches 5 ETH / 4 GiantLP = 1.25 ETH / GiantLP.

Tools Used

VSCode

Recommended Mitigation Steps

In the afterTokenTransfer function call _setClaimedToMax(_from):

-    function afterTokenTransfer(address, address _to, uint256) external {
+    function afterTokenTransfer(address _from, address _to, uint256) external {
         require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
         _setClaimedToMax(_to);
+        _setClaimedToMax(_from);
     }

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L82 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L96 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L141 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L184 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L230 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L17 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L44 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L45 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L20 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L67 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L68

Vulnerability details

Impact

The GiantLP and LPToken tokens record the last interaction time of a token transfer.
The lastInteractedTimestamp of an address is updated whenever the address is the sender or receiver of a transfer.

This lastInteractedTimestamp is then used in the application to allow certain operations only when a certain time since the last token interaction has passed (30 mins or 1 days).

Any user can therefore cause the lastInteractedTimestamp to be updated by making a token transfer transaction with zero value to the victim address.

So an attacker can deny a victim to perform any of the actions that depend on the checks that are listed below.
For example the attacker can deny the victim to withdraw LP tokens from a Giant Pool or deny him to claim rewards from a StakingFundsVault.

Instances where lastInteractedTimestamp is checked

ETHPoolLPFactory.rotateLPTokens

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L82
30 min check

GiantPoolBase._assertUserHasEnoughGiantLPToClaimVaultLP

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L96
1 days check
This function in turn is called in GiantSavETHVaultPool.withdrawDETH and GiantPoolBase.withdrawLPTokens.

SavETHVault.burnLPToken

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L141
30 min check

StakingFundsVault.burnLPForETH

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L184
30 min check

StakingFundsVault.claimRewards

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L230
30 min check

Tokens where lastInteractedTimestamp is implemented

GiantLP

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L17

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L44

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L45

LPToken

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L20

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L67

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L68

Proof of Concept

1. A victim deposits ETH into a Giant Pool. He is thereby minted lpTokenETH with a 1:1 ratio.
2. The victim waits 1 day and wants to withdraw his lp tokens now. However after 23 hours of the initial ETH deposit, the attacker transfers 0 lpTokenETH to the victim. This will cause the victim's lastInteractedTimestamp to be updated.
3. The victim's transaction to withdraw lp tokens will fail (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L96).
4. The attacker can send a transaction every <= 24 hours so the victim can never withdraw the lp tokens.

Tools Used

VSCode

Recommended Mitigation Steps

You might consider to only update the lastInteractedTimestamp of a user if:

1. The sender is the deployer of the token and the receiver is the user

Or

2. The sender is the user

In other words not any sender should be able to update the lastInteractedTimestamp of a user.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L53

Vulnerability details

Impact

The GiantPoolBase.withdrawETH function requires that the amount to withdraw is at least as big as the MIN_STAKING_AMOUNT (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L53).

This check does not serve any purpose and can actually cause the user problems when withdrawing his ETH.

Proof of Concept

1. Bob deposits ETH into the GiantPool with the GiantPoolBase.depositETH function.
   The amount is equal to MIN_STAKING_AMOUNT + 0.99 * MIN_STAKING_AMOUNT.
2. Bob witdraws MIN_STAKING_AMOUNT ETH from the GiantPool.
3. Bob has 0.99 * MIN_STAKING_AMOUNT ETH left in the GiantPool. This is a problem since he cannot withdraw this amount of ETH since it is smaller than MIN_STAKING_AMOUNT.
   In order to withdraw his funds, Bob needs to first add funds to the GiantPool such that the deposited amount is big enough for withdrawal. However this causes extra transaction fees to be paid (loss of funds) and causes a bad user experience.

Tools Used

VSCode

Recommended Mitigation Steps

The require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount"); statement should just be removed. It does not serve any purpose anyway.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L69 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L95

Vulnerability details

Impact

The GiantPoolBase.withdrawLPTokens function (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L69) allows to withdraw LP tokens from a GiantPool by burning an equal amount of GiantLP.

This allows a user to handle the LP tokens directly without the need for a GiantPool as intermediary.

It is not checked however whether the LP tokens to be withdrawn were transferred to the GiantPool in exchange for staking ETH.

I.e. whether the LP token are of any value.

There are two issues associated with this behavior.

1. A malicious user can create and mint his own LP Token and send it to the GiantPool. Users that want to withdraw LP tokens from the GiantPool can then be tricked into withdrawing worthless attacker LP tokens, thereby burning their GiantLP tokens that are mapped 1:1 to ETH. (-> loss of funds)

2. This can also mess up internal accounting logic. For every LP token that is owned by a GiantPool there should be a corresponding GiantLP token. Using the described behavior this ratio can be broken such that there are LP token owned by the GiantPool for which there is no GiantLP token. This means some LP token cannot be transferred from the GiantPool and there will always be some amount of LP token "stuck" in the GiantPool.

Proof of Concept

1. The attacker deploys his own LPToken contract and sends a huge amount of LP tokens to the GiantPool to pass the check in GiantPoolBase._assertUserHasEnoughGiantLPToClaimVaultLP (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L95).
2. The attacker tricks Bob into withdrawing the malicious LP tokens from the GiantPool (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L69).
3. Bob's GiantLP tokens are burnt and he receives worthless LP tokens.

The same issue exists for the GiantSavETHVaultPool.withdrawDETH function.
But in this case, the victim must also provide a wrong savETHVault address which makes this issue less likely to be exploited.

Tools Used

VSCode

Recommended Mitigation Steps

The GiantPool should store information about which LP tokens it receives for staking ETH. When calling the GiantPoolBase.withdrawLPTokens function it can then be checked if the LP tokens to be withdrawn were indeed transferred to the GiantPool in exchange for staking ETH.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L94 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L105-L106

Vulnerability details

Impact

The OwnableSmartWallet contract employs a mechanism for the owner to approve addresses that can then claim ownership (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L94) of the contract.

The source code has a comment included which states that "Approval is revoked, in order to avoid unintended transfer allowance if this wallet ever returns to the previous owner" (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L105-L106).

This means that when ownership is transferred from User A to User B, the approvals that User A has given should be revoked.

The existing code does not however revoke all approvals that User A has given. It only revokes one approval.

This can lead to unwanted transfers of ownership.

Proof of Concept

1. User A approves User B and User C to claim ownership
2. User B claims ownership first
3. Only User A's approval for User B is revoked, not however User A's approval for User C
4. User B transfers ownerhsip back to User A
5. Now User C can claim ownership even though this time User A has not approved User C

Tools Used

VSCode

Recommended Mitigation Steps

You should invalidate all approvals User A has given when another User becomes the owner of the OwnableSmartWallet.

Unfortunately you cannot use a statement like delete _isTransferApproved[owner()].

So you would need an array that keeps track of approvals as pointed out in this StackExchange question: https://ethereum.stackexchange.com/questions/15553/how-to-delete-a-mapping

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L127 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L116 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L22

Vulnerability details

Impact

The GiantSavETHVaultPool and GiantMevAndFeesPool both have a batchRotateLPTokens function that allows to move staked ETH to another key.

Both functions require that the GiantLP balance of the sender is >=0.5 ether.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L127

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L116

The reason for this is that there is a common interest needed in order to rotate LP Tokens.

The way this is implemented right now does not serve this purpose and even makes the functions unable to be called in some cases.

The MIN_STAKING_AMOUNT for the GiantPools is 0.001 ether (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L22).

So a user should expect that this amount is sufficient to properly use the contract.

However even if there are multiple users paying into the GiantPool, they might not reach the 0.5 ETH threshold to call the function.

So even if they would use some kind of multisig wallet to call the batchRotateLPTokens function, it would not be possible.

Also the threshold does not scale.

Imagine that User A puts 100 ETH into the GiantPool. Another User B puts 0.5 ETH into the GiantPool.

Can we speak of "common interest" when User B wants to rotate the LP Tokens?

Tools Used

VSCode

Recommended Mitigation Steps

My suggestion is to use a formula like:
require(lpTokenETH.balanceOf(msg.sender) >= (lpTokenETH.totalSupply() / CONSTANT_VALUE)).
Where you can choose a CONSTANT_VALUE like 20 or 50.

This properly scales the required amount and helps mitigate both scenarios.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L82 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L91

Vulnerability details

Impact

The GiantMevAndFeesPool.previewAccumulatedETH function (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L82) allows to view the ETH that is accumulated by an address.

However the formula is not correct.

In each iteration of the foor loop, accumulated is assigned a new value (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L91) when actually the value should be updated like this:

accumulated += StakingFundsVault(payable(_stakingFundsVaults[i])).batchPreviewAccumulatedETH(
        address(this),
        _lpTokens[i]
    );

Obviously the accumulated value must be calculated for all stakingFundVaults not only for one stakingFundsVault.

While this calculation is not used internally by the contract, it will cause any third-party contract that relies on this calculation to behave incorrectly.

For example a third party smart contract might only allow users to withdraw once the value returned by previewAccumulatedETH reaches a certain threshold. Because of the issue however the accumulated ETH value that is returned will always be too low.

Tools Used

VSCode

Recommended Mitigation Steps

Fix:

@@ -88,7 +88,7 @@ contract GiantMevAndFeesPool is ITransferHookProcessor, GiantPoolBase, Syndicate
 
         uint256 accumulated;
         for (uint256 i; i < _stakingFundsVaults.length; ++i) {
-            accumulated = StakingFundsVault(payable(_stakingFundsVaults[i])).batchPreviewAccumulatedETH(
+            accumulated += StakingFundsVault(payable(_stakingFundsVaults[i])).batchPreviewAccumulatedETH(
                 address(this),
                 _lpTokens[i]
             );

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L278-L284

Vulnerability details

Impact

The LiquidStakingManager.updateNodeRunnerWhitelistStatus (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L278-L284) function will always revert since isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner] always evaluates to false.

The worst case scenario that I can think of is:

1. The LSD Network starts out without node runner whitelisting enabled
2. For some reason it becomes necessary to enable node runner whitelisting
3. Node runner whitelisting cannot be enabled because the node runner whitelist status cannot be set to true

Proof of Concept

Basically just call the LiquidStakingManager.updateNodeRunnerWhitelistStatus function and see that it always reverts.

Tools Used

VSCode

Recommended Mitigation Steps

Change

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

to

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

LSD Network - Stakehouse Low Risk and Non-Critical Issues

Summary

[L-00] Missing zero-address checks

When an address is assigned to a variable, it should be checked whether the address is the zero-address.

In most cases it is not intended to assign the zero-address to the variable and it can cause the contract to be unusable.

There are 6 instances of this issue.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L25

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L38

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L110

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalGatekeeperFactory.sol#LL11C29-L11C50

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalHouseGatekeeper.sol#LL14C25-L14C33

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/SyndicateFactory.sol#L17

[L-01] SavETHVault.isBLSPublicKeyBanned does not behave as stated in comment

The comment documenting the behavior of SavETHVault.isBLSPublicKeyBanned explains the behavior of the function as "Utility function that proxies through to the liquid staking manager to check whether the BLS key ever registered with the network but is now banned" (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L222).

However when looking at the LiquidStakingManager.isBLSPublicKeyBanned function (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L500) it becomes clear that the function returns true not only when the public key is banned but also when the public key is not part of the LSD Network. The comment therefore is misleading and can negatively impact any third-party components that rely on it.

[L-02] Implement 2-Step-Process for assigning roles

In the LiquidStakingManager contract a 2-Step-Process should be used to set a new DAO address.
Currently the updateDAOAddress function is used for this (https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L239).
There should be a second function called claimDAO implemented that needs to be called by the new DAO to actually change the DAO address. This prevents setting a wrong DAO address by mistake.

[N-00] Unlocked pragma

Some of the files in scope use 0.8.13 as the Solidity compiler version. Others use ^0.8.13, meaning that the file will compile with versions >=0.8.13 and <0.9.0.
It is considered best practice to use a locked Solidity version, thereby only allowing compilation with a specific version.
So the instances of ^0.8.13 should be changed to 0.8.13.

There are 18 instances of this.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalGatekeeperFactory.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalHouseGatekeeper.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVaultDeployer.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVaultDeployer.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWalletFactory.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPTokenFactory.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L1

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L3

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

[N-01] Move SPDX-License-Identifier to top of the file

It is best practice to move the SPDX-License-Identifier to the top of the source file.
There are 12 instances where the SPDX-License-Identifier is after the Solidity version.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalHouseGatekeeper.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVaultDeployer.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPTokenFactory.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/SyndicateFactory.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L3

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/SyndicateErrors.sol#L3

[N-02] Event is missing indexed fields

Each event should use three indexed fields if it has three or more fields.

There are 35 instances of events that do not have 3 indexed fields.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L13

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L16

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L19

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L16

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L19

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L22

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L121

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L25

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L28

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L31

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L34

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L42

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L45

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L48

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L51

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L57

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L60

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L63

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L36

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L39

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L48

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L51

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L54

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L57

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L63

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L66

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L69

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L84

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L87

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L9

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L12

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

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

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

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

[N-03] Variable can be declared immutable

Variables that are only assigned in the constructor should be declared immutable.

There are 17 instances of this.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L11

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L14

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L28

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L31

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPTokenFactory.sol#L15

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalHouseGatekeeper.sol#L12

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVaultDeployer.sol#L13

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVaultDeployer.sol#L13

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/SyndicateFactory.sol#L13

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L15

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L18

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L21

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L24

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L27

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L30

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L33

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L36

[N-04] Specify visibility for all state variables

All state variables should have their visibility specified which improves readability and avoids misunderstandings.

There are 3 instances of state variables that don't have a visibility specified (the default is internal).

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L17

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVaultDeployer.sol#L13

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVaultDeployer.sol#L13

[N-05] Make public functions that are not called internally external

Functions that are not called internally should be declared external.

There is 1 instance of this.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LSDNFactory.sol#L73