LSD Network - Stakehouse contest - delfin454000's results

QA Report: low risk

Unused/empty receive()/fallback() function

Leaving the receive() below without a revert could result in a loss of funds for a user who sends Ether to the contract (having no way to get anything back)

LiquidStakingManager.sol: L629

    receive() external payable {}

Similarly for the following:

SyndicateRewardsProcessor.sol: L98

OwnableSmartWallet.sol: L148-150

---

---

Missing checks for address(0x0) when assigning values to address state variables

---

Check for address(0x0) is missing for _pool:

GiantLP.sol: L25

        pool = _pool;

---

Check for address(0x0) is missing for _deployer:

LPToken.sol: L38

        deployer = _deployer;

---

---

Upgrade Open Zeppelin

An outdated Open Zeppelin version (4.5.0) is used.

The version used has known vulnerabilties, all of which have been patched by version 4.7.3. See Security Advisories.

---

---

QA Report: non-critical

Typos

---

SavETHVault.sol: L115

        require(numOfTokens == _amounts.length, "Inconsisent array length");

Change Inconsisent to Inconsistent

---

SavETHVault.sol: L227

    /// @notice Utility function that determins whether an LP can be burned for dETH if the associated derivatives have been minted

Change determins to determines

---

GiantMevAndFeesPool.sol: L103

    /// @param _oldLPTokens List of new savETH vault LP tokens that the vault wants to receive in exchange for moving ETH to a new KNOT

Change _oldLPTokens to _newLPTokens

---

LiquidStakingManager.sol: L101

    /// @notice address of optional gatekeeper for admiting new knots to the house created by the network

Change admiting to admitting

---

LiquidStakingManager.sol: L436

        require(_isNodeRunnerValid(msg.sender) == true, "Unrecognised node runner");

Change Unrecognised to Unrecognized, assuming American English. The in-scope contracts use mostly U.S. rather than British spellings. In any case, a choice of spelling version should be made for consistency.

---

LiquidStakingManager.sol: L476

            // register validtor initals for each of the KNOTs

Change validtor to validator and initals to initials

---

The same typo (overriden) occurs in both lines referenced below:

LiquidStakingManager.sol: L903

LiquidStakingManager.sol: L920

    /// @dev Something that can be overriden during testing

Change overriden to overridden in both cases

---

LiquidStakingManager.sol: L920

    /// @dev This can be overriden to customise fee percentages

Change customise to customize, again assuming American English

---

ETHPoolLPFactory.sol: L74

    /// @param _newLPToken Instane of the new LP token (to be minted)

Change Instane to Instance

---

The same typo (depoistor) occurs in both lines referenced below:

ETHPoolLPFactory.sol: L124

ETHPoolLPFactory.sol: L150

            // mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied

Change depoistor to depositor in both cases

---

---

Duplicate import

The import of LPToken occurs twice in GiantMevAndFeesPool.sol

GiantMevAndFeesPool.sol: L5-12

import { GiantLP } from "./GiantLP.sol";
import { StakingFundsVault } from "./StakingFundsVault.sol";
import { LPToken } from "./LPToken.sol";
import { GiantPoolBase } from "./GiantPoolBase.sol";
import { SyndicateRewardsProcessor } from "./SyndicateRewardsProcessor.sol";
import { LSDNFactory } from "./LSDNFactory.sol";
import { LPToken } from "./LPToken.sol";
import { ITransferHookProcessor } from "../interfaces/ITransferHookProcessor.sol";

Recommendation: Remove duplicate import

---

---

Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

---

GiantPoolBase.sol: L19

    event LPSwappedForVaultLP(address indexed vaultLPToken, address indexed sender, uint256 amount);

---

GiantSavETHVaultPool.sol: L16

    event LPBurnedForDETH(address indexed savETHVaultLPToken, address indexed sender, uint256 amount);

---

SavETHVault.sol: L22

    event ETHWithdrawnForStaking(address withdrawalAddress, address liquidStakingManager, uint256 amount);

