LSD Network - Stakehouse contest - rbserver's results

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L113-L143 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L62-L64 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L76-L90 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L93-L95 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L199-L233 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L360-L368

Vulnerability details

Impact

After the derivatives are minted for the first BLS public key registered for the syndicate, the Staking Funds vault's LP holder can claim the corresponding EIP1559 rewards received by the syndicate. However, after the derivatives are minted for a new BLS public key that is registered after the first one for the syndicate, the Staking Funds vault's LP holder is unable to claim any new EIP1559 rewards received by the syndicate.

When the StakingFundsVault.depositETHForStaking function is called for the first BLS public key, executing updateAccumulatedETHPerLP() would not update any states since totalShares is 0 at that moment. Yet, when StakingFundsVault.depositETHForStaking function is called for the new BLS public key, totalShares is already a positive number, and executing updateAccumulatedETHPerLP() would eventually execute totalETHSeen = received, where received is address(this).balance + totalClaimed returned by totalRewardsReceived(); at that moment, address(this).balance includes the ETH amount deposted for staking, which is not the rewards, so totalETHSeen also includes such ETH amount that is deposited for staking and not the rewards. Later, when the Staking Funds vault's LP holder calls the StakingFundsVault.claimRewards function, which further calls the _claimFundsFromSyndicateForDistribution function that also executes updateAccumulatedETHPerLP(), executing received - totalETHSeen can underflow and revert because totalETHSeen includes the ETH amount that was deposited for staking and not the rewards while received includes the rewards and not the amount deposited for staking, and the amount deposited for staking can be larger than the rewards. Since calling the StakingFundsVault.claimRewards function reverts, the Staking Funds vault's LP holder is unable to receive and essentially loses the EIP1559 rewards that she or he deserves.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L113-L143

    function depositETHForStaking(bytes calldata _blsPublicKeyOfKnot, uint256 _amount) public payable returns (uint256) {
        require(liquidStakingNetworkManager.isBLSPublicKeyBanned(_blsPublicKeyOfKnot) == false, "BLS public key is banned or not a part of LSD network");
        require(
            getAccountManager().blsPublicKeyToLifecycleStatus(_blsPublicKeyOfKnot) == IDataStructures.LifecycleStatus.INITIALS_REGISTERED,
            "Lifecycle status must be one"
        );

        require(msg.value == _amount, "Must provide correct amount of ETH");

        // Update accrued ETH to contract per LP
        updateAccumulatedETHPerLP();

        ...
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L62-L64

    function updateAccumulatedETHPerLP() public {
        _updateAccumulatedETHPerLP(totalShares);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L76-L90

    function _updateAccumulatedETHPerLP(uint256 _numOfShares) internal {
        if (_numOfShares > 0) {
            uint256 received = totalRewardsReceived();
            uint256 unprocessed = received - totalETHSeen;

            if (unprocessed > 0) {
                emit ETHReceived(unprocessed);

                // accumulated ETH per minted share is scaled to avoid precision loss. it is scaled down later
                accumulatedETHPerLPShare += (unprocessed * PRECISION) / _numOfShares;

                totalETHSeen = received;
            }
        }
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L93-L95

    function totalRewardsReceived() public virtual view returns (uint256) {
        return address(this).balance + totalClaimed;
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L199-L233

    function claimRewards(
        address _recipient,
        bytes[] calldata _blsPubKeys
    ) external nonReentrant {
        for (uint256 i; i < _blsPubKeys.length; ++i) {
            ...

            if (i == 0 && !Syndicate(payable(liquidStakingNetworkManager.syndicate())).isNoLongerPartOfSyndicate(_blsPubKeys[i])) {
                // Withdraw any ETH accrued on free floating SLOT from syndicate to this contract
                // If a partial list of BLS keys that have free floating staked are supplied, then partial funds accrued will be fetched
                _claimFundsFromSyndicateForDistribution(
                    liquidStakingNetworkManager.syndicate(),
                    _blsPubKeys
                );

                // Distribute ETH per LP
                updateAccumulatedETHPerLP();
            }

            ...
        }
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L360-L368

    function _claimFundsFromSyndicateForDistribution(address _syndicate, bytes[] memory _blsPubKeys) internal {
        require(_syndicate != address(0), "Invalid configuration");

        // Claim all of the ETH due from the syndicate for the auto-staked sETH
        Syndicate syndicateContract = Syndicate(payable(_syndicate));
        syndicateContract.claimAsStaker(address(this), _blsPubKeys);

        updateAccumulatedETHPerLP();
    }

Proof of Concept

Please add the following code in test\foundry\LiquidStakingManager.t.sol.

1. Import stdError as follows.

import { stdError } from "forge-std/Test.sol";

2. Add the following test. This test will pass to demonstrate the described scenario.

    function testStakingFundsVaultLPHolderCannotClaimRewardsAfterDerivativesAreMintedForNewBLSPublicKey() public {
        // set up users and ETH
        address nodeRunner = accountOne; vm.deal(nodeRunner, 4 ether);
        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);

        // do everything from funding a validator within default LSDN to minting derivatives
        depositStakeAndMintDerivativesForDefaultNetwork(
            nodeRunner,
            feesAndMevUser,
            savETHUser,
            blsPubKeyFour
        );

        // send the syndicate some EIP1559 rewards
        uint256 eip1559Tips = 0.6743 ether;
        (bool success, ) = manager.syndicate().call{value: eip1559Tips}("");
        assertEq(success, true);

        vm.warp(block.timestamp + 3 hours);

        // feesAndMevUser, who is the Staking Funds vault's LP holder, can claim rewards accrued up to the point of pulling the plug
        vm.startPrank(feesAndMevUser);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyFour));
        vm.stopPrank();

        uint256 feesAndMevUserEthBalanceBefore = feesAndMevUser.balance;
        assertEq(feesAndMevUserEthBalanceBefore, (eip1559Tips / 2) - 1);

        vm.warp(block.timestamp + 3 hours);

        vm.deal(nodeRunner, 4 ether);
        vm.deal(feesAndMevUser, 4 ether);
        vm.deal(savETHUser, 24 ether);

        // For a different BLS public key, which is blsPubKeyTwo, 
        //   do everything from funding a validator within default LSDN to minting derivatives.
        depositStakeAndMintDerivativesForDefaultNetwork(
            nodeRunner,
            feesAndMevUser,
            savETHUser,
            blsPubKeyTwo
        );

        // more EIP1559 rewards are received by the syndicate after derivatives are minted for blsPubKeyTwo
        (success, ) = manager.syndicate().call{value: eip1559Tips}("");
        assertEq(success, true);

        vm.warp(block.timestamp + 3 hours);

        vm.startPrank(feesAndMevUser);

        // calling the claimRewards function for blsPubKeyTwo by feesAndMevUser reverts at this moment
        vm.expectRevert(stdError.arithmeticError);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyTwo));

        // calling the claimRewards function for blsPubKeyFour by feesAndMevUser also reverts
        vm.expectRevert(stdError.arithmeticError);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyFour));

        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

