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

`address(this)` is passed to capitalPool.approve() instead of `token` address, breaking the withdraw() functionality

0xaraj

Summary

address(this) is passed to capitalPool.approve() instead of token address in tokenManager:_transfer(), breaking the withdraw() functionality

Vulnerability Details

When a user withdraw his tokens from capitalPool using tokenManager:withdraw(), it calls _transfer() under the hood. if `_from = capitalPool` then it approves tokenAmount to tokenManager.

function withdraw(address _tokenAddress, TokenBalanceType _tokenBalanceType) external whenNotPaused {
...
if (_tokenAddress == wrappedNativeToken) {
@> _transfer(wrappedNativeToken, capitalPoolAddr, address(this), claimAbleAmount, capitalPoolAddr);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(_tokenAddress, capitalPoolAddr, _msgSender(), claimAbleAmount);
}
}
function _transfer(address _token, address _from, address _to, uint256 _amount, address _capitalPoolAddr)
internal
{
...
if (_from == _capitalPoolAddr && IERC20(_token).allowance(_from, address(this)) == 0x0) {
@> ICapitalPool(_capitalPoolAddr).approve(address(this));
}
...
}

Now the problem is, capitalPool.approve() expects token address but address(this) is passed.

Impact

tokenManager:withdraw() will be broken. As result no user will be able to withdraw

Tools Used

Manual Review

Recommendations

Pass token address instead of address(this)

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

Lead Judging Commences

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