---

SavETHVault.sol: L121

    event CurrentStamp(uint256 stamp, uint256 last, bool isConditionTrue);

---

StakingFundsVault.sol: L28

    event ETHWithdrawn(address receiver, address admin, uint256 amount);

---

StakingFundsVault.sol: L31

    event ERC20Recovered(address admin, address recipient, uint256 amount);

---

Syndicate.sol: L63

    event ETHClaimed(bytes BLSPubKey, address indexed user, address recipient, uint256 claim, bool indexed isCollateralizedClaim);

---

LiquidStakingManager.sol: L66

    event ETHWithdrawnFromSmartWallet(address indexed associatedSmartWallet, bytes blsPublicKeyOfKnot, address nodeRunner);

---

SyndicateRewardsProcessor.sol: L12

    event ETHDistributed(address indexed user, address indexed recipient, uint256 amount);

---

ETHPoolLPFactory.sol: L19

    event LPTokenBurnt(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);

---

ETHPoolLPFactory.sol: L22

    event NewLPTokenIssued(bytes blsPublicKeyOfKnot, address token, address firstDepositor, uint256 amount);

---

ETHPoolLPFactory.sol: L25

    event LPTokenMinted(bytes blsPublicKeyOfKnot, address token, address depositor, uint256 amount);

---

---

Inconsistent long comment wrap syntax reduces readability

Long comments are wrapped using inconsistent spacing after the /// or //. While the choice of spacing is up to the developers, it should be made constant throughout, as the suggestions below (which use 2 additional spaces) show:

---

OwnableSmartWalletFactory.sol: L17-19

    /// @notice Can be used to verify that the address is actually
    ///         OwnableSmartWallet and not an impersonating malicious
    ///         account

Suggestion:

    /// @notice Can be used to verify that the address is actually 
    ///   OwnableSmartWallet and not an impersonating malicious account

---

OwnableSmartWallet.sol: L19-21

    /// @dev A map from owner and spender to transfer approval. Determines whether
    ///      the spender can transfer this wallet from the owner. Can be used
    ///      to put this wallet in possession of a strategy (e.g., as collateral).

Suggestion:

    /// @dev A map from owner and spender to transfer approval. Determines whether 
    ///   the spender can transfer this wallet from the owner. Can be used
    ///   to put this wallet in possession of a strategy (e.g., as collateral).

---

SavETHVault.sol: L31-33

    /// @dev If dETH is not withdrawn, then for a non-existing dETH balance
    /// the structure would result in zero balance even though dETH isn't withdrawn for KNOT
    /// withdrawn parameter tracks the status of dETH for a KNOT

Suggestion:

    /// @dev If dETH is not withdrawn, then for a non-existing dETH balance the  
    ///   structure would result in zero balance even though dETH isn't withdrawn
    ///   since KNOT withdrawn parameter tracks the status of dETH for a KNOT

---

---

Long single-line comments

In theory, comments over 80 characters should wrap using multi-line comment syntax. Even if longer comments (up to 120 characters) are acceptable and a scroll bar is provided, very long comments can interfere with readability. As noted in the previous finding, many of the long comments in Stakehouse do wrap. Below are five of the remaining extra-long lines:

---

Syndicate.sol: L119

    /// @notice Once a BLS public key is no longer part of the syndicate, the accumulated ETH per free floating SLOT share is snapshotted so historical earnings can be drawn down correctly

Suggestion:

    /// @notice Once a BLS public key is no longer part of the syndicate,
    ///   the accumulated ETH per free floating SLOT share is snapshotted 
    ///   so historical earnings can be drawn down correctly.

---

LiquidStakingManager.sol: L222

    /// @notice In preparation of a rage quit, restore sETH to a smart wallet which are recoverable with the execution methods in the event this step does not go to plan

Suggestion:

    /// @notice In preparation of a rage quit, restore sETH to a smart wallet which are 
    ///   recoverable with the execution methods in the event this step does not go to plan.

