Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`TokenManager::_transfer` calls the capital pool `approve` function not correctly

marswhitehacker

Summary

TokenManager::_transfer calls ICapitalPool(_capitalPoolAddr).approve(address(this)) with the wrong parameter, making it revert

Vulnerability Details

approve function inside CapitalPool accepts the only argument - the token address. After this, this token address is used to approve tokens from CapitalPool to the TokenManager:

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();
}
}

So TokenManager::_transfer the function should call this approve function with the token address, not with the TokenManager address. It will revert due to TokenManager not having the approve function.
Links:
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L243-L248

Impact

TokenManager withdrawal will revert if made with wrappedNativeToken

Tools Used

Manual review

Recommendations

Pass correct argument - token address:

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

Lead Judging Commences

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