LSD Network - Stakehouse contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

Function _distributeETHRewardsToUserForToken() will distribute any due rewards from node running to msg.sender if they have an LP balance. it's used in GiantMevAndFeesPool to distribute rewards, but in that function the value of claimed[_user][_token] is set to wrong value so the calculations of user rewards would be wrong and contract would distribute funds wrongly.

Proof of Concept

This is _distributeETHRewardsToUserForToken() code in SyndicateRewardsProcessor:

    /// @dev Any due rewards from node running can be distributed to msg.sender if they have an LP balance
    function _distributeETHRewardsToUserForToken(
        address _user,
        address _token,
        uint256 _balance,
        address _recipient
    ) internal {
        require(_recipient != address(0), "Zero address");
        uint256 balance = _balance;
        if (balance > 0) {
            // Calculate how much ETH rewards the address is owed / due 
            uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
            if (due > 0) {
                claimed[_user][_token] = due;

                totalClaimed += due;

                (bool success, ) = _recipient.call{value: due}("");
                require(success, "Failed to transfer");

                emit ETHDistributed(_user, _recipient, due);
            }
        }
    }

This function is used to distribute user's ETH rewards, the code calculates the remaining user's rewards in contract and then transfer it to user and update user's claimed information. to calculated remaining user's rewards code uses: due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token] and then code sets claimed[_user][_token] = due;. accumulatedETHPerLPShare is storing Total accumulated ETH per share of LP<>KNOT that has minted derivatives. imagine this steps: 1- user1 balance in token1 (giant pool token) is 10, accumulatedETHPerLPShare is 1 and claimed[user1][token1] is 10 (claim is the max). 2- some time passes and accumulatedETHPerLPShare increase to 2. 3- user1 calls claimRewards() to get his remaining rewards. code recalculated accumulatedETHPerLPShare and set it to 2. code calculates user rewards as due = (10*2) - 10 = 10 and transfer 10 ETH to user and set claimed[user1][token1] to 10. 4- some time passes and accumulatedETHPerLPShare increase to 3. 5- user1 calls claimRewards() again to get his remaining rewards. code recalculated accumulatedETHPerLPShare and set it to 3. code calculates user rewards as due = (10*3) - 10 = 20 and transfer 20 ETH to user and set claimed[user1][token1] to 20. 6- user1 calls claimRewards() again to get his remaining rewards. code recalculated accumulatedETHPerLPShare and set it to 3(no time passed). code calculates user rewards as due = (10*3) - 20 = 10 and transfer 10 ETH to user and set claimed[user1][token1] to 10.

as you can see user1 had only 10 balance and accumulatedETHPerLPShare were 3 so user should get maximum 30 ETH as reward but user received 40 rewards. even when user calls claimRewards() consequently(step 5 and 6) user get rewards in each time because the value of claimed[user1][token1] doesn't set correctly. contract after calculating user remaining rewards and transferring them to user should set claimed amount to maximum so user don't get history rewards in the future.

Tools Used

VIM

Recommended Mitigation Steps

set claimed[user1][token1] as balance * accumulatedETHPerLPShare after transferring user rewards.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L169-L173 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/StakingFundsVault.sol#L342-L351

Vulnerability details

Impact

function afterTokenTransfer() is for that after an LP token is transferred, ensure that the new account cannot claim historical rewards and it sets claimed[_to][address(token)] to max. but it doesn't set claimed[_from][address(token)] value and the _from address would lose some future rewards because his claimed[_from][address(token)] value has been set by his old balance (before transfer).

Proof of Concept

This is afterTokenTransfer() and beforeTokenTransfer() codes in GiantMevAndFeesPool:

/// @notice Allow giant LP token to notify pool about transfers so the claimed amounts can be processed
    function beforeTokenTransfer(address _from, address _to, uint256) external {
        require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
        updateAccumulatedETHPerLP();

        // Make sure that `_from` gets total accrued before transfer as post transferred anything owed will be wiped
        if (_from != address(0)) {
            _distributeETHRewardsToUserForToken(
                _from,
                address(lpTokenETH),
                lpTokenETH.balanceOf(_from),
                _from
            );
        }

        // Make sure that `_to` gets total accrued before transfer as post transferred anything owed will be wiped
        _distributeETHRewardsToUserForToken(
            _to,
            address(lpTokenETH),
            lpTokenETH.balanceOf(_to),
            _to
        );
    }

    /// @notice Allow giant LP token to notify pool about transfers so the claimed amounts can be processed
    function afterTokenTransfer(address, address _to, uint256) external {
        require(msg.sender == address(lpTokenETH), "Caller is not giant LP");
        _setClaimedToMax(_to);
    }

