Incomplete integration for fee on transfer tokens since contract logic does not provide method for the complete debt size to be paid off

Summary

The protocol caters to fee on transfer tokens by measuring token balances before and after transfers to determine the value received. However, the mechanism to pay the full debt will never succeed in paying off the debt if it is used with a fee on transfer token.

This case was previously submitted in the past contest, but the fix the team provided does not really solve this, as the idea of the vulnerability is not from the fact that type(uint256).max was used but rather cause the amountCall = oldDebt, and if this is the case the user's full repayment of oldDebt wouldn't really pay off oldDebt since a fee would be deducted from the oldDebt, to clarify this, look at a short POC below

Proof Of Concept:

UserA has an oldDebt of 100, he then provided an oldDebt = 100 to pay of his debt, thinking he's done so. but if the token charges lets say 1% fee per transfer, in real sense he only sent 99 tokens to repay and still has a debt of 1 token unknowing to him that he hasn't paid off, so the repayment of his full debt would fail silently since the fee on transfer would be deducted from oldDebt and the oldDebt would not be paid in full at the same time users remaining debt would be accruing interest.

Vulnerability Detail

The protocol is clearly designed to ensure it is compatible with fee on transfer tokens. For example, all functions that receive tokens check the balance before and after, and calculate the difference between these values to measure tokens received:

    /// @dev Internal function to perform ERC20 transfer in and return amount actually received.
    /// @param token The token to perform transferFrom action.
    /// @param amountCall The amount use in the transferFrom call.
    function _doERC20TransferIn(
        address token,
        uint256 amountCall
    ) internal returns (uint256) {
        uint256 balanceBefore = IERC20Upgradeable(token).balanceOf(
            address(this)
        );
        IERC20Upgradeable(token).safeTransferFrom(
            msg.sender,
            address(this),
            amountCall
        );
        uint256 balanceAfter = IERC20Upgradeable(token).balanceOf(
            address(this)
        );
        return balanceAfter - balanceBefore;
    }

In previous code there was another feature of the protocol, which is that when loans are being repaid, the protocol gives the option of passing type(uint256).max to pay your debt in full:

if (amountCall == type(uint256).max) {
amountCall = oldDebt;
}

But now the protocol only provides this option

       if (amountCall > oldDebt) {
            amountCall = oldDebt;
        }

However, these two features are not really that different, since If a user paying off fee on transfer tokens passes in type(uint256).max or a value of amountCall > oldDebt to pay their debt in full, the full amount of their debt will be calculated (i.e oldDebt ). But when that amount is transferred to the contract, the amount that the result increases will be slightly less. As a result, the user will retain some balance that is not paid off.

Impact

The feature to allow loans of average users to be paid in full will always silently fail when used with fee on transfer tokens, which would trick normal users into thinking they have completely paid off their loans, and accidentally maintaining a balance. This balance could still be accruing more interest which would lead to users even having more debt than oldDebt - fee

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#L859-L878

Tool used

Manual Review

Recommendation

Afaik it's going to be very ddifficult to implement a mechanism to pay fee on transfer tokens off in full. This would add a lot of complexity, one can give an idea of finding out what the fees are for each fee on transfer tokens, but this just seems too bogus to integrate into protocol and unnecessary complexity.

But the main case here is that the failure is silent, cause users that request to pay off their loan in full, get confirmation, and may not realize that the loan still has an outstanding balance with interest accruing.

To solve this, there should be a confirmation that any user who passes any value of amountCall > oldDebt has paid off their debt in full. Otherwise, the function should revert, so that users paying fee on transfer tokens know that they cannot use the "pay in full" feature and must specify the correct amount to get their outstanding balance down to zero.

sherlock-audit / 2023-04-blueberry-judging

Bauchibred - Incomplete integration for fee on transfer tokens since contract logic does not provide method for the complete debt size to be paid off #79