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

Incorrect Token Address Passed to approve Function Leading to Transfer Failures

hunter_w3b

Summary

The _transfer function in the TokenManager contract incorrectly passes the address(this) as the argument to the approve function of the ICapitalPool contract instead of the ERC20 token address. This could lead to the incorrect approval of token transfers, resulting in failed transfers.

Vulnerability Details

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
@>>> ICapitalPool(_capitalPoolAddr).approve(address(this));
}

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/TokenManager.sol#L243-L248

The approve function of the CapitalPool contract is designed to approve the token manager to spend a specified ERC20 token. However, in the _transfer function, the address(this) is passed to the approve function instead of the ERC20 token address (_token). This incorrect argument means the approval process is not executed for the intended token, which could prevent the expected transfer from occurring.

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

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/core/CapitalPool.sol#L24-L39

Impact

It calls ICapitalPool(_capitalPoolAddr).approve(address(this));. This passes the address(this) (referring to the TokenManager contract itself) instead of the ERC20 token address (_token). The approval might not be correctly set up for the intended ERC20 token, leading to a failed transfer when attempting to move tokens from the _capitalPoolAddr.

Tools Used

  • Manual code review

Recommendations

Replace address(this) with the correct ERC20 token address (_token) when calling the approve function.

if (
_from == _capitalPoolAddr &&
IERC20(_token).allowance(_from, address(this)) == 0x0
) {
ICapitalPool(_capitalPoolAddr).approve(_token); // Correct usage
}

This change ensures that the correct token address is passed to the approve function, thereby allowing the CapitalPool contract to set up the necessary approvals for token transfers as intended.

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.