LSD Network - Stakehouse contest - yixxas's results

Lines of code

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

Vulnerability details

Impact

Node runners can withdraw more than 4 ether from the associated smart wallet and I believe only 4 ether should be allowed to be refunded to the node runner.

Proof of Concept

Once this function is called once, bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet, which means that it can only be called once. However, a malicious user can reenter when ether is received as mapping is updated only after rawExecute. Note that even though the comment says "refund 4 ether from smart wallet to node runner's EOA", there is no check to verify that _recipient is not a smart contract address.

LiquidStakingManager.sol#L326-L350

    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);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Do the update of the mapping bannedBLSPublicKeys[_blsPublicKeyOfKnot] = associatedSmartWallet before transferring ether, or add the nonReentrant modifier to this function.

Lines of code

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

Vulnerability details

Impact

User may have withdrawn an amount that results in the remaining withdrawable amount to be less than the MIN_STAKING_AMOUNT which prevents them from doing withdrawETH(), especially when they do not have enough ether to "top up" to make the minimum withdraw.

Proof of Concept

withdrawETH() forces users to have at least 0.001 ether deposited to be able to withdraw.

require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount")

But if for example users deposits 4 ether, withdraws 3.9995 ether, has 0.0005 ether remaining, the user will not be able to make the withdraw.

Tools Used

Manual Review

Recommended Mitigation Steps

withdrawETH() should not require _amount >= MIN_STAKING_AMOUNT. Recommend this check to be removed.

Lines of code

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

Vulnerability details

Impact

Node runner can set a smart contract wallet as the smartWalletRepresentative[smartWallet] address. This breaks the underlying assumption of the protocol that this address must be an EOA, as frequently commented in the contract. This can cause or lead to many unintended effects.

Proof of Concept

In registerBLSPublicKeys(), the check is done with require(!Address.isContract(_eoaRepresentative), "Only EOA representative permitted"). However this check is not done in rotateEOARepresentative() hence a user can first register a key with an EOA account, then rotate it to a smart contract wallet with rotateEOARepresentative() to reach the mentioned state.

Tools Used

Manual Review

Recommended Mitigation Steps

The same check needs to be done in both rotateEOARepresentative() and rotateEOARepresentativeOfNodeRunner().

Lines of code

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

Vulnerability details

Impact

A user can be tricked to call withdrawLPTokens() with a malicious token address that is not part of the LPToken issued by the lptokenFactory which can potentially cause them to lose all their giant LP token.

Proof of Concept

withdrawLPTokens() does not check that _lpTokens is a token that is issued by lptokenFactory. This token can be one that is not registered as part of any BLS public key. A malicious user can use this fact and create a "fake" LPToken, send it into the contract and trick a user to call this function with this fake token as input. End result is that users will be transferred this fake token which has no value in the stakehouse ecosystem, but burn their valuable giant LP token.

Tools Used

Manual Review

Recommended Mitigation Steps

A check should be added in withdrawLPTokens() to ensure that tokens being used are created by the lptokenFactory.

Lines of code

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

Vulnerability details

Impact

The underlying assumption of eoaRepresentative being an EOA can be untrue. This can cause many unintended effects as the contract comments strongly suggests that this must be an EOA account.

Proof of Concept

When BLS public key is registered in registerBLSPublicKeys(), it has the check of

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

However, this check can be passed even though input is a smart contract if

1. Function is called in the constructor. Address.isContract() checks for the code length, but during construction code length is 0.
2. Smart contract that has not been deployed yet can be used. The CREATE2 opcode can be used to deterministically calculate the address of a smart contract before it is created. This means that the user can bypass this check by calling this function before deploying the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

It is generally not recommended to enforce an address to be only EOA and AFAIK, this is impossible to enforce due to the aforementioned cases. I recommend the protocol team to take a closer look at this and build the protocol with the assumption that _eoaRepresentative == EOA.

Lines of code

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

Vulnerability details

Impact

Node runners can have all their stake rewards taken by the DAO as commissions can be set to a 100%.

Proof of Concept

There is no limits on _updateDAORevenueCommission() except not exceeding MODULO, which means it can be set to a 100%.

LiquidStakingManager.sol#L948-L955

    function _updateDAORevenueCommission(uint256 _commissionPercentage) internal {
        require(_commissionPercentage <= MODULO, "Invalid commission");

        emit DAOCommissionUpdated(daoCommissionPercentage, _commissionPercentage);

        daoCommissionPercentage = _commissionPercentage;
    }

This percentage is used to calculate uint256 daoAmount = (_received * daoCommissionPercentage) / MODULO in _calculateCommission(). Remaining is then caculated with uint256 rest = _received - daoAmount, and in this case rest = 0. When node runner calls claimRewardsAsNodeRunner(), the node runner will receive 0 rewards.

Tools Used

Manual Review

Recommended Mitigation Steps

There should be maximum cap on how much commission DAO can take from node runners.