c4-bot-7 commented 4 months ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/Magnetar/modules/MagnetarOptionModule.sol#L151-L158

Vulnerability details

Description

exitPositionAndRemoveCollateral function attempts to remove asset of user from Singularity market.

function exitPositionAndRemoveCollateral(ExitPositionAndRemoveCollateralData memory data) public payable {
    ...

    if (data.removeAndRepayData.removeAssetFromSGL) {
        uint256 _assetId = singularity_.assetId();
        uint256 share = yieldBox_.toShare(_assetId, _removeAmount, false);

        address removeAssetTo = data.removeAndRepayData.assetWithdrawData.withdraw
            || data.removeAndRepayData.repayAssetOnBB ? address(this) : data.user;

        singularity_.removeAsset(data.user, removeAssetTo, share);

        ...
}

It calls the removeAsset function of Singularity with the share variable, which represents the YieldBox shares of the repaid asset amount. However, the removeAsset function of Singularity uses a fraction of the lended part to be the repaid variable (see code snippet from gitmodule tapioca-bar).

///https://github.com/Tapioca-DAO/Tapioca-bar/blob/0dca4a0b8e31b7aa59cb20540079b80a95bbe494/contracts/markets/singularity/Singularity.sol#L245-L253
function removeAsset(address from, address to, uint256 fraction)
    external
    optionNotPaused(PauseType.RemoveAsset)
    returns (uint256 share)
{
    _accrue();
    share = _removeAsset(from, to, fraction);
    _allowedLend(from, share);
}

///https://github.com/Tapioca-DAO/Tapioca-bar/blob/0dca4a0b8e31b7aa59cb20540079b80a95bbe494/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();
    ...

Due to removing with a parameter being the share amount (YieldBox shares of asset tokens), the contract will remove more assets than needed from the user in exitPositionAndRemoveCollateral. This results in the user losing funds when using this functionality because removeAssetTo can be this contract if the user attempts to withdraw after removal:

address removeAssetTo = data.removeAndRepayData.assetWithdrawData.withdraw
            || data.removeAndRepayData.repayAssetOnBB ? address(this) : data.user;

For example, after depositing and accruing interest in the Singularity contract, Bob now has 100 asset fractions (base) and 1000 YieldBox shares of the asset (elastic). Bob calls exitPositionAndRemoveCollateral with withdraw flag set to true, expecting to remove his 100 asset shares and withdraw, but this function will pass 100 to the removeAsset function. Consequently, 100 asset fractions of Bob will be removed, which is equivalent to 1000 asset shares (more than the needed 900 shares). These shares are transferred to the Magnetar address to withdraw from YieldBox, so the redundant shares (900 shares) will be stuck in the contract.

Impact

Users will lose their funds when using exitPositionAndRemoveCollateral function.

Tools Used

Manual review

Recommended Mitigation Steps

Before calling Singularity.removeAssets(), exitPositionAndRemoveCollateral function should convert from asset shares amount (YieldBox shares of asset tokens) to the fraction of the lent part