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

Faulty approval call in `TokenManager.sol`

0xshoonya

Summary

The approve function in the CapitalPool contract is designed to approve the TokenManager contract to spend a specific token (tokenAddr). However, in the _transfer function of the TokenManager contract, it incorrectly calls the approve function on the CapitalPool contract, passing address(this) (the TokenManager's address) instead of the token address.

Vulnerability Details

The approvefunction in CapitalPool.sol is designed to approve the TokenManager to spend a specific token (tokenAddr).

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

Now let's look at the _transfer function in the TokenManager contract. In the _transfer function, when the _from address is the CapitalPool and the allowance is zero, it attempts to get approval:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
@> ICapitalPool(_capitalPoolAddr).approve(address(this));
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
// ... (balance checks)
}

It calls approve on the CapitalPool contract, but passes address(this) (the TokenManager's address) instead of the token address, which is incorrect. CapitalPool expects the address of the token to be approved, not the address of the contract calling it.
As a result, the actual ERC20 tokens that need to be approved for the TokenManager to spend from the CapitalPool are never getting approved.

Impact

It would likely result in failed transfers and could potentially lock funds in the CapitalPool contract.

Tools Used

Recommendations

The correct implementation should be:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
uint256 fromBalanceBef = IERC20(_token).balanceOf(_from);
uint256 toBalanceBef = IERC20(_token).balanceOf(_to);
​
if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
- ICapitalPool(_capitalPoolAddr).approve(address(this));
+ ICapitalPool(_capitalPoolAddr).approve(_token);
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
// ... (balance checks)
}
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.