---

SavETHVault.sol: L222

    /// @notice 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

Suggestion:

    /// @notice 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.

---

LSDNFactory.sol: L35

    /// @notice Address of the contract that can deploy new instances of optional gatekeepers for controlling which knots can join the LSDN house

Suggestion:

    /// @notice Address of the contract that can deploy new instances of optional  
    ///   gatekeepers for controlling which knots can join the LSDN house.

---

GiantSavETHVaultPool.sol: L65

    /// @param _amounts Amounts of giant LP the user owns which is burnt 1:1 with savETH vault LP and in turn that will give a share of dETH

Suggestion:

    /// @param _amounts Amounts of giant LP the user owns which is burnt 1:1  
    ///   with savETH vault LP and in turn that will give a share of dETH.

---

Similarly for the following extra-long (at least 140 characters) comment lines:

GiantSavETHVaultPool.sol: L111

SavETHVault.sol: L111

SavETHVault.sol: L217

Syndicate.sol: L35

Syndicate.sol: L95

Syndicate.sol: L116

LiquidStakingManager.sol: L933

---

---

Update sensitive terms in both comments and code

Terms incorporating "white," "black," "master" or "slave" are potentially problematic. Substituting more neutral terminology is becoming common practice

---

LiquidStakingManager.sol: L38-39

    /// @notice signalize updated whitelist status of node runner
    event NodeRunnerWhitelistingStatusChanged(address indexed nodeRunner, bool updatedStatus);

Suggestion: Change whitelist to allowlist and NodeRunnerWhitelistingStatusChanged to NodeRunnerAllowlistingStatusChanged

Similarly for the following instances of whitelist and its variants:

LiquidStakingManager.sol: L35-36

LiquidStakingManager.sol: L119-123

LiquidStakingManager.sol: L265-283

LiquidStakingManager.sol: L453

LiquidStakingManager.sol: L681

LiquidStakingManager.sol: L687

---

OwnableSmartWalletFactory.sol: L14

    address public immutable masterWallet;

Suggestion: Change masterWallet to primaryWallet

Similarly for the following instances of masterWallet:

OwnableSmartWalletFactory.sol: L23-25

OwnableSmartWalletFactory.sol: L39

---

---

Missing NatSpec

NatSpec is completely missing for the following constructor:

SavETHVaultDeployer.sol: L14-16

    constructor() {
        implementation = address(new SavETHVault());
    }

Natspec is also absent or mostly absent for the following constructors:

StakingFundsVaultDeployer.sol: L14-16

LSDNFactory.sol: L41-50

---

---

NatSpec is completely missing for the following event:

OptionalGatekeeperFactory.sol: L9

    event NewOptionalGatekeeperDeployed(address indexed keeper, address indexed manager);

Note that NatSpec for this event would be expected to include an @notice statement (or its equivalent) and @param statements for keeper and manager

NatSpec is also absent for the following events:

SavETHVaultDeployer.sol: L11

StakingFundsVaultDeployer.sol: L11

SavETHVault.sol: L121

---

---

@param statement is missing for the following event:

LPTokenFactory.sol: L11-12

    /// @notice Emitted when a new LP token instance is deployed
    event LPTokenDeployed(address indexed factoryCloneToken);

One or more @param statements are also missing for the following events:

GiantPoolBase.sol: L12-13

GiantPoolBase.sol: L15-16

GiantPoolBase.sol: L18-19

LSDNFactory.sol: L11-12

GiantSavETHVaultPool.sol: L15-16

SavETHVault.sol: L18-19

SavETHVault.sol: L21-22

StakingFundsVault.sol: L24-25

StakingFundsVault.sol: L27-28

StakingFundsVault.sol: L30-31

StakingFundsVault.sol: L33-34

Syndicate.sol: L41-42

Syndicate.sol: L44-45

Syndicate.sol: L47-48

Syndicate.sol: L50-51

