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

TokenManager tries to approve itself, instead of the token, making _transfer function broken

Audittens

Summary

TokenManager calls approve function on CapitalPool with wrong arguments, leading to a freeze of funds/broken logic.

Vulnerability Details

In CapitalPool, there is a function approve that should only be callable by TokenManager, according to the @notice comment. Currently, it is callable by anyone. There are two possible outcomes: first, if this gets fixed and only TokenManager can call approve, and second, if it is not fixed and anyone can call approve on CapitalPool.

/**
* @dev Approve token for token manager
* @notice only can be called by token manager
* @param tokenAddr address of token
*/
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();
}
}

TokenManager calls this function in the _transfer function, specifically when _from is CapitalPool and the allowance of _token has not yet been set.

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

As we can see, the argument for approve is set to address(this), which means that CapitalPool will attempt to call TokenManager instead of _token. Since TokenManager does not have a function with a selector matching APPROVE_SELECTOR, this will result in a failure, preventing the use of _transfer until the allowance is set.

As mentioned earlier, the current code allows anyone to call the approve function in CapitalPool with any input. This means it is possible to set the allowance for TokenManager, temporarily freezing the funds. However, if the code is fixed, it will no longer be possible to fix _transfer from outside, rendering both TokenManager and CapitalPool unusable.

Impact

There are two possible scenarios:

  1. The approve function of CapitalPool is fixed (accordingly to how it's documented to behave) so that only the TokenManager can call it. In this case, the _transfer functionality becomes broken and unusable, without fix from outside. Since the TokenManager relies heavily on _transfer, this affects the integrity of the entire system.

  2. The approve function of CapitalPool is not fixed, allowing anyone to call it externally. This would allow the _transfer functionality to be fixed, so it results in a temporary freeze of funds.

Tools Used

Manual Review.

Recommendations

Fix call to CapitalPool in TokenManager.

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

This is the expected behavior of the _transfer function, and the fix applies to both cases: whether approve is fixed or left unchanged.

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.