LSD Network - Stakehouse contest - pashov's results

Lines of code

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

Vulnerability details

Proof of Concept

The function that updates the daoCommissionPercentage has a check that the percentage is not more than 100%, but 100% would be a valid value with the current implementation. This is not good for the protocol though, because if a malicious or compromised DAO address sets it to 100% this will mean that on a claimRewardsAsNodeRunner() call, the recipient will get 0 rewards and the “dao” will get 100% of the reward amount, essentially rugging rewards.

Impact

The impact is all node runner rewards can be stolen by the “dao” but it requires a malicious or a compromised dao account

Recommendation

Add a sensible upper limit to the commission percentage, for example 5%.

Lines of code

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

Vulnerability details

Proof of Concept

The method responsible for setting a whitelist status of a node runner has the following check

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

which will always result in false because it is comparing the same values.

Impact

This results in the whole whitelisting functionality being impossible to use with the current state of the code.

Recommendation

Update the check to look like isNodeRunnerWhitelisted[_nodeRunner] != isWhitelisted

NC-01 Incomplete NatSpec docs in the codebase

1. GiantMevAndFeesPool::previewAccumulatedETH
2. LPTokenFactory::deployLPToken
3. LPToken - all external methods
4. GiantPoolBase::depositETH
5. LSDNFactory::deployNewLiquidStakingDerivativeNetwork
6. StakingFundsVault::claimRewards
7. Syndicate::claimAsCollateralizedSLOTOwner

NC-02 Use latest Solidity version

Codebase is currently using Solidity version 0.8.13 - it is a best practice to use latest Solidity version as you can get compiler optimisations and bugfixes.

NC-03 Do not use a floating pragma, use a concrete version instead

The codebase is using floating pragma (for example ***pragma*** solidity ^0.8.13;) - Always use a concrete version so you get the same bytecode on each compilation for certain.

NC-04 License comment should be on the first line in a smart contract file, not after the pragma statement

1. GiantMevAndFeesPool
2. OptionalHouseGatekeeper
3. SavETHVaultDeployer
4. LPTokenFactory
5. GiantLP
6. LPToken
7. SyndicateFactory
8. GiantPoolBase
9. LSDNFactory
10. SyndicateErrors
11. GiantSavETHVaultPool
12. Syndicate

NC-05 Codebase is not using latest OpenZeppelin library version but one with vulnerabilities

Here you can see all security advisories for the OpenZeppelin library and you can see that the version used in the codebase

"@openzeppelin/contracts": "^4.5.0",
    "@openzeppelin/contracts-upgradeable": "4.5.0",

is not entirely safe. The contracts that are vulnerable are not used, but this is still a best practice for security - use latest version that has all security patches.

NC-06 Code casts address to address payable even though it is not needed

1. StakingFundsVaultDeployer
2. StakingFundsVault
3. StakingFundsVault
4. StakingFundsVault
5. LiquidStakingManager
6. LiquidStakingManager
7. LiquidStakingManager
8. LiquidStakingManager

NC-07 Wrong error messages in require statements

OwnableSmartWalletFactory require(owner !**=** address(*0*), 'Wallet cannot be address 0'); - should be Owner cannot be address 0

LPToken require(**msg.sender** **==** deployer, "Only savETH vault"); should be Only Deployer

SavETHVault require(address(**this**).balance **>=** _amount, "Insufficient withdrawal amount"); should be Insufficient balance in vault to withdraw

NC-08 Typos in the code

Allows a LP token owner to burn their tokens → Allows an LP token owner to burn their tokens

depoistor → depositor

NC-09 No need to do == false or == true on boolean expressions

There are 20+ occurrences of == false or == true in the code - just use !boolexpr for == false and boolexpr for == true

NC-10 Open TODO in the code

• https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/syndicate/Syndicate.sol#L195

Remove the TODO comment or resolve it

NC-11 Use type(uint256).max instead of 2 ** 256 - 1

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

Use the built-in type(uint256).max

NC-12 Not used event (dead code)

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

The event CurrentStamp is not used and is also in the middle of the smart contract. Remove it

NC-13 Wrong parameter name in function burn()

Both GiantLP::burn and LPToken::burn have a _recipient named parameter. There is no recipient in a “burn” so rename parameter to “account” or “burnFrom”.

L-01 Missing non-zero address checks in constructor or init method

1. GiantMevAndFeesPool
2. OptionalHouseGatekeeper
3. GiantLP
4. LPToken
5. GiantSavETHVaultPool

L-02 Implementation contracts are not initialised in constructors

This should not be an issue since those contracts are only used to be cloned, but it is still a good practice to initialise them yourself

1. StakingFundsVaultDeployer
2. SavETHVaultDeployer
3. OwnableSmartWalletFactory

L-03 Functions that send ether with a low level call or do ERC20::transfer cals are missing nonReentrant modifier

1. GiantPoolBase::withdrawLPTokens
2. StakingFundsVault::batchDepositETHForStaking
3. StakingFundsVault::depositETHForStaking
4. LiquidStakingManager::withdrawETHForKnot

L-04 Use two-step ownership transfer pattern

OwnableSmartWallet inherits from OZ’s Ownable and also LiquidStakingManager has the updateDAOAddress functionality. Both are doing them in a one-step way, consider using a two-step ownership transfer pattern.