Inconsistent approval and withdraw logic can lead to user funds stuck.
Summary
Due to an inconsistency in the token approval process for different token types (e.g., WETH vs. other ERC20 tokens) and a mismatch between the NatSpec documentation and the actual code implementation, there is a risk that user funds could become stuck.
Vulnerability Details
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L24
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L137
The approve function in the provided code is intended to be called exclusively by the token manager contract. This is evident from the NatSpec comment (@notice only can be called by token manager). The function is designed to approve an unlimited allowance (type(uint256).max) of a specified token for the token manager.
However, the function lacks an Access Control List (ACL) or any other access control mechanism to enforce this restriction. Without such controls, any external contract or user could potentially call this function, leading to unintended or unauthorized approvals.
Apart from this issue,, there is a distinction in how the approval process is handled for WETH compared to other ERC20 tokens when interacting with the capitalPool in TokenManager contract
In the provided TokenManager contract, there is a distinction in how the approval process is handled for WETH (Wrapped Ether) compared to other ERC20 tokens when interacting with the capitalPool. For other ERC20 tokens, the approval process differs. There is no explicit call to the capitalPool to approve the transfer amount. Instead, the contract directly attempts to transfer the tokens using the _safe_transfer_from function.
Impact
Given the issues described, there is currently a workaround that users can utilize to withdraw their tokens: they can explicitly call the capitalPool.approve() function themselves before attempting to withdraw. By doing this, users can manually grant the necessary allowance for the TokenManager to transfer their ERC20 tokens from the capitalPool.
However, this workaround is not how the implementation is supposed to function. The intended behavior is for the contract to manage token approvals internally
Tools Used
Manual review.
Recommendations
Implement ACL for the approve() function in CapitalPool.sol.
Add approval for the ERC20 tokens which are going to be transferred like it is done for WETH.
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.