The totalRewardsReceived function, which is called when calling the _updateAccumulatedETHPerLP function, can be updated to exclude the part of the address(this).balance that is the deposited amount for staking and not the EIP1559 rewards.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L218-L220 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L154-L157 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L597-L607 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L610-L627 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L174-L197

Vulnerability details

Impact

When the deRegisterKnotFromSyndicate function is called by the DAO, the _deRegisterKnot function is eventually called to execute numberOfRegisteredKnots -= 1. It is possible that numberOfRegisteredKnots is reduced to 0. During the period when the syndicate has no registered knots, the EIP1559 rewards that are received by the syndicate remain in the syndicate since functions like updateAccruedETHPerShares do not include any logics for handling such rewards received by the syndicate. Later, when a new knot is registered and mints the derivatives, the node runner can call the claimRewardsAsNodeRunner function to receive half ot these rewards received by the syndicate during the period when it has no registered knots. Yet, because such rewards are received by the syndicate before the new knot mints the derivatives, the node runner should not be entitled to these rewards. Moreover, due to the issue mentioned in my other finding titled "Staking Funds vault's LP holder cannot claim EIP1559 rewards after derivatives are minted for a new BLS public key that is not the first BLS public key registered for syndicate", calling the StakingFundsVault.claimRewards function by the Staking Funds vault's LP holder reverts so the other half of such rewards is locked in the syndicate. Even if calling the StakingFundsVault.claimRewards function by the Staking Funds vault's LP holder does not revert, the Staking Funds vault's LP holder does not deserve the other half of such rewards because these rewards are received by the syndicate before the new knot mints the derivatives. Because these EIP1559 rewards received by the syndicate during the period when it has no registered knots can be unfairly sent to the node runner or remain locked in the syndicate, such rewards are lost.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L218-L220

    function deRegisterKnotFromSyndicate(bytes[] calldata _blsPublicKeys) external onlyDAO {
        Syndicate(payable(syndicate)).deRegisterKnots(_blsPublicKeys);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L154-L157

    function deRegisterKnots(bytes[] calldata _blsPublicKeys) external onlyOwner {
        updateAccruedETHPerShares();
        _deRegisterKnots(_blsPublicKeys);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L597-L607

    function _deRegisterKnots(bytes[] calldata _blsPublicKeys) internal {
        for (uint256 i; i < _blsPublicKeys.length; ++i) {
            bytes memory blsPublicKey = _blsPublicKeys[i];

            // Do one final snapshot of ETH owed to the collateralized SLOT owners so they can claim later
            _updateCollateralizedSlotOwnersLiabilitySnapshot(blsPublicKey);

            // Execute the business logic for de-registering the single knot
            _deRegisterKnot(blsPublicKey);
        }
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L610-L627

    function _deRegisterKnot(bytes memory _blsPublicKey) internal {
        if (isKnotRegistered[_blsPublicKey] == false) revert KnotIsNotRegisteredWithSyndicate();
        if (isNoLongerPartOfSyndicate[_blsPublicKey] == true) revert KnotHasAlreadyBeenDeRegistered();

        // We flag that the knot is no longer part of the syndicate
        isNoLongerPartOfSyndicate[_blsPublicKey] = true;

        // For the free floating and collateralized SLOT of the knot, snapshot the accumulated ETH per share
        lastAccumulatedETHPerFreeFloatingShare[_blsPublicKey] = accumulatedETHPerFreeFloatingShare;

        // We need to reduce `totalFreeFloatingShares` in order to avoid further ETH accruing to shares of de-registered knot
        totalFreeFloatingShares -= sETHTotalStakeForKnot[_blsPublicKey];

        // Total number of registered knots with the syndicate reduces by one
        numberOfRegisteredKnots -= 1;

        emit KnotDeRegistered(_blsPublicKey);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L174-L197

    function updateAccruedETHPerShares() public {
        ...
        if (numberOfRegisteredKnots > 0) {
            ...
        } else {
            // todo - check else case for any ETH lost
        }
    }

Proof of Concept

Please add the following code in test\foundry\LiquidStakingManager.t.sol.

1. Import stdError as follows.

import { stdError } from "forge-std/Test.sol";

2. Add the following test. This test will pass to demonstrate the described scenario.

    function testEIP1559RewardsReceivedBySyndicateDuringPeriodWhenItHasNoRegisteredKnotsCanBeLost() public {
        // set up users and ETH
        address nodeRunner = accountOne; vm.deal(nodeRunner, 4 ether);
        address feesAndMevUser = accountTwo; vm.deal(feesAndMevUser, 4 ether);
        address savETHUser = accountThree; vm.deal(savETHUser, 24 ether);

        // do everything from funding a validator within default LSDN to minting derivatives
        depositStakeAndMintDerivativesForDefaultNetwork(
            nodeRunner,
            feesAndMevUser,
            savETHUser,
            blsPubKeyFour
        );

        // send the syndicate some EIP1559 rewards
        uint256 eip1559Tips = 0.6743 ether;
        (bool success, ) = manager.syndicate().call{value: eip1559Tips}("");
        assertEq(success, true);

        // de-register the only knot from the syndicate to send sETH back to the smart wallet
        IERC20 sETH = IERC20(MockSlotRegistry(factory.slot()).stakeHouseShareTokens(manager.stakehouse()));
        uint256 sETHBalanceBefore = sETH.balanceOf(manager.smartWalletOfNodeRunner(nodeRunner));
        vm.startPrank(admin);
        manager.deRegisterKnotFromSyndicate(getBytesArrayFromBytes(blsPubKeyFour));
        manager.restoreFreeFloatingSharesToSmartWalletForRageQuit(
            manager.smartWalletOfNodeRunner(nodeRunner),
            getBytesArrayFromBytes(blsPubKeyFour),
            getUint256ArrayFromValues(12 ether)
        );
        vm.stopPrank();

        assertEq(
            sETH.balanceOf(manager.smartWalletOfNodeRunner(nodeRunner)) - sETHBalanceBefore,
            12 ether
        );

        vm.warp(block.timestamp + 3 hours);

        // feesAndMevUser, who is the Staking Funds vault's LP holder, can claim rewards accrued up to the point of pulling the plug
        vm.startPrank(feesAndMevUser);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyFour));
        vm.stopPrank();

        uint256 feesAndMevUserEthBalanceBefore = feesAndMevUser.balance;
        assertEq(feesAndMevUserEthBalanceBefore, (eip1559Tips / 2) - 1);

        // nodeRunner, who is the collateralized SLOT holder for blsPubKeyFour, can claim rewards accrued up to the point of pulling the plug
        vm.startPrank(nodeRunner);
        manager.claimRewardsAsNodeRunner(nodeRunner, getBytesArrayFromBytes(blsPubKeyFour));
        vm.stopPrank();
        assertEq(nodeRunner.balance, (eip1559Tips / 2));

        // more EIP1559 rewards are sent to the syndicate, which has no registered knot at this moment        
        (success, ) = manager.syndicate().call{value: eip1559Tips}("");
        assertEq(success, true);

        vm.warp(block.timestamp + 3 hours);

        // calling the claimRewards function by feesAndMevUser has no effect at this moment
        vm.startPrank(feesAndMevUser);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyFour));
        vm.stopPrank();
        assertEq(feesAndMevUser.balance, feesAndMevUserEthBalanceBefore);

        // calling the claimRewardsAsNodeRunner function by nodeRunner reverts at this moment
        vm.startPrank(nodeRunner);
        vm.expectRevert("Nothing received");
        manager.claimRewardsAsNodeRunner(nodeRunner, getBytesArrayFromBytes(blsPubKeyFour));
        vm.stopPrank();

        // however, the syndicate still holds the EIP1559 rewards received by it during the period when the only knot was de-registered
        assertEq(manager.syndicate().balance, eip1559Tips + 1);

        vm.warp(block.timestamp + 3 hours);

        vm.deal(nodeRunner, 4 ether);
        vm.deal(feesAndMevUser, 4 ether);
        vm.deal(savETHUser, 24 ether);

        // For a different BLS public key, which is blsPubKeyTwo, 
        //   do everything from funding a validator within default LSDN to minting derivatives.
        depositStakeAndMintDerivativesForDefaultNetwork(
            nodeRunner,
            feesAndMevUser,
            savETHUser,
            blsPubKeyTwo
        );

        // calling the claimRewards function by feesAndMevUser reverts at this moment
        vm.startPrank(feesAndMevUser);
        vm.expectRevert(stdError.arithmeticError);
        stakingFundsVault.claimRewards(feesAndMevUser, getBytesArrayFromBytes(blsPubKeyTwo));
        vm.stopPrank();

        // Yet, calling the claimRewardsAsNodeRunner function by nodeRunner receives half of the EIP1559 rewards
        //   received by the syndicate during the period when it has no registered knots.
        // Because such rewards are not received by the syndicate after the derivatives are minted for blsPubKeyTwo,
        //   nodeRunner does not deserve these for blsPubKeyTwo. 
        vm.startPrank(nodeRunner);
        manager.claimRewardsAsNodeRunner(nodeRunner, getBytesArrayFromBytes(blsPubKeyTwo));
        vm.stopPrank();
        assertEq(nodeRunner.balance, eip1559Tips / 2);

        // Still, half of the EIP1559 rewards that were received by the syndicate
        //   during the period when the syndicate has no registered knots is locked in the syndicate.
        assertEq(manager.syndicate().balance, eip1559Tips / 2 + 1);
    }