As you can see function beforeTokenTransfer() sends _from and _to remaining rewards by calling _distributeETHRewardsToUserForToken() and in the function _distributeETHRewardsToUserForToken() code sets claimed[][] value of _from and _to based on their balance before transfer happens. in the function afterTokenTransfer() call _setClaimedToMax(_to) which reset claimed[][] for _to address but don't change claimed[][] for _from address even so balance of _from is changed too. this can cause that claimed[][] for _from has wrong value and the future reward calculation for _from address would fail. these are the steps that this bug happens in them:

1. user1 has 10 LP token balance and claimed[user1][token1] is 10 and accumulatedETHPerLPShare is 1.
2. user2 has 5 LP token balance and claimed[user2][token1] is 5.
3. some time passes and accumulatedETHPerLPShare sets to 3.
4. user1 send 5 LP toekn to user2.
5. contract first calls beforeTokenTransfer() which send remaining rewards to users. user1 would receive 3*10 - 10 = 20 ETH reward and claimed[user1][token1] would be 20 too. user2 would receive 3*5 - 5 = 10 ETH reward and claimed[user2][token1] would be 10.
6. contract then perform the transfer and update the balance and user1 would have 5 LP token and user2 would have 15 LP token.
7. contract then call afterTokenTransfer() and would set claimed[user2][token1] to 15 * 3 = 45.
8. some time passes and accumulatedETHPerLPShare sets to 4. in this period user1 had 5 LP token balance and should be entitled to some rewards.
9. user1 calls claimReward() to receive his rewards but contract would calculate rewards as 5 * 4 - 20 = 0 and would transfer no reward to user1. (due = balance * ETHperShare - claimed)

As you can see because claiemd[user1][token1] didn't set to correct value in afterTokenTransfer() so future rewards for user1 would be calculated wrongly.

Tools Used

VIM

Recommended Mitigation Steps

update _from address claimed[][] tokens in afterTokenTransfer() too.

Lines of code

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

Vulnerability details

Impact

Function withdrawDETH() in GiantSavETHVaultPool allows a user to burn their giant LP in exchange for dETH that is ready to withdraw from a set of savETH vaults. This function make external calls to user provided addresses without checking those addresses and send increased dETH balance of contract during the call to user. user can provide malicious addresses to contract and then took the execution flow during the transaction and increase dETH balance of contract by other calls and make contract to transfer them to him.

Proof of Concept

This is withdrawDETH() in GiantSavETHVaultPool code:

    /// @notice Allow a user to burn their giant LP in exchange for dETH that is ready to withdraw from a set of savETH vaults
    /// @param _savETHVaults List of savETH vaults being interacted with
    /// @param _lpTokens List of savETH vault LP being burnt from the giant pool in exchange for dETH
    /// @param _amounts Amounts of giant LP the user owns which is burnt 1:1 with savETH vault LP and in turn that will give a share of dETH
    function withdrawDETH(
        address[] calldata _savETHVaults,
        LPToken[][] calldata _lpTokens,
        uint256[][] calldata _amounts
    ) external {
        uint256 numOfVaults = _savETHVaults.length;
        require(numOfVaults > 0, "Empty arrays");
        require(numOfVaults == _lpTokens.length, "Inconsistent arrays");
        require(numOfVaults == _amounts.length, "Inconsistent arrays");

        // Firstly capture current dETH balance and see how much has been deposited after the loop
        uint256 dETHReceivedFromAllSavETHVaults = getDETH().balanceOf(address(this));
        for (uint256 i; i < numOfVaults; ++i) {
            SavETHVault vault = SavETHVault(_savETHVaults[i]);

            // Simultaneously check the status of LP tokens held by the vault and the giant LP balance of the user
            for (uint256 j; j < _lpTokens[i].length; ++j) {
                LPToken token = _lpTokens[i][j];
                uint256 amount = _amounts[i][j];

                // Check the user has enough giant LP to burn and that the pool has enough savETH vault LP
                _assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

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

                // Giant LP is burned 1:1 with LPs from sub-networks
                require(lpTokenETH.balanceOf(msg.sender) >= amount, "User does not own enough LP");

                // Burn giant LP from user before sending them dETH
                lpTokenETH.burn(msg.sender, amount);

                emit LPBurnedForDETH(address(token), msg.sender, amount);
            }

            // Ask
            vault.burnLPTokens(_lpTokens[i], _amounts[i]);
        }

        // Calculate how much dETH has been received from burning
        dETHReceivedFromAllSavETHVaults = getDETH().balanceOf(address(this)) - dETHReceivedFromAllSavETHVaults;

        // Send giant LP holder dETH owed
        getDETH().transfer(msg.sender, dETHReceivedFromAllSavETHVaults);
    }

