LSD Network - Stakehouse contest - datapunk's results

Lines of code

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

Vulnerability details

Impact

All Eth can be drained by fake vault addresses. https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L29

Proof of Concept

In batchDepositETHForStaking, _savETHVault is checked for its validity through

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

However, an attacker can create a fake contract that retuns a correct liquidStakingNetworkManager, thus passing the check easily. After the check, any ETH in the pool will be sent to an address this fake contract provide:

savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
    _blsPublicKeys[i],
    _stakeAmounts[i]
);

Tools Used

manual

Recommended Mitigation Steps

Always passing liquid staking manager address, checking its real and then requesting either the savETH vault or staking funds vault is a good idea rather than other way around from a giant pool perspective.

Lines of code

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

Vulnerability details

potential reentrancy risk in _distributeETHRewardsToUserForToken

Impact

Reentrancy risk can potentially be demaging

Proof of Concept

_distributeETHRewardsToUserForToken, which in turn calls (bool success, ) = _recipient.call{value: due}("");. As recipient of ETH, the msg.sender can potentially reenter. _distributeETHRewardsToUserForToken is being used in multiple places, such as batchDepositETHForStaking in SavETHVault beforeTokenTransfer in SavETHVault batchDepositETHForStaking in StakingFundsVault beforeTokenTransfer in StakingFundsVault

Tools Used

manual

Recommended Mitigation Steps

Careful examination and consider adding reentrancyGuard to above functions.

Lines of code

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

Vulnerability details

Impact

real LPTokens can be transferred out of GiantMevAndFeesPool through fake _stakingFundsVaults provided by an attacker. https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L126

Proof of Concept

bringUnusedETHBackIntoGiantPool takes in _stakingFundsVaults, _oldLPTokens, _newLPTokens and rotate _amounts from old to new tokens. The tokens are thoroughly verified by burnLPForETH in ETHPoolLPFactory. However, theres is no checking for the validity of _stakingFundsVaults, nor the relationship between LPTokens and _stakingFundsVaults. Therefore, an attacker can create fake contracts for _stakingFundsVaults, with burnLPTokensForETH, that takes LPTokens as parameters. The msg.sender in burnLPTokensForETH is GiantMevAndFeesPool, thus the attacker can transfer LPTokens that belongs to GiantMevAndFeesPool to any addresses it controls.

Tools Used

manual

Recommended Mitigation Steps

Always passing liquid staking manager address, checking its real and then requesting either the savETH vault or staking funds vault is a good idea to prove the validity of vaults.

Lines of code

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

Vulnerability details

Impact

GiantPoolBase keeps track of idleETH, which can be potentially wrong if Eth are sent directly into the contract not through depositETH. When that happens, idleETH is incorrect.

Proof of Concept

function depositETH(uint256 _amount) public payable {
    idleETH += msg.value;
}

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

Tools Used

manual

Recommended Mitigation Steps

add a receive function:

receive() external payable {
    depositETH(msg.sender);
}

Lines of code

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

Vulnerability details

Impact

User's lpTokenETH and _lpTokens will be out-of-sync https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#66

Proof of Concept

In withdrawDETH, vault and token are checked for their validity through

_assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

which, in turn, checks

require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
require(_token.balanceOf(address(this)) >= _amount, "Pool does not own specified LP");

and

require(vault.isDETHReadyForWithdrawal(address(token)), "dETH is not ready for withdrawal");

However, if the website gets hacked and when users interact with withdrawDETH, they might be supplying fake _savETHVaults and _lpTokens as specified by the attacker. These fake contracts can return anything needed to pass the above checks easily. After theses checks, users' legitimate lpTokenETH will be burned:

lpTokenETH.burn(msg.sender, amount); 

but only fake lpTokens will be burned: getDETH().transfer(msg.sender, dETHReceivedFromAllSavETHVaults);

Tools Used

manual

Recommended Mitigation Steps

