Liquidation logic is not correct

Summary

The implementation of BlueBerryBank.liquidate is wrong, and the liquidator can get all collaterals and underlying token shares by paying only one debt token.

Vulnerability Detail

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.debtShareOf[debtToken];
        (uint256 amountPaid, uint256 share) = repayInternal(
            positionId,
            debtToken,
            amountCall
        );

Because when the liquidator pays oldDebt in repayInternal, lessShare will be same as oldShare.

    function repayInternal(
        uint256 positionId,
        address token,
        uint256 amountCall
    ) internal returns (uint256, uint256) {
        Bank storage bank = banks[token];
        Position storage pos = positions[positionId];
        uint256 totalShare = bank.totalShare;
        uint256 totalDebt = bank.totalDebt;
        uint256 oldShare = pos.debtShareOf[token];
        uint256 oldDebt = (oldShare * totalDebt).divCeil(totalShare);
        if (amountCall == type(uint256).max) {
            amountCall = oldDebt;
        }
        amountCall = doERC20TransferIn(token, amountCall);
        uint256 paid = doRepay(token, amountCall);
        if (paid > oldDebt) revert REPAY_EXCEEDS_DEBT(paid, oldDebt); // prevent share overflow attack
        uint256 lessShare = paid == oldDebt
            ? oldShare
            : (paid * totalShare) / totalDebt;
        bank.totalShare = totalShare - lessShare;
        uint256 newShare = oldShare - lessShare;
        pos.debtShareOf[token] = newShare;
        if (newShare == 0) {
            pos.debtMap &= ~(1 << uint256(bank.index));
        }
        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 happend to uVaultShare and uTokenSize.

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

        pos.collateralSize -= liqSize;

        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-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L522-L527 https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L760-L787 https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L529 https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L533 https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L538-L544

Tool used

Manual Review

Recommendation

Use

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

instead of

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

And same things to uVaultShare and uTokenSize.

Duplicate of #127

sherlock-audit / 2023-02-blueberry-judging

XKET - Liquidation logic is not correct #277