Syndicate.sol: L53-54

Syndicate.sol: L56-57

Syndicate.sol: L59-60

Syndicate.sol: L62-63

LiquidStakingManager.sol: L35-36

LiquidStakingManager.sol: L38-39

LiquidStakingManager.sol: L41-42

LiquidStakingManager.sol: L44-45

LiquidStakingManager.sol: L47-48

LiquidStakingManager.sol: L50-51

LiquidStakingManager.sol: L53-54

LiquidStakingManager.sol: L56-57

LiquidStakingManager.sol: L59-60

LiquidStakingManager.sol: L62-63

LiquidStakingManager.sol: L65-66

LiquidStakingManager.sol: L68-69

LiquidStakingManager.sol: L71-72

LiquidStakingManager.sol: L74-75

LiquidStakingManager.sol: L77-78

LiquidStakingManager.sol: L80-81

LiquidStakingManager.sol: L83-84

LiquidStakingManager.sol: L86-87

SyndicateRewardsProcessor.sol: L8-9

SyndicateRewardsProcessor.sol: L11-12

ETHPoolLPFactory.sol: L15-16

ETHPoolLPFactory.sol: L18-19

ETHPoolLPFactory.sol: L21-22

ETHPoolLPFactory.sol: L24-25

---

---

NatSpec is completely missing for the following function:

OptionalGatekeeperFactory.sol: L11-16

    function deploy(address _liquidStakingManager) external returns (OptionalHouseGatekeeper) {
        OptionalHouseGatekeeper newKeeper = new OptionalHouseGatekeeper(_liquidStakingManager);

        emit NewOptionalGatekeeperDeployed(address(newKeeper), _liquidStakingManager);

        return newKeeper;
    }

Natspec is also absent for the public or external functions below (NatSpec is not required for functions marked private or internal):

SavETHVaultDeployer.sol: L18

StakingFundsVaultDeployer.sol: L18

OwnableSmartWalletFactory.sol: L28

OwnableSmartWalletFactory.sol: L32

OptionalGatekeeperFactory.sol: L11

GiantLP.sol: L29

GiantLP.sol: L34

SavETHVault.sol: L45

---

---

@param and @return statements are missing for the function below:

OptionalHouseGatekeeper.sol: L18-21

    /// @notice Method called by the house before admitting a new KNOT member and giving house sETH
    function isMemberPermitted(bytes calldata _blsPublicKeyOfKnot) external override view returns (bool) {
        return liquidStakingManager.isBLSPublicKeyPartOfLSDNetwork(_blsPublicKeyOfKnot);
    }

Similarly, one or more @param and/or @return statements are missing for the following functions:

LPTokenFactory.sol: L24-32

LPToken.sol: L30-37

LPToken.sol: L44-46

LPToken.sol: L50-51

LPToken.sol: L55-56

GiantPoolBase.sol: L33-34

LSDNFactory.sol: L70-78

SavETHVault.sol: L79-83

SavETHVault.sol: L217-218

SavETHVault.sol: L222-223

SavETHVault.sol: L227-228

GiantMevAndFeesPool.sol: L55-60

GiantMevAndFeesPool.sol: L81-86

GiantMevAndFeesPool.sol: L145-146

GiantMevAndFeesPool.sol: L169-170

GiantMevAndFeesPool.sol: L175-176

StakingFundsVault.sol: L45-46

StakingFundsVault.sol: L55-56

StakingFundsVault.sol: L110-113

StakingFundsVault.sol: L197-202

Syndicate.sol: L153-154

LiquidStakingManager.sol: L217-218

LiquidStakingManager.sol: L238-239

LiquidStakingManager.sol: L248-249

LiquidStakingManager.sol: L254-255

LiquidStakingManager.sol: L323-326

LiquidStakingManager.sol: L352-356

LiquidStakingManager.sol: L494-495

LiquidStakingManager.sol: L499-500

LiquidStakingManager.sol: L631-635

---

---