Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Allowance issue in transfer function

aresaudits

Summary

Vulnerability Details

In the TokenManager.sol file, the _transfer function contains the following logic:

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​

The current logic only checks if the allowance is exactly zero. If the allowance is greater than zero but less than the amount to be transferred, the function will not call ICapitalPool(_capitalPoolAddr).approve(address(this));, and the subsequent _safe_transfer_from call will fail due to insufficient allowance, causing the function to revert.

Example Scenario

  1. Initial Setup:

    • _token: Address of the ERC20 token.

    • _from: Address of the sender (e.g., 0xSender).

    • _to: Address of the recipient (e.g., 0xRecipient).

    • _amount: Amount to be transferred (e.g., 100 tokens).

    • _capitalPoolAddr: Address of the capital pool (e.g., 0xCapitalPool).

  2. Allowance State:

    • Assume the current allowance of 0xSender to TokenManager is 50 tokens.

  3. Transfer Attempt:

    • The function _transfer is called with the above parameters.

    • The if condition checks if the allowance is exactly zero. Since the allowance is 50, the condition fails.

    • The function proceeds to call _safe_transfer_from, which attempts to transfer 100 tokens.

    • Since the allowance is only 50, the transfer fails and the function reverts.

Additional Scenario

Also, if you think the approve will be set to the maximum value (type(uint256).max), consider that some standard ERC20 tokens do not allow setting the allowance to type(uint256).max. For example, if the allowance is set to 100 tokens and subsequent transactions reduce the allowance to 50 tokens, any further attempts to transfer more than 50 tokens will fail. If a user calls the withdraw function under these conditions, the allowance check will not trigger the approve function, causing the withdraw function to always revert.

Impact

Transfer function may revert frequently

Tools Used

Recommendations

To handle the case where the allowance is greater than zero but less than the amount to be transferred, the if condition should be updated to check if the allowance is less than the amount to be transferred

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby If we consider the correct permissioned implementation for the `approve()` function within `CapitalPool.sol`, this would be a critical severity issue, because the withdrawal of funds will be permanently blocked and must be rescued by the admin via the `Rescuable.sol` contract, given it will always revert [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L36-L38) when attempting to call a non-existent function selector `approve` within the TokenManager contract. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, the transfer will infact not revert when a withdrawal is invoked. I will leave open for escalation discussions, but based on my first point, I believe high severity is appropriate. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.