As you can see first contract save the dETH balance of contract by this line: uint256 dETHReceivedFromAllSavETHVaults = getDETH().balanceOf(address(this)); and then it loops through user provided vaults addresses and call those vaults to withdraw dETH and in the end it calculates dETHReceivedFromAllSavETHVaults and transfer those dETH to user: getDETH().transfer(msg.sender, dETHReceivedFromAllSavETHVaults);. attacker can perform these steps: 1- create a malicious contract AttackerVault which is copy of SavETHVault with modifiction. 2- call withdrawDETH() with Vault list [ValidVault1, ValidVault2, AttackerVault, ValidVaul3]. 3- contract would save the dETH balance of itself and then loops through Vaults to validate and burn LPTokens. 4- contract would reach Vault AttackerVault and call attacker controlled address. 5- attacker contract call other functions to increase dETH balance of contract (if it's not possible to increase dETH balance of contract by other way so there is no need to save contract initial balance of dETH before the loop and dETH balance of contract would be zero always) 6- withdrawDETH() would finish the loop and transfer all the increase dETH balance to attacker which includes extra amounts.

because contract don't check the provided addresses and calls them and there is no reentrancy defense mechanism there is possibility of reentrancy attack which can cause fund lose.

Tools Used

VIM

Recommended Mitigation Steps

check the provided addresses and also have some reentrancy defence mechanisim.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L133-L157 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L24-L25

Vulnerability details

Impact

Variable idleETH in giant pools is storing total amount of ETH sat idle ready for either withdrawal or depositing into a liquid staking network and whenever a deposit or withdraw happens contract adjust the value of idleETH of contract, but in function bringUnusedETHBackIntoGiantPool() which brings unused ETH from savETH vault to giant pool the value of idleETH don't get increased which would cause those ETH balance to not be accessible for future staking or withdrawing.

Proof of Concept

This is bringUnusedETHBackIntoGiantPool() code in GiantSavETHVaultPool():

    /// @notice Any ETH that has not been utilized by a savETH vault can be brought back into the giant pool
    /// @param _savETHVaults List of savETH vaults where ETH is staked
    /// @param _lpTokens List of LP tokens that the giant pool holds which represents ETH in a savETH vault
    /// @param _amounts Amounts of LP within the giant pool being burnt
    function bringUnusedETHBackIntoGiantPool(
        address[] calldata _savETHVaults,
        LPToken[][] calldata _lpTokens,
        uint256[][] calldata _amounts
    ) external {
        uint256 numOfVaults = _savETHVaults.length;
        require(numOfVaults > 0, "Empty arrays");
        require(numOfVaults == _lpTokens.length, "Inconsistent arrays");
        require(numOfVaults == _amounts.length, "Inconsistent arrays");
        for (uint256 i; i < numOfVaults; ++i) {
            SavETHVault vault = SavETHVault(_savETHVaults[i]);
            for (uint256 j; j < _lpTokens[i].length; ++j) {
                require(
                    vault.isDETHReadyForWithdrawal(address(_lpTokens[i][j])) == false,
                    "ETH is either staked or derivatives minted"
                );
            }

            vault.burnLPTokens(_lpTokens[i], _amounts[i]);
        }
    }

As you can see it checks that ETH is available in savETH vault and then calls to burnLPTokens() to burn savETH LP tokens and bring unused ETH to giant pool address, this would increase giant pool ETH balance but code don't increase the idleETH value so contract would lose tracking of real idle ETH balance of contract. because the vaule of idleETH is used when withdrawing or depositing into savETH vaults so the contract can't reuse the returned ETH. these are the steps that cause this bug to happen: 1- giant pool has 100 idleETH. 2- with function batchDepositETHForStaking() users stake 80 ETH and the new value of idleETH would be 20 and contract LP Token balance increase by 80. 3- the 80 newly staked ETH is not yet staked in stakehouse. 4- with function bringUnusedETHBackIntoGiantPool() users bring back those 80 ETH from Vaults to giant pool and burn giant pool LP tokens and then giant pool have 100 idle ETH but because idleETH value don't get increase it still would show 20. 5- the extra 80 ETH would returned to giant pool wouldn't be accessible for withdrawing to users or depositing into Vaults because in withdrawing or depositing into Vaults the value of idleETH has been used to know the amount of idle ETH in giant pool and because the value doesn't show the correct amount so the extra amount of ETH wouldn't be lost.

Tools Used

VIM

Recommended Mitigation Steps

contract should correctly update value of idleETH in different actions because withdraw and deposit logics depend on it.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L195-L204 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L33-L48

Vulnerability details

Impact

When depositETH() is called in giant pool it calls _onDepositETH() which calls _setClaimedToMax() to make sure new ETH stakers are not entitled to ETH earned by but this can cause users to lose their remaining rewards when they deposits. code should first transfer user remaining rewards when deposit happens.

Proof of Concept

This is depositETH() code in GiantPoolBase:

    /// @notice Add ETH to the ETH LP pool at a rate of 1:1. LPs can always pull out at same rate.
    function depositETH(uint256 _amount) public payable {
        require(msg.value >= MIN_STAKING_AMOUNT, "Minimum not supplied");
        require(msg.value == _amount, "Value equal to amount");

        // The ETH capital has not yet been deployed to a liquid staking network
        idleETH += msg.value;

        // Mint giant LP at ratio of 1:1
        lpTokenETH.mint(msg.sender, msg.value);

        // If anything extra needs to be done
        _onDepositETH();

        emit ETHDeposited(msg.sender, msg.value);
    }

As you can see it increase user lpTokenETH balance and then calls _onDepositETH(). This is _onDepositETH() and _setClaimedToMax() code in GiantMevAndFeesPool contract:

    /// @dev On depositing on ETH set claimed to max claim so the new depositor cannot claim ETH that they have not accrued
    function _onDepositETH() internal override {
        _setClaimedToMax(msg.sender);
    }

    /// @dev Internal re-usable method for setting claimed to max for msg.sender
    function _setClaimedToMax(address _user) internal {
        // New ETH stakers are not entitled to ETH earned by
        claimed[_user][address(lpTokenETH)] = (accumulatedETHPerLPShare * lpTokenETH.balanceOf(_user)) / PRECISION;
    }

As you can see the code set claimed[msg.sender][address(lpTokenETH] to maximum value so the user wouldn't be entitled to previous rewards but if user had some remaining rewards in contract he would lose those rewards can't withdraw them. these are the steps: 1- user1 deposit 10 ETH to giant pool and accumulatedETHPerLPShare value is 2 and claimed[user1][lpTokenETH] would be 10 * 2 = 20. 2- some time passes and accumulatedETHPerLPShare set to 4 and user1 has 10 * 4 - 20 = 20 unclaimed ETH rewards (the formula in the code: balance * rewardPerShare - claimed). 3- user deposit 5 ETH to giant pool and accumulatedETHPerLPShare is 4 so the code would call _onDepositETH() which calls _setClaimedToMax which sets claimed[user1][lpTokenETH] to 15 * 4 = 60. 4- user1 new remaining ETH reward would be 15 * 4 - 60 = 0. and user1 won't receive his rewards because when he deposits contract don't transfer remaining rewards and set claim to max so user loses his funds.

Tools Used

VIM

Recommended Mitigation Steps

when deposit happens contract should first send remaining rewards, then increase the user's balance and then set the user claim to max.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L22-L53 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L23-L60

Vulnerability details

Impact

function batchDepositETHForStaking() in GiantSavETHVaultPool and GiantMevAndFeesPool are supposed to get the liquidity of the giant pool, stake ETH to receive protected deposits from many liquid staking networks (LSDNs). This functions get the Vaults list from user and perform some checks to make sure they are valid Vaults belonging to whitelisted LSD networks and send ETH balance of giant pool to those Vaults. but it's possible to bypass this whitelist check and make giant pool contract to send ETH balance to attacker controlled address. the bug is that for whitelist checking, contract checks the return value of a function call to user specified address and doesn't check the real address user specified.

Proof of Concept

This is batchDepositETHForStaking() code in GiantSavETHVaultPool:

   /// @notice Given the liquidity of the giant pool, stake ETH to receive protected deposits from many liquid staking networks (LSDNs)
    /// @dev Take ETH from the contract balance in order to send money to the individual vaults
    /// @param _savETHVaults List of savETH vaults that belong to individual liquid staking derivative networks
    /// @param _ETHTransactionAmounts ETH being attached to each savETH vault in the list
    /// @param _blsPublicKeys For every savETH vault, the list of BLS keys of LSDN validators receiving funding
    /// @param _stakeAmounts For every savETH vault, the amount of ETH each BLS key will receive in funding
    function batchDepositETHForStaking(
        address[] calldata _savETHVaults,
        uint256[] calldata _ETHTransactionAmounts,
        bytes[][] calldata _blsPublicKeys,
        uint256[][] calldata _stakeAmounts
    ) public {
        uint256 numOfSavETHVaults = _savETHVaults.length;
        require(numOfSavETHVaults > 0, "Empty arrays");
        require(numOfSavETHVaults == _ETHTransactionAmounts.length, "Inconsistent array lengths");
        require(numOfSavETHVaults == _blsPublicKeys.length, "Inconsistent array lengths");
        require(numOfSavETHVaults == _stakeAmounts.length, "Inconsistent array lengths");

        // For every vault specified, supply ETH for at least 1 BLS public key of a LSDN validator
        for (uint256 i; i < numOfSavETHVaults; ++i) {
            uint256 transactionAmount = _ETHTransactionAmounts[i];

            // As ETH is being deployed to a savETH pool vault, it is no longer idle
            idleETH -= transactionAmount;

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

            // Deposit ETH for staking of BLS key
            savETHPool.batchDepositETHForStaking{ value: transactionAmount }(
                _blsPublicKeys[i],
                _stakeAmounts[i]
            );
        }
    }

As you can see contract checks the savETHPool address provided by user with this line: require(liquidStakingDerivativeFactory.isLiquidStakingManager(address(savETHPool.liquidStakingManager())), "Invalid liquid staking") and then send ETH to that address in the line: savETHPool.batchDepositETHForStaking{ value: transactionAmount }. it's possible to bypass the check, because contract calls savETHPool.liquidStakingManager() and checks the return value of this call, attacker can create a malicious contract with the public function liquidStakingManager which returns correct value to pass the liquidStakingDerivativeFactory.isLiquidStakingManager() and fool the giant pool contract and then giant pool would send its ETH balance to attacker contract. so these are the steps attacker would do: 1- create a copy of SavETHVault contract(let's call it AttackerVault) which function liquidStakingManager() returns valid whitelisted liquid staking manager and function batchDepositETHForStaking() accepts payments and return true. 2- call function batchDepositETHForStaking() with address AttackerVault and amount equal to giant pool ETH balance. the other parameters don't matter 3- function batchDepositETHForStaking() to check the validity of AttackerVault would call AttackerVault.liquidStakingManager() and check the return value by liquidStakingDerivativeFactory.isLiquidStakingManager() and because AttackerVault returns a fake valid value the checks would pass. 4- then giant pool send all the ETH balance to AttackerVault by calling function batchDepositETHForStaking(). 5- attacker receive all the ETH balance of giant pool and steal them.

This bug happens because contract don't check the validity of AttackerVault and tries to check the validity of AttackerVault.liquidStakingManager(). and the return value of call to attacker controlled address can be anything and attacker can return fake values to fool contract and bypass the checks, it's almost like you are asking someone "are you a bad person?" and trusting the answer of that person.

This bug is also exists in batchDepositETHForStaking() of GiantMevAndFeesPool() too.

Tools Used

VIM

Recommended Mitigation Steps

contract should check the validity of provided address not the validity of return values of calls to those addresses.

Lines of code

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

Vulnerability details

Impact

function _updateAccumulatedETHPerLP() in SyndicateRewardsProcessor is tracking accumulated ETH per share but due to the fact that it doesn't consider deposited ETH (idelETH in contract balance) it consider deposited ETH as rewards. this would cause totally wrong reward distributions and some user lose their deposits.

Proof of Concept

This is _updateAccumulatedETHPerLP() and totalRewardsReceived() code in SyndicateRewardsProcessor:

    /// @dev Internal logic for tracking accumulated ETH per share
    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;
            }
        }
    }

    /// @notice Total rewards received by this contract from the syndicate
    function totalRewardsReceived() public virtual view returns (uint256) {
        return address(this).balance + totalClaimed;
    }

