Missing Approval Check in withdraw Function for ERC-20 Tokens Leading to Potential Transfer Failures
Summary
The withdraw function in the TokenManager contract is responsible for allowing users to withdraw their deposited tokens, whether they are ERC-20 tokens or native tokens (wrapped native tokens). However, the implementation for ERC-20 token withdrawals lacks an essential approval check and mechanism which can result in transfer failures under certain conditions.
Vulnerability Details
During the withdraw function execution, if the token being withdrawn is an ERC-20 token (not a wrapped native token), the function directly calls _safe_transfer_from to transfer tokens from the capitalPoolAddr to the user.
The call to _safe_transfer_from skips part of the _transfer function logic that ensures that the capitalPoolAddr contract has granted the TokenManager contract approval to transfer tokens on its behalf.
The logic has a condition if (_from == _capitalPoolAddr) that ensures the approval is done if the from== _capitalPoolAddr.This then implies that even if the _transfer function is called during the token deposits(tillIn function),The logic will still be skipped since in tillIn from==_accountAddress
The CapitalPool contract includes an external approve function thats supposed to allow the tokenManager max approval of tokens. The approve function according to the docs should be only called by the token Manager- A condition that is not enforced in the code. If at all the capitalPool must be called from tokenManager as the docs state that means it will not be possible to manually set the max approvals for the tokens to the token Manager.
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L24
If the capitalPoolAddr contract has not previously approved the TokenManager contract to transfer the specified ERC-20 tokens, the direct call to _safe_transfer_from in the withdraw function will fail causing the withdrawal process to fail
Impact
Without the necessary approval, users' funds could become inaccessible effectively being stuck in the contract.
Tools Used
Manual Review
Recommendations
Modify the withdraw function to include the allowance check and approval logic currently used in the _transfer function. This ensures that before any ERC-20 token transfer is attempted, the TokenManager contract has the necessary approval from the capitalPoolAddr contract.
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.