0xHati - safeApprove should only be used to set an initial allowance

[sherlock-admin]

0xHati

medium

safeApprove should only be used to set an initial allowance

Summary

_loan within FlashLoan.sol is using safeApprove, this will revert when the FlashLoan contract already has an allowance for an asset and a spender and wants to increase it.
OpenZeppelin/openzeppelin-contracts#2219

Vulnerability Detail

Let's look at the implementation of safeApprove:

function safeApprove(IERC20 token, address spender, uint256 value) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        require(
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
    }

As the comments indicate, this call will revert when approving from non-zero to non-zero.

Now let's look at where it's used. We see that there's a check if allowance < amount then we call safeApprove. Let's say the market is 'A' and the first flashloan issued sets the max approve to IronBank for 'A'. With time the allowance will get lower and lower as more flashloans are used for that market. When allowance < amount it will try calling safeApprove but it will revert and it won't be possible anymore to use a flashloan for that market.

function _loan(
        IERC3156FlashBorrower receiver,
        address token,
        uint256 amount,
        bytes memory data,
        address msgSender
    ) internal {
        IronBankInterface(ironBank).borrow(address(this), address(receiver), token, amount);

        require(
            receiver.onFlashLoan(msgSender, token, amount, 0, data) == CALLBACK_SUCCESS,
            "callback failed"
        ); // no fee

        IERC20(token).safeTransferFrom(address(receiver), address(this), amount);

        uint256 allowance = IERC20(token).allowance(address(this), ironBank);
        if (allowance < amount) {
            //@audit-issue safeApprove is deprecated:
            //https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219
            // use safeIncreaseAllowance instead
            IERC20(token).safeApprove(ironBank, type(uint256).max);
        }

        IronBankInterface(ironBank).repay(address(this), address(this), token, amount);
    }

Impact

It won't be possible to use a flashloan anymore for a market when an initial allowance is set and
allowance < amount

Code Snippet

Link to code
See the loan fragment above.

Tool used

Manual Review

Recommendation

Use safeIncreaseAllowance, just as IronBank uses it correctly everywhere except here.

Duplicate of #420