As you can see in totalRewardsReceived() the total balance of contract is treated as received rewards and in _updateAccumulatedETHPerLP() the value of idleETH is not used so if a user deposits ETH in the giant pool then those ETH would be considered as rewards and distributed among other users. these are the steps that cause this bug to happen: 1- contract has 100 ETH balance and totalClaimed is 20 and totalETHSeen is 120 and idleETH is 100 and lpTokenETH.totalSupply() is 200 and accumulatedETHPerLPShare is 0.1. (users deposited 200 ETH which 100 of them is staked and 100 ETH stayed in giant pool as idleETH and the staked ones created 20 ETH as reward) 2- user1 deposits 20 ETH into the contract which increase idleETH to 120 and lpTokenETH.totalSupply() to 220 and ETH balance contract to 120. 3- now updateAccumulatedETHPerLP() is get called it would calculate totalRewardsReceived() as 120 + 20 = 140 and unprocessed ETH as 140 - 120 = 20 and accumulatedETHPerLPShare as 0.1 + (20/220) = 0.19. 4- even so there was no new reward because contract don't consider the new deposits in the rewards formula so it treat deposits as rewards and user who deposited his ETH would lose his deposits.

Tools Used

VIM

Recommended Mitigation Steps

contract should consider deposits and idleETH in reward calculations.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantPoolBase.sol#L50-L64 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantMevAndFeesPool.sol#L180-L193

