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

Flawed approval mechanism leads to failed transfers and potential fund lock

0x5chn0uf

Summary

The CapitalPool contract contains an approve function that approves the token manager to spend an unlimited amount of tokens. This function is only called from within the TokenManager::_transfer function under certain conditions.

However, the implementation is flawed in several ways:

  • The check in _transfer calls CapitalPool::approve, passing address(this) (the calling contract's address) instead of the token address.

  • The approve function lacks access control, allowing any address to call it.

  • The check in _transfer only occurs when the allowance is exactly zero, missing cases where the allowance is insufficient but non-zero.

  • There is no check if tokenAddr is indeed a ERC20 token, allowing an attacker to pass a malicious contract with the same selector.

Vulnerability Details

In TokenManager.sol:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
// ...
​
if (
_from == _capitalPoolAddr &&
@> IERC20(_token).allowance(_from, address(this)) == 0x0 // @audit - Exactly 0? Not < _amount ?
// What if _capitalPoolAddr got an allowance elsewhere?
) {
@> ICapitalPool(_capitalPoolAddr).approve(address(this));
// @audit - This is passing the TokenManager contract address, not the token address
}
​
// ...
}
​

And the called function in CapitalPool.sol:

function approve(address tokenAddr) external { // @audit - External, no access control. Reentrancy? A call() is made
// No check on tokenAddr, what if I pass a malicious contract?
// ...
// @audit - We call tokenAddr. But address(this) was passed,
// i.e. the address of the TokenManager.
@> (bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max // @audit - Unlimited allowance?
)
);
​
// ...
}

The current implementation violates the principle of least privilege by granting unlimited approvals (type(uint256).max) to the token manager. This approach unnecessarily increases the potential impact of any security breach. By allowing unlimited token spending, it exposes the contract to greater risk if the token manager is compromised or contains vulnerabilities.

A more secure approach would be to approve only the specific amount needed for each transaction, limiting the potential damage in case of an exploit.

Impact

This vulnerability can lead to several high-severity issues:

  1. Failed Transfers: Transfers from the capital pool will fail when the allowance is zero, as the approval call will not succeed due to incorrect parameters.

  2. Locked Funds: Tokens in the capital pool may become inaccessible if the approval mechanism fails and there's no other way to approve or transfer tokens.

  3. Unauthorized Approvals: Any address can call the approve function, potentially leading to unauthorized token approvals if exploited.

Tools Used

Manual review

Recommendations

  • Redesign the approval mechanism:

    • Remove the CapitalPool contract. It has no use appart from storing the tokens. Send them to the TokenManager instead, it will simplify the protocol’s design and potential attack surfaces.

  • Before a transfer, wrap the _token in an IERC20 interface and use the approve method to safely set the required allowances. This change ensures that the contract does not accidentally interact with non-standard tokens and correctly handles all token operations.

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
// ...
​
+ bool approved = IERC20(_token).approve(address(this), _amount);
+ if (!approved) {
+ revert ApprovalFailed();
+ }
​
_safe_transfer_from(_token, _from, _to, _amount);
​
// ...
}

  • Correct the _transfer function:

    • Remove the current approve call

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-CapitalPool-approve-missing-access-control

This is at most low severity, even though giving max approvals shouldn't be permisionless, the respective tokenManager address is retrieved from the TadleFactory contract whereby the trusted guardian role is responsible for deploying such contracts as seen [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/factory/TadleFactory.sol#L68). Since the user still has to go through the PreMarkets/DeliveryPlace contracts to perform market actions, this max approval cannot be exploited.

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.