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

[H-3] `TokenManager::withdraw` will not work for Native Tokens causing stuck funds for users!

nilay27

Description:

In TokenManager::withdraw, the users can call this functions to withdraw their pending claimable amounts updated via addTokenBalance() which is called by both DeliveryPlace and PreMarkets
as it keeps track of balance for users in the CapitalPool corresponding to whether the income is for the following:

enum TokenBalanceType {
TaxIncome,
ReferralBonus,
SalesRevenue,
RemainingCash,
MakerRefund,
PointToken
}

However, the function has a critical vulnerability that causes the funds to be stuck in the CapitalPool in case if these balances are in native token

Proof of concept:

In withdraw we have the following lines of code:

if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,//token
capitalPoolAddr, //from
address(this),// to
claimAbleAmount,// amount
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
}

Where the _transfer method is internal and contains the following logic:

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
/**
* @audit high: this will not work
* ICapitalPool.approve() expects the argument as token address
* we need to pass token address, as the approval automaticall
* gives approval for TokenManager only.
* withdrawal will always fail!
*/
ICapitalPool(_capitalPoolAddr).approve(address(this));
}

The issue is that this method passes the address(this) that is the address of the TokenManager as argument to ICapitalPool.approve(), which expects the input to be a token address.

For reference:

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

We can clearly see that the expected address is ERC20 token address as we are calling approve on it.
This will always fail in case of nativeToken withdrawal!

Impact:

The users who are trading in native token will have their funds locked!

Recommended Mitigation:

Change the code to following:

- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);
Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-approve-wrong-address-input

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. The argument up in the air is since the approval function `approve` was made permisionless, the `if` block within the internal `_transfer()` function will never be invoked if somebody beforehand calls approval for the TokenManager for the required token, so 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.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.