Vulnerability details

Impact

Function _distributeETHRewardsToUserForToken() is used to distribute remaining reward of user and it's called in _onWithdraw() of GiantMevAndFeesPool. but function withdrawETH() in GiantPoolBase don't call either of them and burn user giant LP token balance so if user withdraw his funds and has some remaining ETH rewards he would lose those rewards because his balance set to zero.

Proof of Concept

This is withdrawETH() code in GiantPoolBase:

    /// @notice Allow a user to chose to burn their LP tokens for ETH only if the requested amount is idle and available from the contract
    /// @param _amount of LP tokens user is burning in exchange for same amount of ETH
    function withdrawETH(uint256 _amount) external nonReentrant {
        require(_amount >= MIN_STAKING_AMOUNT, "Invalid amount");
        require(lpTokenETH.balanceOf(msg.sender) >= _amount, "Invalid balance");
        require(idleETH >= _amount, "Come back later or withdraw less ETH");

        idleETH -= _amount;

        lpTokenETH.burn(msg.sender, _amount);
        (bool success,) = msg.sender.call{value: _amount}("");
        require(success, "Failed to transfer ETH");

        emit LPBurnedForETH(msg.sender, _amount);
    }

As you can see it burn user lpTokenETH balance and don't call either _distributeETHRewardsToUserForToken() or _onWithdraw(). and in function claimRewards() uses lpTokenETH.balanceOf(msg.sender) to calculate user rewards so if user balance get to 0 user won't get the remaining rewards. These are steps that this bug happens:

1. user1 deposit 10 ETH into the giant pool and claimed[user1][lpTokenETH] is 20 and accumulatedETHPerLPShare is 2.
2. some time passes and accumulatedETHPerLPShare set to 3.
3. user1 unclaimed rewards are 10 * 3 - 20 = 10 ETH.
4. user1 withdraw his 10 ETH by calling withdrawETH(10) and contract set lpTokenETH balance of user1 to 0 and transfer 10 ETH to user.
5. now if user1 calls claimRewards() he would get 0 reward as his lpTokenETH balance is 0.

so users lose their unclaimed rewards by withdrawing their funds.

Tools Used

VIM

Recommended Mitigation Steps

user's unclaimed funds should be calculated and transferred before any actions that change user's balance.

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/GiantSavETHVaultPool.sol#L133-L157 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SavETHVault.sol#L122-L194 https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/LPToken.sol#L65-L70

Vulnerability details

Impact

Function bringUnusedETHBackIntoGiantPool() in GiantSavETHVaultPool is for bringing back any ETH that has not been utilized by a Staking Funds vault into the giant pool. attacker can prevent others from calling this function by resetting lastInteractedTimestamp value of giant pool contract (by sending small number of LPToken to giant pool address) and this DOS attack could cause critical functionlity of giant pool to broke. Function bringUnusedETHBackIntoGiantPool calls vault.burnLPTokens() in vault contract, but function burnLPTokens only allows to be called by any address every 30 minutes because of the line: bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp; and require(isStaleLiquidity, "Liquidity is still fresh");. so calls for bringUnusedETHBackIntoGiantPool() only can happen every 30 minutes by all users and one user can deny other from calling this function and he can call with dummy values and then other users can't bring the unused ETH amounts to giant pool (attacker would call this function with small values every 30 minutes or he can front-run others transaction, attacker can use LP token transfer too because it reset lastInteractedTimestamp too) and the giant pool funds would stuck in wrong LSDNs.

Proof of Concept

This is bringUnusedETHBackIntoGiantPool() code in GiantSavETHVaultPool:

    /// @notice Any ETH that has not been utilized by a savETH vault can be brought back into the giant pool
    /// @param _savETHVaults List of savETH vaults where ETH is staked
    /// @param _lpTokens List of LP tokens that the giant pool holds which represents ETH in a savETH vault
    /// @param _amounts Amounts of LP within the giant pool being burnt
    function bringUnusedETHBackIntoGiantPool(
        address[] calldata _savETHVaults,
        LPToken[][] calldata _lpTokens,
        uint256[][] calldata _amounts
    ) external {
        uint256 numOfVaults = _savETHVaults.length;
        require(numOfVaults > 0, "Empty arrays");
        require(numOfVaults == _lpTokens.length, "Inconsistent arrays");
        require(numOfVaults == _amounts.length, "Inconsistent arrays");
        for (uint256 i; i < numOfVaults; ++i) {
            SavETHVault vault = SavETHVault(_savETHVaults[i]);
            for (uint256 j; j < _lpTokens[i].length; ++j) {
                require(
                    vault.isDETHReadyForWithdrawal(address(_lpTokens[i][j])) == false,
                    "ETH is either staked or derivatives minted"
                );
            }

            vault.burnLPTokens(_lpTokens[i], _amounts[i]);
        }
    }

