`withdraw` function will not work for tokens other than wrappedNativeToken, as direct transfers are done from CapitalPool without approval
Summary
withdraw function allows users to withdraw their token which are accumulated in CapitalPool, and the tokens are transferred from the CapitalPool to the user by getting an approval from CapitalPool.
But in the else block of withdraw function which deals with non-native token, direct transfers are performed from capital pool to user without doing approval from CapitalPool leading to a revert.
Vulnerability Details
The vulnerability is present in the
withdrawfunction else statement where it tries to perform transfers from capital pool to users without getting an approval of tokens from capital pool for token manager, leading to failed txn and no withdrawal can be made.The Capital Pool has an
approvefunction which allows approvals of tokens to the token manager, and that is expected to be called before making any transfer, but as it is not called the approved amount would be 0, thus leading to a failed withdrawal.
The vulnerability arises due to 2 cases:
-
There is no access control on
approve, as a result of which even if there is an already approved amount to be spent by token manager, then a front-running malicious txn will make the approval to 0, leading to a revert. -
Consider the case where there is an access control, but as the function makes no call to CapitalPool to perform approvals to token manager, the txn will fail, as the approvals were 0 for that token from capital pool to token manager, leading to no withdrawal possible.
Impact
Withdrawal will revert due to no approval done or due to front-running.
Tools Used
Manual Review
Recommendations
Step 1: Make the approve function on CapitalPool to be only callable by token manager contract.
Step 2: Instead of calling _safe_transfer_from in else statement of withdraw function, call the below function as shown:
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.