Liquidation fees are permanently frozen on Penrose YB account

Summary

There is no treatment of liquidation fees in SGL, they are frozen on Penrose YB account.

Vulnerability Detail

There are 3 kinds of fees, borrow/interest and liquidation ones. The latter miss the handling logic, so such funds are accumulated and frozen.

Impact

Protocol-wide loss of funds, which othwerwise would be channelled to stakers.

Code Snippet

Interest fees are accumulated in the accrueInfo.feesEarnedFraction variable:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLCommon.sol#L103-L105

        uint256 feeAmount = (extraAmount * protocolFee) / FEE_PRECISION; // % of interest paid goes to fee
        feeFraction = (feeAmount * _totalAsset.base) / (fullAssetAmount - feeAmount);
        _accrueInfo.feesEarnedFraction += feeFraction.toUint128();

Which is then accumulated on internal Penrose account via withdrawing feeShares = _removeAsset(_feeTo, msg.sender, balanceOf[address(penrose)]):

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L565-L568

    function _depositFeesToTwTap(IMarket market, ITwTap twTap) private {
        if (!isMarketRegistered[address(market)]) revert NotValid();

>>      uint256 feeShares = market.refreshPenroseFees();

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/Singularity.sol#L285-L298

    function refreshPenroseFees() external onlyOwner returns (uint256 feeShares) {
        address _feeTo = address(penrose);
        // withdraw the fees accumulated in `accrueInfo.feesEarnedFraction` to the balance of `feeTo`.
        if (accrueInfo.feesEarnedFraction > 0) {
            _accrue();
            uint256 _feesEarnedFraction = accrueInfo.feesEarnedFraction;
            balanceOf[_feeTo] += _feesEarnedFraction;
            emit Transfer(address(0), _feeTo, _feesEarnedFraction);
            accrueInfo.feesEarnedFraction = 0;
            emit LogWithdrawFees(_feeTo, _feesEarnedFraction);
        }

>>      feeShares = _removeAsset(_feeTo, msg.sender, balanceOf[_feeTo]);
    }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLCommon.sol#L199-L216

    function _removeAsset(address from, address to, uint256 fraction) internal returns (uint256 share) {
        if (totalAsset.base == 0) {
            return 0;
        }
        Rebase memory _totalAsset = totalAsset;
        uint256 allShare = _totalAsset.elastic + yieldBox.toShare(assetId, totalBorrow.elastic, false);
>>      share = (fraction * allShare) / _totalAsset.base;

        _totalAsset.base -= fraction.toUint128();
        if (_totalAsset.base < 1000) revert MinLimit();

        balanceOf[from] -= fraction;
        emit Transfer(from, address(0), fraction);
        _totalAsset.elastic -= share.toUint128();
        totalAsset = _totalAsset;
        emit LogRemoveAsset(from, to, share, fraction);
>>      yieldBox.transfer(address(this), to, assetId, share);
    }

However, liquidation fees are being placed to Penrose account directly and aren't included in the feeShares = share variable above:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLiquidation.sol#L297-L312

    function _extractLiquidationFees(uint256 extraShare, uint256 callerReward)
        ...
    {
        callerShare = (extraShare * callerReward) / FEE_PRECISION; //  y%  of profit goes to caller.
>>      feeShare = extraShare - callerShare; // rest goes to the fee

        if (feeShare > 0) {
>>          uint256 feeAmount = yieldBox.toAmount(assetId, feeShare, false);
>>          yieldBox.depositAsset(assetId, address(this), address(penrose), feeAmount, 0);
        }
        if (callerShare > 0) {
            uint256 callerAmount = yieldBox.toAmount(assetId, callerShare, false);
            yieldBox.depositAsset(assetId, address(this), msg.sender, callerAmount, 0);
        }
    }

But _depositFeesToTwTap() uses only refreshPenroseFees() returned yieldBox.toAmount(_assetId, feeShares, false), which consists of interest and borrowing fees only:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L565-L578

    function _depositFeesToTwTap(IMarket market, ITwTap twTap) private {
        ...

>>      uint256 feeShares = market.refreshPenroseFees();
        ...
        yieldBox.withdraw(_assetId, address(this), address(this), 0, feeShares);

        uint256 rewardTokenId = twTap.rewardTokenIndex(_asset);
>>      uint256 feeAmount = yieldBox.toAmount(_assetId, feeShares, false);
>>      _distributeOnTwTap(feeAmount, rewardTokenId, _asset, twTap);
    }

This way liquidation fees accumulated on Penrose's YB account are frozen there as there are no Singularity fees distribution mechanics besides withdrawAllMarketFees() $\rightarrow$ _depositFeesToTwTap():

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L240-L255

    /// @notice Loop through the master contracts and call `_depositFeesToTwTap()` to each one of their clones.
    /// @param markets_ Singularity &/ BigBang markets array
    /// @param twTap the TwTap contract
    function withdrawAllMarketFees(IMarket[] calldata markets_, ITwTap twTap) external onlyOwner notPaused {
        if (address(twTap) == address(0)) revert ZeroAddress();

        uint256 length = markets_.length;
        unchecked {
            for (uint256 i; i < length;) {
                _depositFeesToTwTap(markets_[i], twTap);
                ++i;
            }
        }

        emit ProtocolWithdrawal(markets_, block.timestamp);
    }

Tool used

Manual Review

Recommendation

Consider placing liquidation fees into Penrose internal account, leaving them with common YB account of SGL, e.g.:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLiquidation.sol#L304-L307

        if (feeShare > 0) {
            uint256 feeAmount = yieldBox.toAmount(assetId, feeShare, false);
+           uint256 fullAssetAmount = yieldBox.toAmount(assetId, totalAsset.elastic, false) + totalBorrow.elastic;
+           uint256 feeFraction = (feeAmount * totalAsset.base) / fullAssetAmount;
+           balanceOf[address(penrose)] += feeFraction;
+           totalAsset.base += feeFraction.toUint128();
+           totalAsset.elastic += feeShare.toUint128();
-           yieldBox.depositAsset(assetId, address(this), address(penrose), feeAmount, 0);
+           yieldBox.depositAsset(assetId, address(this), address(this), 0, feeShare);
        }

sherlock-audit / 2024-02-tapioca-judging

hyh - Liquidation fees are permanently frozen on Penrose YB account #148