Always passing liquid staking manager address, checking its real and then requesting either the savETH vault or staking funds vault is a good idea to prove the validity of vaults.

Judge has assessed an item in Issue #373 as M risk. The relevant finding follows:

N2. ETH not accumulated in previewAccumulatedETH https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L91 supposed to have accumulated += ... Although it is an external view function, depending on its usages, it may present more issues to the callers.

Lines of code

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

Vulnerability details

Impact

If trusted manager supplies a wrong address, the funds might be non-retrievable.

Proof of Concept

in withdrawETHForStaking, the manager can supply any _smartWallet to withdraw ETH, which the _smartWallet can be very easily checked against OwnableSmartWalletFactory, since it keeps track of them in mapping(address => bool) public walletExists; Despite the fact this function is only callable by the trusted manager, it is quite be a scary mistake if wrong address is used.

Tools Used

manual

Recommended Mitigation Steps

check supplied _smartWallet against walletExists from OwnableSmartWalletFactory

Lines of code

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

Vulnerability details

Impact

Deposits tolpToken can be made such as it will never reach 24 ether as required

Proof of Concept

Funds can be deposited into lpToken through either _depositETHForStaking or 'rotateLPTokens', both have: require(_amount >= MIN_STAKING_AMOUNT, "Min amount not reached"); Both also requires that the total deposit per lpToken does not exceed 24 ether: _depositETHForStaking: require(lpToken.totalSupply() + _amount <= maxStakingAmountPerValidator require(_amount <= maxStakingAmountPerValidator and rotateLPTokens: require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");.

An attacker can monitor deposits and front-running them to make the totalSupply in the range of (23.999, 24). In such cases, users would have to withdraw their ETH to correct.

Tools Used

manual

Recommended Mitigation Steps

in deposit and rotate, adding require(amount % MIN_STAKING_AMOUNT == 0); will prevent such attacks.

N1. dividing before multiplication

The way claims are calculated, it is dividing by _numOfShares first, then multiply by _balanceOfSende: uint256 newAccumulatedETH = accumulatedETHPerLPShare + ((unprocessed * PRECISION) / _numOfShares); ((newAccumulatedETH * _balanceOfSender) / PRECISION) https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#43 However, since PRECISION is present, I am not sure how much the impact is. Just fruit for thought to potentially refactor the formula

N2. ETH not accumulated in previewAccumulatedETH

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L91 supposed to have accumulated += ... Although it is an external view function, depending on its usages, it may present more issues to the callers.

N3. in _addPriorityStakers, duplicated stakers are pass the dup check

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L588 checks if (i > 0 && staker < _priorityStakers[i-1]) revert DuplicateArrayElements(); while it should check if (i > 0 && staker <= _priorityStakers[i-1]) revert DuplicateArrayElements();

N4. incorrect comment

require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether"); should be: require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must have 4 ether"); https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L940

N5. redundant calls to updateAccumulatedETHPerLP

in StakingFundsVault, the last thing _claimFundsFromSyndicateForDistribution does is to call updateAccumulatedETHPerLP, yet everywhere _claimFundsFromSyndicateForDistribution is called, it is followed by updateAccumulatedETHPerLP. The 2nd does nothing, so can be removed.

N6. unstake in Syndicate does not check for 0 amount.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/syndicate/Syndicate.sol#L259 There is no minimum value for _sETHAmount to be unstaked. When 0 amount is supplied, it should be checked and skipped early.

N7. not checking against address(0)

Add require(_pool != address(0)) to https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantLP.sol#L25

Add require(_liquidStakingManager != address(0)) to https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalGatekeeperFactory.sol#L12

Add require(_manager != address(0)) to https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/OptionalHouseGatekeeper.sol#L14

N8. wrong error message

if (_recipient == address(this)) revert ZeroAddress(); if (getSlotRegistry().currentSlashedAmountOfSLOTForKnot(blsPubKey) != 0) revert InvalidNumberOfCollateralizedOwners();

N9. make contract GiantPoolBase abstract