Tools Used

VSCode

Recommended Mitigation Steps

The else block of the updateAccruedETHPerShares function (https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/syndicate/Syndicate.sol#L194-L196) can be updated to include logics that handle the EIP1559 rewards received by the syndicate during the period when it has no registered knots.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L202-L215 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/smart-wallet/OwnableSmartWallet.sol#L52-L64

Vulnerability details

Impact

Calling the executeAsSmartWallet function by the DAO further calls the OwnableSmartWallet.execute function. Since the executeAsSmartWallet function is payable, an ETH amount can be sent when calling it. However, since the sent ETH amount is not forwarded to the smart wallet contract, such sent amount can become locked in the LiquidStakingManager contract. For example, when the DAO attempts to call the executeAsSmartWallet function for sending some ETH to the smart wallet so the smart wallet can use it when calling its execute function, if the smart wallet's ETH balance is also higher than this sent ETH amount, calling the executeAsSmartWallet function would not revert, and the sent ETH amount is locked in the LiquidStakingManager contract while such amount is deducted from the smart wallet's ETH balance for being sent to the target address. Besides that this is against the intention of the DAO, the DAO loses the sent ETH amount that becomes locked in the LiquidStakingManager contract, and the node runner loses the amount that is unexpectedly deducted from the corresponding smart wallet's ETH balance.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L202-L215

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

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/smart-wallet/OwnableSmartWallet.sol#L52-L64

    function execute(
        address target,
        bytes memory callData,
        uint256 value
    )
        external
        override
        payable
        onlyOwner // F: [OSW-6A]
        returns (bytes memory)
    {
        return target.functionCallWithValue(callData, value); // F: [OSW-6]
    }

Proof of Concept

Please add the following code in test\foundry\LSDNFactory.t.sol.

1. Add the following receive function for the POC purpose.

    receive() external payable {}

2. Add the following test. This test will pass to demonstrate the described scenario.

    function testETHSentWhenCallingExecuteAsSmartWalletFunctionCanBeLost() public {
        vm.prank(address(factory));
        manager.updateDAOAddress(admin);

        uint256 nodeStakeAmount = 4 ether;
        address nodeRunner = accountOne;
        vm.deal(nodeRunner, nodeStakeAmount);

        address eoaRepresentative = accountTwo;

        vm.prank(nodeRunner);
        manager.registerBLSPublicKeys{value: nodeStakeAmount}(
            getBytesArrayFromBytes(blsPubKeyOne),
            getBytesArrayFromBytes(blsPubKeyOne),
            eoaRepresentative
        );

        // Before the executeAsSmartWallet function is called, the manager contract owns 0 ETH,
        //   and nodeRunner's smart wallet owns 4 ETH. 
        assertEq(address(manager).balance, 0);
        assertEq(manager.smartWalletOfNodeRunner(nodeRunner).balance, 4 ether);

        uint256 amount = 1.5 ether;

        vm.deal(admin, amount);

        vm.startPrank(admin);

        // admin, who is dao at this moment, calls the executeAsSmartWallet function while sending 1.5 ETH
        manager.executeAsSmartWallet{value: amount}(nodeRunner, address(this), bytes(""), amount);

        vm.stopPrank();

        // Although admin attempts to send the 1.5 ETH through calling the executeAsSmartWallet function,
        //   the sent 1.5 ETH was not transferred to nodeRunner's smart wallet but is locked in the manager contract instead.
        assertEq(address(manager).balance, amount);

        // Because nodeRunner's smart wallet owns more than 1.5 ETH, 1.5 ETH of this smart wallet's ETH balance is actually sent to address(this).
        assertEq(manager.smartWalletOfNodeRunner(nodeRunner).balance, 4 ether - amount);
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L210-L214 can be updated to the following code.

        IOwnableSmartWallet(smartWallet).execute{value: msg.value}(
            _to,
            _data,
            _value
        );

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L278-L284 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L684-L692 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L426-L492

Vulnerability details

Impact

Calling the updateNodeRunnerWhitelistStatus function by the DAO supposes to allow the trusted node runners to use and interact with the protocol when enableWhitelisting is set to true. However, since calling the updateNodeRunnerWhitelistStatus function executes require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status"), which always reverts, the DAO is unable to whitelist any trusted node runners. Because none of them can be whitelisted, all trusted node runners cannot call functions like registerBLSPublicKeys when the whitelisting mode is enabled. As the major functionalities become unavailable, the protocol's usability becomes much limited, and the user experience becomes much degraded.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L278-L284

    function updateNodeRunnerWhitelistStatus(address _nodeRunner, bool isWhitelisted) external onlyDAO {
        require(_nodeRunner != address(0), "Zero address");
        require(isNodeRunnerWhitelisted[_nodeRunner] != isNodeRunnerWhitelisted[_nodeRunner], "Unnecessary update to same status");

        isNodeRunnerWhitelisted[_nodeRunner] = isWhitelisted;
        emit NodeRunnerWhitelistingStatusChanged(_nodeRunner, isWhitelisted);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L684-L692

    function _isNodeRunnerValid(address _nodeRunner) internal view returns (bool) {
        require(_nodeRunner != address(0), "Zero address");

        if(enableWhitelisting) {
            require(isNodeRunnerWhitelisted[_nodeRunner] == true, "Invalid node runner");
        }

        return true;
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L426-L492

    function registerBLSPublicKeys(
        bytes[] calldata _blsPublicKeys,
        bytes[] calldata _blsSignatures,
        address _eoaRepresentative
    ) external payable nonReentrant {
        ...
        require(_isNodeRunnerValid(msg.sender) == true, "Unrecognised node runner");
        ...
    }

Proof of Concept

Please add the following test in test\foundry\LSDNFactory.t.sol. This test will pass to demonstrate the described scenario.

    function testCallingUpdateNodeRunnerWhitelistStatusFunctionAlwaysReverts() public {
        vm.prank(address(factory));
        manager.updateDAOAddress(admin);

        vm.startPrank(admin);

        vm.expectRevert("Unnecessary update to same status");
        manager.updateNodeRunnerWhitelistStatus(accountOne, true);

        vm.expectRevert("Unnecessary update to same status");
        manager.updateNodeRunnerWhitelistStatus(accountTwo, false);

        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L280 can be updated to the following code.

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

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356-L377 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L507-L509 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L426-L492

Vulnerability details

Impact

Currently, the rotateNodeRunnerOfSmartWallet function provides the only way to set bannedNodeRunners to true for a malicious node runner. However, before the node runner calls the registerBLSPublicKeys function to create a smart wallet, calling the rotateNodeRunnerOfSmartWallet function reverts. This means that for a node runner, who is already known to be malicious such as someone controlling a hacker address, calling the isNodeRunnerBanned function always return false before the registerBLSPublicKeys function is called for the first time, and executing require(isNodeRunnerBanned(msg.sender) == false, "Node runner is banned from LSD network") when calling the registerBLSPublicKeys function for the first time is not effective. As the monitoring burden can be high, the malicious node runner could interact with the protocol maliciously for a while already after the registerBLSPublicKeys function is called until the DAO notices the malicious activities and then calls the rotateNodeRunnerOfSmartWallet function. When the DAO does not react promptly, some damages to the protocol could be done already.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356-L377

    function rotateNodeRunnerOfSmartWallet(address _current, address _new, bool _wasPreviousNodeRunnerMalicious) external {
        ...

        if (msg.sender == dao && _wasPreviousNodeRunnerMalicious) {
            bannedNodeRunners[_current] = true;
            emit NodeRunnerBanned(_current);
        }

        ...
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L507-L509

    function isNodeRunnerBanned(address _nodeRunner) public view returns (bool) {
        return bannedNodeRunners[_nodeRunner];
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L426-L492

    function registerBLSPublicKeys(
        bytes[] calldata _blsPublicKeys,
        bytes[] calldata _blsSignatures,
        address _eoaRepresentative
    ) external payable nonReentrant {
        ...
        require(isNodeRunnerBanned(msg.sender) == false, "Node runner is banned from LSD network");

        address smartWallet = smartWalletOfNodeRunner[msg.sender];

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

            // associate node runner with the newly created wallet
            smartWalletOfNodeRunner[msg.sender] = smartWallet;
            nodeRunnerOfSmartWallet[smartWallet] = msg.sender;

            _authorizeRepresentative(smartWallet, _eoaRepresentative, true);
        }

        ...
    }

Proof of Concept

Please add the following test in test\foundry\LSDNFactory.t.sol. This test will pass to demonstrate the described scenario.

    function testMaliciousNodeRunnerCannotBeBannedBeforeCorrespondingSmartWalletIsCreated() public {
        vm.prank(address(factory));
        manager.updateDAOAddress(admin);

        // Simulate a situation where accountOne is known to be malicious already.
        // accountOne is not banned at this moment.
        assertEq(manager.bannedNodeRunners(accountOne), false);

        // Calling the rotateNodeRunnerOfSmartWallet function is the only way to ban accountOne;
        //   however, calling it reverts because accountOne has not called the registerBLSPublicKeys function to create a smart wallet yet.
        // This means that it is not possible to prevent accountOne from interacting with the protocol until her or his smart wallet is created.
        vm.prank(admin);
        vm.expectRevert("Wallet does not exist");
        manager.rotateNodeRunnerOfSmartWallet(accountOne, accountTwo, true);
    }

Tools Used

VSCode

Recommended Mitigation Steps

A function, which should be only callable by the DAO, that can directly set bannedNodeRunners for a node runner can be added.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LSDNFactory.sol#L73-L102 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L239-L246 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L308-L321 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356-L377 https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L326-L350

Vulnerability details

Impact

When calling the deployNewLiquidStakingDerivativeNetwork function, _dao is not required to be an address that corresponds to a governance contract. This is also confirmed by the code walkthrough at https://www.youtube.com/watch?v=7UHDUA9l6Ek&t=650s, which mentions that _dao can correspond to an address of a single user. Especially when the DAO is set to be an EOA address, it is possible that its private key becomes compromised. Moreover, because the updateDAOAddress function lacks a two step procedure for transferring the DAO's role, it is possible that the DAO is set to an uncontrolled address, which can be malicious. When the DAO becomes compromised or malicious, the actions of the node runners, who are not malicious, can be restricted at the DAO's will, such as by calling functions like rotateEOARepresentativeOfNodeRunner and rotateNodeRunnerOfSmartWallet. For example, a compromised DAO can call the rotateNodeRunnerOfSmartWallet function to transfer a smart wallet from a node runner, who is not malicious at all, to a colluded party. Afterwards, the affected node runner is banned from many interactions with the protocol and can no longer call, for instance, the withdrawETHForKnot function for withdrawing ETH from the corresponding smart wallet. Hence, a compromised or malicious DAO can cause severe consequences, including ETH losses.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LSDNFactory.sol#L73-L102

    function deployNewLiquidStakingDerivativeNetwork(
        address _dao,
        uint256 _optionalCommission,
        bool _deployOptionalHouseGatekeeper,
        string calldata _stakehouseTicker
    ) public returns (address) {

        // Clone a new liquid staking manager instance
        address newInstance = Clones.clone(liquidStakingManagerImplementation);
        ILiquidStakingManager(newInstance).init(
            _dao,
            ...
        );

        ...
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L239-L246

    function updateDAOAddress(address _newAddress) external onlyDAO {
        require(_newAddress != address(0), "Zero address");
        require(_newAddress != dao, "Same address");

        emit UpdateDAOAddress(dao, _newAddress);

        dao = _newAddress;
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L308-L321

    function rotateEOARepresentativeOfNodeRunner(address _nodeRunner, address _newRepresentative) external onlyDAO {
        require(_newRepresentative != address(0), "Zero address");

        address smartWallet = smartWalletOfNodeRunner[_nodeRunner];
        require(smartWallet != address(0), "No smart wallet");
        require(stakedKnotsOfSmartWallet[smartWallet] == 0, "Not all KNOTs are minted");
        require(smartWalletRepresentative[smartWallet] != _newRepresentative, "Invalid rotation to same EOA");

        // unauthorize old representative
        _authorizeRepresentative(smartWallet, smartWalletRepresentative[smartWallet], false);

        // authorize new representative
        _authorizeRepresentative(smartWallet, _newRepresentative, true);
    }

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L356-L377

    function rotateNodeRunnerOfSmartWallet(address _current, address _new, bool _wasPreviousNodeRunnerMalicious) external {
        require(_new != address(0) && _current != _new, "New is zero or current");

        address wallet = smartWalletOfNodeRunner[_current];
        require(wallet != address(0), "Wallet does not exist");
        require(_current == msg.sender || dao == msg.sender, "Not current owner or DAO");

        address newRunnerCurrentWallet = smartWalletOfNodeRunner[_new];
        require(newRunnerCurrentWallet == address(0), "New runner has a wallet");

        smartWalletOfNodeRunner[_new] = wallet;
        nodeRunnerOfSmartWallet[wallet] = _new;

        delete smartWalletOfNodeRunner[_current];

        if (msg.sender == dao && _wasPreviousNodeRunnerMalicious) {
            bannedNodeRunners[_current] = true;
            emit NodeRunnerBanned(_current);
        }

        emit NodeRunnerOfSmartWalletRotated(wallet, _current, _new);
    }

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

    function withdrawETHForKnot(address _recipient, bytes calldata _blsPublicKeyOfKnot) external {
        ...

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

Proof of Concept

Please add the following test in test\foundry\LSDNFactory.t.sol. This test will pass to demonstrate the described scenario.

    function testCompromisedDaoCanRestrictActionsOfNodeRunnersWhoAreNotMalicious() public {
        vm.prank(address(factory));
        manager.updateDAOAddress(admin);

        uint256 nodeStakeAmount = 4 ether;
        address nodeRunner = accountOne;
        vm.deal(nodeRunner, nodeStakeAmount);

        address eoaRepresentative = accountTwo;

        vm.prank(nodeRunner);
        manager.registerBLSPublicKeys{value: nodeStakeAmount}(
            getBytesArrayFromBytes(blsPubKeyOne),
            getBytesArrayFromBytes(blsPubKeyOne),
            eoaRepresentative
        );

        // Simulate a situation where admin, who is the dao at this moment, is compromised.
        // Although nodeRunner is not malicious,
        //   the compromised admin can call the rotateNodeRunnerOfSmartWallet function to assign nodeRunner's smart wallet to a colluded party.
        vm.prank(admin);
        manager.rotateNodeRunnerOfSmartWallet(nodeRunner, accountThree, true);

        // nodeRunner is blocked from other interactions with the protocol since it is now banned unfairly
        assertEq(manager.bannedNodeRunners(accountOne), true);

        // for example, nodeRunner is no longer able to call the withdrawETHForKnot function
        vm.prank(nodeRunner);
        vm.expectRevert("Not the node runner for the smart wallet ");
        manager.withdrawETHForKnot(nodeRunner, blsPubKeyOne);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When calling the deployNewLiquidStakingDerivativeNetwork function, instead of explicitly setting the DAO's address, a configurable governance contract, which can have features like voting and timelock, can be deployed and used as the DAO.

[L-01] BUGS IN SOLIDITY 0.8.13

For this protocol, the used pragmas are 0.8.13 and ^0.8.13, and the hardhat configuration uses 0.8.13. Yet, Solidity 0.8.13 has a size check bug, which involves nested calldata array and abi.encode, and an optimizer bug that affects inline assembly. It looks like that the code in scope are not affected by these bugs but the protocol team should be aware of them.

Please see the following links for reference:

• https://blog.soliditylang.org/2022/05/17/calldata-reencode-size-check-bug/
• https://blog.soliditylang.org/2022/06/15/inline-assembly-memory-side-effects-bug/

Please consider using at least Solidity 0.8.15 instead of 0.8.13 to address these bugs and be more future-proofed.

The following files use 0.8.13.

contracts\syndicate\Syndicate.sol
  1: pragma solidity 0.8.13;

contracts\syndicate\SyndicateErrors.sol
  1: pragma solidity 0.8.13;

contracts\syndicate\SyndicateFactory.sol
  1: pragma solidity 0.8.13;

The current hardhat configuration is shown below. https://github.com/code-423n4/2022-11-stakehouse/blob/main/hardhat.config.js#L4-L14

module.exports = {
  solidity: {
    version: "0.8.13",
    settings: {
      optimizer: {
        enabled: true,
        runs: 200
      }
    }
  }
};

[L-02] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

To prevent unintended behaviors, critical address inputs should be checked against address(0).

address(0) checks are missing for _savETHVaultDeployer, _stakingFundsVaultDeployer, and _optionalGatekeeperDeployer in the following _init function. Please consider checking these. https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L645-L679

    function _init(
        address _dao,
        address _syndicateFactory,
        address _smartWalletFactory,
        address _lpTokenFactory,
        address _brand,
        address _savETHVaultDeployer,
        address _stakingFundsVaultDeployer,
        address _optionalGatekeeperDeployer,
        uint256 _optionalCommission,
        bool _deployOptionalGatekeeper,
        string calldata _stakehouseTicker
    ) internal {
        ...

        _initStakingFundsVault(_stakingFundsVaultDeployer, _lpTokenFactory);
        _initSavETHVault(_savETHVaultDeployer, _lpTokenFactory);

        if (_deployOptionalGatekeeper) {
            gatekeeper = OptionalGatekeeperFactory(_optionalGatekeeperDeployer).deploy(address(this));
        }
    }

[L-03] INCORRECT COMMENT

Incorrect comment can be misleading. The following require statement acts as the opposite of its comment. For better readability and maintainability, please consider updating the comment to match the code.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L468-L469

            // check if the BLS public key is part of LSD network and is not banned
            require(isBLSPublicKeyPartOfLSDNetwork(_blsPublicKey) == false, "BLS public key is banned or not a part of LSD network");

[L-04] INACCURATE require REASONS

Inaccurate reason strings for require statements can be misleading. The following require statements' reason strings do not describe the require statements' conditions accurately. Please consider updating these reason strings to be more accurate and descriptive.

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/LiquidStakingManager.sol#L468-L469

            // check if the BLS public key is part of LSD network and is not banned
            require(isBLSPublicKeyPartOfLSDNetwork(_blsPublicKey) == false, "BLS public key is banned or not a part of LSD network");

https://github.com/code-423n4/2022-11-stakehouse/blob/main/contracts/liquid-staking/StakingFundsVault.sol#L175

        require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");

[N-01] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the following magic numbers, such as 4 ether, with constants.

contracts\liquid-staking\LiquidStakingManager.sol
  333: require(associatedSmartWallet.balance >= 4 ether, "Insufficient balance");  
  343: 4 ether  ; mi: extra amt left that is less than 4 ether cannot be withdrawn
  434: require(msg.value == len * 4 ether, "Insufficient ether provided"); 
  640: 1   
  749: savETHVault.withdrawETHForStaking(smartWallet, 24 ether);
  752: stakingFundsVault.withdrawETH(smartWallet, 4 ether);   
  766: 32 ether   
  870: sETH.approve(syndicate, (2 ** 256) - 1);    
  882: uint256 stakeAmount = 12 ether; 
  936: require(associatedSmartWallet.balance >= 4 ether, "Smart wallet balance must be at least 4 ether"); 
  940: require(stakingFundsLP.totalSupply() == 4 ether, "DAO staking funds vault balance must be at least 4 ether");
  944: require(savETHVaultLP.totalSupply() == 24 ether, "KNOT must have 24 ETH in savETH vault");

contracts\liquid-staking\SavETHVault.sol
  161: require(dETHBalance >= 24 ether, "Nothing to withdraw");    
  174: redemptionValue = (dETHDetails.savETHBalance * _amount) / 24 ether;    
  204: require(_amount >= 24 ether, "Amount cannot be less than 24 ether");    
  244: maxStakingAmountPerValidator = 24 ether;    

contracts\liquid-staking\StakingFundsVault.sol
  58: totalShares += 4 ether; 
  184: require(_lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Too new");    
  230: require(token.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp, "Last transfer too recent");  
  240: require(_amount >= 4 ether, "Amount cannot be less than 4 ether");  
  380: maxStakingAmountPerValidator = 4 ether; 

contracts\syndicate\Syndicate.sol
  215: if (_sETHAmount < 1 gwei) revert FreeFloatingStakeAmountTooSmall(); 
  221: if (totalStaked == 12 ether) revert KnotIsFullyStakedWithFreeFloatingSlotTokens();  
  223: if (_sETHAmount + totalStaked > 12 ether) revert InvalidStakeAmount();  
  362: if (stakedBal < 1 gwei) revert FreeFloatingStakeAmountTooSmall();  
  504: if (currentSlashedAmount < 4 ether) {  
  522: balance * unprocessedETHForCurrentKnot / (4 ether - currentSlashedAmount);

[N-02] MISSING INDEXED EVENT FIELDS

Querying event can be optimized with index. Please consider adding indexed to the relevant fields of the following events.

contracts\liquid-staking\LiquidStakingManager.sol
  84: event DAOCommissionUpdated(uint256 old, uint256 newCommission); 

contracts\liquid-staking\SavETHVault.sol
  19: event DETHRedeemed(address depositor, uint256 amount);  
  22: event ETHWithdrawnForStaking(address withdrawalAddress, address liquidStakingManager, uint256 amount);  

contracts\liquid-staking\StakingFundsVault.sol
  25: event ETHDeposited(address sender, uint256 amount); 
  28: event ETHWithdrawn(address receiver, address admin, uint256 amount); 
  31: event ERC20Recovered(address admin, address recipient, uint256 amount); 
  34: event WETHUnwrapped(address admin, uint256 amount); 

[N-03] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

contracts\liquid-staking\LiquidStakingManager.sol
  911: function _initStakingFundsVault(address _stakingFundsVaultDeployer, address _tokenFactory) internal virtual {   

contracts\liquid-staking\SavETHVault.sol
  45: function init(address _liquidStakingManagerAddress, LPTokenFactory _lpTokenFactory) external virtual initializer {  

contracts\liquid-staking\SavETHVaultDeployer.sol
  18: function deploySavETHVault(address _liquidStakingManger, address _lpTokenFactory) external returns (address) {  

contracts\liquid-staking\StakingFundsVaultDeployer.sol
  18: function deployStakingFundsVault(address _liquidStakingManager, address _tokenFactory) external returns (address) { 

contracts\smart-wallet\OwnableSmartWalletFactory.sol
  28: function createWallet() external returns (address wallet) { 
  32: function createWallet(address owner) external returns (address wallet) { 
  36: function _createWallet(address owner) internal returns (address wallet) { 

contracts\syndicate\Syndicate.sol
  491: function _updateCollateralizedSlotOwnersLiabilitySnapshot(bytes memory _blsPubKey) internal {