As you can see it calls vault.burnLPTokens(). This is burnLPToken() code in SavETHVault:

   /// @notice function to allow users to burn LP token in exchange of ETH or dETH
    /// @param _lpToken instance of LP token to be burnt
    /// @param _amount number of LP tokens the user wants to burn
    /// @return amount of ETH withdrawn
    function burnLPToken(LPToken _lpToken, uint256 _amount) public nonReentrant returns (uint256) {
        require(_amount >= MIN_STAKING_AMOUNT, "Amount cannot be zero");
        require(_amount <= _lpToken.balanceOf(msg.sender), "Not enough balance");

        // get BLS public key for the LP token
        bytes memory blsPublicKeyOfKnot = KnotAssociatedWithLPToken[_lpToken];
        IDataStructures.LifecycleStatus validatorStatus = getAccountManager().blsPublicKeyToLifecycleStatus(blsPublicKeyOfKnot);

        require(
            validatorStatus == IDataStructures.LifecycleStatus.INITIALS_REGISTERED ||
            validatorStatus == IDataStructures.LifecycleStatus.TOKENS_MINTED,
            "Cannot burn LP tokens"
        );

        // before burning, check the last LP token interaction and make sure its more than 30 mins old before permitting ETH withdrawals
        bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp;

        // burn the amount of LP token from depositor's wallet
        _lpToken.burn(msg.sender, _amount);
        emit LPTokenBurnt(blsPublicKeyOfKnot, address(_lpToken), msg.sender, _amount);

        if(validatorStatus == IDataStructures.LifecycleStatus.TOKENS_MINTED) {
            // return dETH
            // amount of dETH redeemed by user for given LP token
            uint256 redemptionValue;

            KnotDETHDetails storage dETHDetails = dETHForKnot[blsPublicKeyOfKnot];

            if(!dETHDetails.withdrawn) {
                // withdraw dETH if not done already

                // get dETH balance for the KNOT
                uint256 dETHBalance = getSavETHRegistry().knotDETHBalanceInIndex(indexOwnedByTheVault, blsPublicKeyOfKnot);
                uint256 savETHBalance = getSavETHRegistry().dETHToSavETH(dETHBalance);
                // This require should never fail but is there for sanity purposes
                require(dETHBalance >= 24 ether, "Nothing to withdraw");

                // withdraw savETH from savETH index to the savETH vault
                // contract gets savETH and not the dETH
                getSavETHRegistry().addKnotToOpenIndex(liquidStakingManager.stakehouse(), blsPublicKeyOfKnot, address(this));

                // update mapping
                dETHDetails.withdrawn = true;
                dETHDetails.savETHBalance = savETHBalance;
                dETHForKnot[blsPublicKeyOfKnot] = dETHDetails;
            }

            // redeem savETH from the vault
            redemptionValue = (dETHDetails.savETHBalance * _amount) / 24 ether;

            // withdraw dETH (after burning the savETH)
            getSavETHRegistry().withdraw(msg.sender, uint128(redemptionValue));

            uint256 dETHRedeemed = getSavETHRegistry().savETHToDETH(redemptionValue);

            emit DETHRedeemed(msg.sender, dETHRedeemed);
            return redemptionValue;
        }

        // Before allowing ETH withdrawals we check the value of isStaleLiquidity fetched before burn
        require(isStaleLiquidity, "Liquidity is still fresh");

        // return ETH for LifecycleStatus.INITIALS_REGISTERED
        (bool result,) = msg.sender.call{value: _amount}("");
        require(result, "Transfer failed");
        emit ETHWithdrawnByDepositor(msg.sender, _amount);

        return _amount;
    }

