The user will not be liquidated due to discrepancies between `returnedShare` and `borrowShare` caused by rounding errors

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x5b66cf2b8f130df2a37362a32438f5ba2b12c8d078527a7ced1b08f7d99d4b40 Severity: medium

Description: Description\ Describe the context and the effect of the vulnerability.

Attack Scenario\ Describe how the vulnerability can be exploited.

Attachments**Description**\ The BBLiquidation:_liquidateUser will revert due to wrong casting of borrowShare and returnedShare. When we calculate the borrowShare of assets we round up the share in favor of caller. https://github.com/hats-finance/Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/blob/dev/contracts/markets/bigBang/BBLiquidation.sol#L277-L282

Inside this _swapCollateralWithAsset function We do not round up:

https://github.com/hats-finance/Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/blob/dev/contracts/markets/bigBang/BBLiquidation.sol#L174-L175

Due to this rounding issue The User will not be able to liquidate the borrower who has created bad bedt in the market. However The owner can still liquidate this user because in case of owner liquidation calls we never round up. That why it is medium.

        uint256 borrowShare = yieldBox.toShare(assetId, borrowAmount, true); // @audit : round up 

        (uint256 returnedShare,) =
            _swapCollateralWithAsset(collateralShare, _liquidatorReceiver, _liquidatorReceiverData, _exchangeRate, true);
        if (returnedShare < borrowShare) revert AmountNotValid(); // share = 3.4 => borrowShare = 4 , && returndShare = 3 3<4 it will revert 

Attack Scenario\ Let assumes that Bob calls liquidate function to Liquidate Alice.

1. The shares of borrowAmount of ALice will be 3.4 we round up it will return 4. So the borrowShare=4
2. After swapping we convert the returned amount into share of assets. after conversion er receive 3 Because there is no rounding up. So the returnedShare=3
3. This call will if (returnedShare < borrowShare) revert AmountNotValid(); because returnShare=3 and borrowShare=4.

Matigation\ Either don't perform rounding up in either case or perform it in all cases. It can also be fix if we keep consistency by passing an Flag to _swapCollateralWithAsset which will be passed _toShare function in both function _swapCollateralWithAsset and _liquidateUser. this will it will not impact the owner logic

[amankakar]

reference of _toShare function :

function _toShares(
        uint256 amount,
        uint256 totalShares_,
        uint256 totalAmount,
        bool roundUp
    ) internal pure returns (uint256 share) {
        // To prevent reseting the ratio due to withdrawal of all shares, we start with
        // 1 amount/1e8 shares already burned. This also starts with a 1 : 1e8 ratio which
        // functions like 8 decimal fixed point math. This prevents ratio attacks or inaccuracy
        // due to 'gifting' or rebasing tokens. (Up to a certain degree)
        totalAmount++;
        totalShares_ += 1e8;

        // Calculte the shares using te current amount to share ratio
        share = (amount * totalShares_) / totalAmount;

        // Default is to round down (Solidity), round up if required
        if (roundUp && (share * totalAmount) / totalShares_ < amount) {
            share++;
        }
    }

[cryptotechmaker]

collateralShare is borrowAmount + bonus. Rounding up borrowAmount when is converted to shares doesn't make it higher than collateralShare

[amankakar]

Hi @cryptotechmaker I will disagree with you respectfully on following points:

1. Let first check where the borrowAmount is calculated :

   Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/contracts/markets/bigBang/BBLiquidation.sol:203
   203:         borrowPartWithBonus = borrowPartWithBonus > maxBorrowPart ? maxBorrowPart : borrowPartWithBonus;
   204:         borrowAmount = borrowPartWithBonus;
   205: 

The borrowPartWithBonus will only be update in case the market has set liquidationBonusAmount other wise the borrowAmount and borrowPartWithBonus.

Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/contracts/markets/bigBang/BBLiquidation.sol:212
212:         if (liquidationBonusAmount > 0) {
213:             borrowPartWithBonus = borrowPartWithBonus + (borrowPartWithBonus * liquidationBonusAmount) / FEE_PRECISION;
214:         }

Here We can assume that there are certain case or markets on which the borrowAmount and borrowPartWithBonus will be same , even in cases where the liquidationBonusAmount is 1e2 the borrowPartWithBonus will be increase by 1. More on this point in POC.

2. Let check where the collateralShare is calcualted : Assume here that collateralPartInAsset >= borrowPartWithBonus so the collateralShare is :

   Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/contracts/markets/bigBang/BBLiquidation.sol:234
   234:             collateralShare =
   235:                 yieldBox.toShare(collateralId, (borrowPartWithBonus * _exchangeRate) / EXCHANGE_RATE_PRECISION, false); // @audit : also round down here

So now Lets put all above points together with some POC values:

borrowAmount = 1051 // code snippet 1 both are smae
borrowWithBonus = 1052; //  = liquidationBonusAmount = 1e2 =>  1051 + 1=> 1052 ,  even it is 1e1 =>1051 + (1051 * 1e1)/1e55 => 1051 + 0.1051 . if we have `liquidationBonusAmount>0` then borrowWithBonus will be updated

borrowWithBonus=1052
collateralShare = 1051// due to round down it return 1051  in this case  assume exchangeRate => 1 ether / 1e18 => 1

So now we have :
collateralShare = 1051
borrowAmount = 1051

borrowShare = 1052 // due to round up 

uint256 borrowShare = yieldBox.toShare(assetId, borrowAmount, true); // @audit : round up 

// _swapCollateralWithAsset function will return returnedShare which we have round down
returnedShare = 1051
returnedShare = yieldBox.toShare(assetId, returnedAmount, false); // @audit : no rounding

if (returnedShare < borrowShare) revert AmountNotValid(); //  borrowShare = 1052 , && returndShare = 1051 it will revert 

The issue would occur very frequently where the Bonus Fee is 0>=liquidationBonusAmount<1e2. I would suggest to remove the roundup where the borrowShare calculated or add roundup where the collateralShare is calculated.

One more Point to keep in mind is that we do allow to liquidate the user when returndShare=borrowShare So the above POC is valid according to code logic.

[cryptotechmaker]

I think you're presenting a hypothetical case here, because:

• we always have a liquidation bonus, otherwise, there's no point for users to call liquidate
• borrowAmount = 1051 and collateralShare = 1051 is possible only if the position is not liquidated at the right time; it would also revert with BadDebt error since if (collateralPartInAsset < borrowPartWithBonus) revert BadDebt();
• also numbers are in 18 decimals. Rounding up would be a 1 wei difference (like 999999999999999999996484125 vs 999999999999999999996484126 )

[amankakar]

borrowAmount = 1051 and collateralShare = 1051 is possible only if the position is not liquidated at the right time; it would also revert with BadDebt error since if (collateralPartInAsset < borrowPartWithBonus) revert BadDebt();

1. The value for collateralShare is calculated in else case so the BadDebt() would not apply . and collateralPartInAsset < borrowPartWithBonus is not true because both are equal.
2. In case of rounding the result would be the same.
3. The returnedShare value is result of two round down 1 here ,then we pass the collateralShare value for swap and round down returnedShare 2nd here after swap.

Note: I have perform all the calculation without decimals. Exactly the 1/2 wei difference will not allow the liquidator to liquidate due to wrong rounding.