Max amount for repay function does not have correct formula

Summary

If amountCall parameter in repayInternal in BlueBerryBank.sol is set to type(uint256).max then the max amount of borrow tokens are sent back to the user. The formula for calculating the max number of borrow tokens is incorrect and can lead to a revert of the tx.

Vulnerability Detail

This is the code in question:

    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) {
            // STENT this amount is not correct, it should involve a more complicated equation or just have the amount given by the balance of borrowToken in the spell
            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);
    }

The formula for oldDebt is not the max amount of borrow tokens that can be transferred. The bank tries to transfer borrow tokens from the Ichi spell to the bank (in the case that the Ichi spell is used to open and close positions). The spell got the tokens from the Ichi vault and then did a swap on Uniswap, thus the amount of borrow tokens that the spell has is dependent on the inner workings of Ichi and Uniswap, which the formula for oldDebt does not take into account.

Example test where tx is reverted with ERC20: transfer amount exceeds balance:

    describe("Ichi spell open-close functionality", async () => {
            const depositAmount = utils.parseUnits('100', 18); // worth of $400
            const borrowAmount = utils.parseUnits('300', 6);
            const iface = new ethers.utils.Interface(SpellABI);
                    const tenE18 = utils.parseUnits('10', 18);
                    const tenE6 = utils.parseUnits('10', 6);

            beforeEach(async () => {
                  await usdc.approve(bank.address, ethers.constants.MaxUint256);
                  await ichi.approve(bank.address, ethers.constants.MaxUint256);
                  await bank.execute(
                        0,
                        spell.address,
                        iface.encodeFunctionData("openPosition", [
                              0,
                              ICHI,
                              USDC,
                              depositAmount,
                              borrowAmount // 3x
                        ])
                  )
            })
        it("should be able to close a position", async () => {
                  const positionIds = await bank.getPositionIdsByOwner(admin.address);
                  console.log(positionIds);
                  await ichiVault.rebalance(-260400, -260200, -260800, -260600, 0);
                  let positionInfo = await bank.getPositionInfo(1);
                  let debtValue = await bank.getDebtValue(1)
                  let positionValue = await bank.getPositionValue(1);
                  let risk = await bank.getPositionRisk(1)

            await bank.borrowBalanceCurrent(1, USDC);
            const bbc = await bank.borrowBalanceStored(1, USDC);
            const totalShare = await bank.getTotalShare(USDC);
            const totalDebt = await bank.getTotalDebt(USDC);
            const debtShareOf = await bank.getPositionDebtShareOf(1, USDC);
            const repayAmount = debtShareOf.mul(totalDebt).div(totalShare);

                  await bank.execute(
                        1,
                        spell.address,
                        iface.encodeFunctionData("closePosition", [
                              0,
                              ICHI,
                              USDC,
                                                  positionInfo.collateralSize,
                                                  repayAmount,
                                                  0,
                                                  positionInfo.underlyingVaultShare
                        ])
                  )
        })
    })

Impact

Any smart contract that depends on the assumption that amountCall == type(uint256).max means all borrow tokens are returned to Compound may break, up to the mercy of inner workings of Uni and Ichi. If there are more borrow tokens than the formula suggests then everything works fine, but otherwise the tx will revert.

Code Snippet

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/BlueBerryBank.sol#L770

Tool used

Manual Review

Recommendation

Set the max amount to min(oldDebt, borrowToken.balanceOf(ichiSpell)

Duplicate of #14

sherlock-audit / 2023-02-blueberry-judging

stent - Max amount for repay function does not have correct formula #321