TokenManager::withdraw Misuses Function Causing All ERC-20 Transfers to Revert
Summary
The TokenManager::withdraw function fails when attempting to transfer ERC-20 tokens due to a lack of allowance. While native token transfers are handled correctly, the function incorrectly uses _safe_transfer_from for ERC-20 tokens, causing transactions to revert.
issue Details
Native Token Handling:
-
When a user requests a withdrawal, the contract first checks if the token is a native token (e.g., Ether).
-
If it is a native token, the contract calls the
_transferfunction to process the transfer, which works correctly.
ERC-20 Token Handling:
-
If the token is an ERC-20 token, the contract calls
_safe_transfer_fromto transfer the tokens. -
The
_safe_transfer_fromfunction requires an allowance to be set for the contract to transfer tokens on behalf of thecapitalPool. -
Since the necessary allowance is not set, the
_safe_transfer_fromfunction reverts, causing the entire transaction to fail.
Impact
-
Users are unable to withdraw their ERC-20 tokens, leading to frustration and a lack of trust in the platform.
-
The inability to process ERC-20 withdrawals disrupts the overall functionality of the TokenManager, leading to potential financial and operational setbacks.
Tools Used
manual review, foundry
Recommendations
use the _transfer function instead of _safe_transfert_from
Lead Judging Commences
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
Prize pool breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.