code423n4 commented 2 years ago

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.