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

`TokenManager.withdraw()` doesn't approve for standard ERC20 tokens.

bladesec

Summary

TokenManager.withdraw() doesn't approve for standard ERC20 tokens.

Vulnerability Details

In withdraw(), it calls _safe_transfer_from() for standard ERC20 tokens which transfers tokens from CapitalPool to the caller.

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
//
​
if (_tokenAddress == wrappedNativeToken) {
//
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
//
}
​
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

But as it didn't approve this transfer using approve(), it will keep reverting.

Impact

Users can't withdraw ERC20 tokens because _safe_transfer_from() reverts.

Tools Used

Manual Review

Recommendations

withdraw() should use _transfer() which approves if necessary instead of _safe_transfer_from().

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
//
​
if (_tokenAddress == wrappedNativeToken) {
//
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
- _safe_transfer_from(
- _tokenAddress,
- capitalPoolAddr,
- _msgSender(),
- claimAbleAmount
- );
+ _transfer(
+ _tokenAddress,
+ capitalPoolAddr,
+ _msgSender(),
+ claimAbleAmount,
+ capitalPoolAddr
+ );
}
//
}
Updates

Lead Judging Commences

0xnevi Lead Judge about 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.