Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Lack of Automated Approval in `TokenManager::withdraw` Prevents User Withdrawals

0xdarko

Summary

The TokenManager::withdraw function does not automatically call CapitalPool::approve, potentially preventing users from withdrawing funds from CapitalPool. This issue becomes critical once CapitalPool::approve access control is correctly restricted to be called only by TokenManager.

Vulnerability Details

  1. CapitalPool::approve should only be callable by TokenManager for security reasons.

  2. TokenManager::withdraw uses Rescuable::_safe_transfer_from without ensuring sufficient allowance or increasing it.

Impact

  1. Users will be unable to withdraw their funds until the owner of TokenManager manually calls CapitalPool::approve on that asset

  2. This can lead to locked funds and user frustration, especially in time-sensitive situations.

  3. It introduces a centralization risk, as withdrawals become dependent on manual intervention.

Tools Used

Foundry

Recommendations

  1. Implement an automatic approval mechanism in TokenManager::withdraw:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
// .. OTHER CODE .. //
​
} else {
+ uint256 currentAllowance = IERC20(_tokenAddress).allowance(capitalPoolAddr, address(this));
+ if (currentAllowance < claimAbleAmount) {
+ ICapitalPool(capitalPoolAddr).approve(_tokenAddress);
}
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year 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.

Give us feedback!