Liquidation logic is still not completely correct

Summary

The implementation of BlueBerryBank.liquidate() is wrong in a few cases, which could lead to the liquidator getting all collaterals and underlying token shares by paying only one debt token.

Vulnerability Detail

Note that when a position is liquidatable, anybody can call BlueBerryBank.liquidate() and get some collaterals and underlying token shares stored in a vault after paying some debt. The liquidator pays only one debt token at a time.

If the liquidator pays full debt of a debtToken, the share will be same as oldShare.

        uint256 oldShare = pos.debtShare;
        (uint256 amountPaid, uint256 share) = _repay(
            positionId,
            debtToken,
            amountCall
        );

Because when the liquidator pays oldDebt in _repay(), lessShare will be same as oldShare.

function _repay(
        uint256 positionId,
        address token,
        uint256 amountCall
    ) internal returns (uint256, uint256) {
        Bank storage bank = banks[token];
        Position storage pos = positions[positionId];
        if (pos.debtToken != token) revert Errors.INCORRECT_DEBT(token);
        uint256 totalShare = bank.totalShare;
        uint256 totalDebt = _borrowBalanceStored(token);
        uint256 oldShare = pos.debtShare;
        uint256 oldDebt = (oldShare * totalDebt).divCeil(totalShare);
        if (amountCall > oldDebt) {
            amountCall = oldDebt;
        }
        amountCall = _doERC20TransferIn(token, amountCall);
        uint256 paid = _doRepay(token, amountCall);
        if (paid > oldDebt) revert Errors.REPAY_EXCEEDS_DEBT(paid, oldDebt); // prevent share overflow attack
        uint256 lessShare = paid == oldDebt
            ? oldShare
            : (paid * totalShare) / totalDebt;
        bank.totalShare -= lessShare;
        pos.debtShare -= lessShare;
        return (paid, lessShare);
    }

Let us get back to BlueBerryBank.liquidate(). If share = oldShare, liqSize = pos.collateralSize, and all of collateral amounts will be sent to the liquidator. And pos.collateralSize will be 0, and similar things happen to uVaultShare.

        uint256 liqSize = (pos.collateralSize * share) / oldShare;
        uint256 uVaultShare = (pos.underlyingVaultShare * share) / oldShare;

        pos.collateralSize -= liqSize;
        pos.underlyingVaultShare -= uVaultShare;

        // Transfer position (Wrapped LP Tokens) to liquidator
        IERC1155Upgradeable(pos.collToken).safeTransferFrom(
            address(this),
            msg.sender,
            pos.collId,
            liqSize,
            ""
        );

The liquidator paid only one debt, but he will get all of collateral and underlying shares stored in vault. The collateral and underlying supports all debt tokens, not only one debt token, so this implementation is not correct.

Impact

The liquidator gets more than he should get, so it will cause fund loss of the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L731-L759

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/BlueBerryBank.sol#L483-L548

Tool used

Manual Review

Recommendation

Use something like:

liqSize = (pos.collateralSize * share) * (debt of debtToken) / oldShare / (total debt)

instead of:

liqSize = (pos.collateralSize * share) / oldShare.

And same things to uVaultShare.

sherlock-audit / 2023-04-blueberry-judging

Bauchibred - Liquidation logic is still not completely correct #81