As you can see in lines bool isStaleLiquidity = _lpToken.lastInteractedTimestamp(msg.sender) + 30 minutes < block.timestamp; and require(isStaleLiquidity, "Liquidity is still fresh"); it checks that msg.sender didn't interacted with LPToken in the last 30 minutes. so attacker by calling bringUnusedETHBackIntoGiantPool() every 30 minutes can make giant pool to interact with the LPToken and deny other from calling bringUnusedETHBackIntoGiantPool(). even attacker can front-run others transaction when they are calling bringUnusedETHBackIntoGiantPool() and make their calls to fail by resetting lastInteractedTimestamp for giant pool address. because when LPToken transfer happens code reset lastInteractedTimestamp for _from and _to so attacker can send small number of LPToken to giant pool address every 30 minutes(or by front-running others transaction) and make others call to bringUnusedETHBackIntoGiantPool() to fail. Function bringUnusedETHBackIntoGiantPool()

    /// @dev If set, notify the transfer hook processor after token transfer
    function _afterTokenTransfer(address _from, address _to, uint256 _amount) internal override {
        lastInteractedTimestamp[_from] = block.timestamp;
        lastInteractedTimestamp[_to] = block.timestamp;
        if (address(transferHookProcessor) != address(0)) transferHookProcessor.afterTokenTransfer(_from, _to, _amount);
    }

Tools Used

VIM

Recommended Mitigation Steps

vault contract shouldn't limit giant pool calls, because giant pool calls to Vault has been really made by users in the first place, maybe it should check tx.origin insted of msg.sender.

Lines of code

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

Vulnerability details

Impact

Function withdrawLPTokens() in GiantPoolBase Allows a user to chose to withdraw vault LP tokens by burning their giant LP tokens. 1 Giant LP == 1 vault LP. but there is no check that provided LP tokens are valid LP tokens which belongs to whitelisted LSD networks. so if a user send wrong LP token address he would lose his giant LP tokens and receive wrong tokens. attacker can send malicious LP tokens and steal contract token balances like airdrops. also when this bug happens some of the vault LP tokens which belongs to Giant Pool would be in accessible, because the corresponding Giant LP token was burned.

Proof of Concept

This is function withdrawLPTokens() code in GiantPoolBase:

    /// @notice Allow a user to chose to withdraw vault LP tokens by burning their giant LP tokens. 1 Giant LP == 1 vault LP
    /// @param _lpTokens List of LP tokens being owned and being withdrawn from the giant pool
    /// @param _amounts List of amounts of giant LP being burnt in exchange for vault LP
    function withdrawLPTokens(LPToken[] calldata _lpTokens, uint256[] calldata _amounts) external {
        uint256 amountOfTokens = _lpTokens.length;
        require(amountOfTokens > 0, "Empty arrays");
        require(amountOfTokens == _amounts.length, "Inconsistent array lengths");

        _onWithdraw(_lpTokens);

        for (uint256 i; i < amountOfTokens; ++i) {
            LPToken token = _lpTokens[i];
            uint256 amount = _amounts[i];

            _assertUserHasEnoughGiantLPToClaimVaultLP(token, amount);

            // Burn giant LP from user before sending them an LP token from this pool
            lpTokenETH.burn(msg.sender, amount);

            // Giant LP tokens in this pool are 1:1 exchangeable with external savETH vault LP
            token.transfer(msg.sender, amount);

            emit LPSwappedForVaultLP(address(token), msg.sender, amount);
        }

This function Allows a user to chose to withdraw vault LP tokens by burning their giant LP tokens. 1 Giant LP == 1 vault LP. As you can see there is no check that _lpTokens list items are valid whitelisted Vaults LP tokens. contract just burns user's giant LP token balance and send contract's token balance to user (which user specified token address). contract doesn't have any sanity check that this LP tokens are belong to whitelisted LSD netowork.

This can cause multiple issues. first if a user send wrong LP Token address he would burn his balance in the giant pool but don't receive any savETH vault LP token. attacker can deceive users to perform such an action by crafting wrong transactions and contract don't protect users by any sanity check. the steps are: 1- user call this function with wrong token as LPToken (which contract has balance) either by mistake or other reasons. 2- contract would burn user balance in giant LP pool and transfer the worthless token balance of contract to user. 3- user would burn his giant LP token but don't receive equal savETH token.

second is that attacker can steal contract balance of other tokens like airdrops. 1- attacker call this function with airdropped token as LPToken. 2- contract would burn attacker balance in giant LP pool and transfer the airdropped balance of contract to attacker. 3- attacker would burn his giant LP token but received more valuable airdropped token.

also when this bug happens sum of all balance's of giant pool in savETH Vault pools don't equal to total giant LP token and some of giant pool balances wouldn't be accessible in the future because the corresponding giant LP token has been burned.

Tools Used

VIM

Recommended Mitigation Steps

perform some sanity checks for LPTokens and make sure they belong to whitelisted LSD networks that are deployed by manager.