devScrooge - DOS for supplyPToken due to not approving 0 amount first

[sherlock-admin]

devScrooge

medium

DOS for supplyPToken due to not approving 0 amount first

Summary

There are some tokens like, USDT, which will revert when approving or executing safeIncreaseAllowance is the allowance was not previosuly set to 0

Vulnerability Detail

The TxBuilderExtension.sol contract implements a function called supplyPToken which is used for the users to supply tokens for a market on the IronBank contract:

 function supplyPToken(address user, address pToken, uint256 amount) internal nonReentrant {
        address underlying = PTokenInterface(pToken).getUnderlying();
        IERC20(underlying).safeTransferFrom(user, pToken, amount);
        PTokenInterface(pToken).absorb(address(this));
        IERC20(pToken).safeIncreaseAllowance(address(ironBank), amount);
        ironBank.supply(address(this), user, pToken, amount);
    }

There is a specific line which is increase the allowance of the pToken for the IronBank to be able to execute the supply function:

 IERC20(pToken).safeIncreaseAllowance(address(ironBank), amount);

The problem is that if pToken is one of the tokens that reverts if the allowance was not first set to 0 before increasing it to other amount, such as USDT, the transaction will revert and a denial of service will take place.

Impact

Denial of service for the users that execute the supplyPToken function in on the TxBuilderExtensioncontract by calling execute for supplying tokens to a market which has a token that reverts, such as USDT, as underlying token.

Code Snippet

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/TxBuilderExtension.sol#L380

Tool used

Manual Review

Recommendation

Approve 0 amount first.

IERC20(pToken).safeApprove(address(ironBank), 0); 
IERC20(pToken).safeIncreaseAllowance(address(ironBank), amount);

[ibsunhub]

First, pToken is a standard ERC20 token so this issue is invalid. We use OZ's ERC20.sol to implement it.

Many issues relate to FlashLoan.sol about DoS of USDT (#13 #52 #63 #136 #241 #359 ). They're also invalid. The if condition here will only be entered in the first time when the allowance must be 0. Therefore, we can set the initial allowance to type(uint256).max by using safeApprove. After that, the allowance is basically unlimited. It's almost impossible to enter the if condition again. I ran a mainnet-fork test on this, all good.

[0xffff11]

Agree with the ptoken not being an issue. Though on the second point sponsor made, usually the solution should not be approving type(uint256).max due to possible future drain of approvals. @ibsunhub Thoughts?

[ibsunhub]

Yes, usually we shouldn't approve max to any smart contract for safety. However, the flashLoan adaptor here is not upgradable. If the review is done, this could save a lot of gas consumption for users since only the first user needs to approve for the flashLoan adaptor. Also, the approval is made from the flashLoan contract to the ironBank contract. Both contracts are controlled by us and normally there should be no funds in the flashLoan contract to be drained.

[0xffff11]

In this edge case approving max should be fine because it is inside the same codebase. But always take extreme care for approving max

[0xffff11]

Invalid