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

Incorrect approve usage in `TokenManager::_transfer` and Missing Approve Call in `TokenManager::withdraw`

jesjupyter

Summary

The CapitalPool::approve function is intended to approve the tokenAddr for maximum allowance to the tokenManager. However, in the TokenManager::_transfer function, an incorrect argument address(this) is passed to ICapitalPool(_capitalPoolAddr).approve, which should actually be _token. This mistake causes the approval to revert unexpectedly. Furthermore, the TokenManager::withdraw function fails to call approve when _tokenAddress != wrappedNativeToken when the allowance is 0, potentially leading to issues when CapitalPool::approve has not been called for this _tokenAddress before.

Vulnerability Details

Issue 1: Incorrect Argument in TokenManager::_transfer

The CapitalPool::approve function receives tokenAddr and calls tokenAddr.approve(tokenManager, type(uint256).max):

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

However, in the TokenManager::_transfer function, the following code calls ICapitalPool(_capitalPoolAddr).approve(address(this)):

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

In this code, the address(this) argument passed to approve is incorrect; it should be _token instead. Passing address(this) as the argument results in the approval being called on the wrong address, which will not allow the tokenManager to spend tokens as intended. This mistake causes the approval process to fail and the transaction to revert unexpectedly.

The PoC is shown below:

function test_approve_fails() public {
test_ask_offer_turbo_eth();
vm.prank(user);
tokenManager.withdraw(address(weth9), TokenBalanceType.SalesRevenue);
}

The log shows the approve call fails since there is no approve in the contract TokenManager.

...
WETH9::allowance(UpgradeableProxy: [0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0], UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]) [staticcall]
│ │ │ └─ ← [Return] 0
│ │ ├─ [9999] UpgradeableProxy::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39])
│ │ │ ├─ [4983] CapitalPool::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]) [delegatecall]
│ │ │ │ ├─ [534] TadleFactory::relatedContracts(5) [staticcall]
│ │ │ │ │ └─ ← [Return] UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39]
│ │ │ │ ├─ [708] UpgradeableProxy::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ │ │ │ │ ├─ [192] TokenManager::approve(UpgradeableProxy: [0x6891e60906DEBeA401F670D74d01D117a3bEAD39], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77]) [delegatecall]
│ │ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ │ └─ ← [Revert] EvmError: Revert
│ │ │ │ └─ ← [Revert] ApproveFailed()
│ │ │ └─ ← [Revert] ApproveFailed()
│ │ └─ ← [Revert] ApproveFailed()
│ └─ ← [Revert] ApproveFailed()
└─ ← [Revert] ApproveFailed()

Issue 2: Missing Allowance Check and Approval Call in TokenManager::withdraw

In the TokenManager::withdraw function, the approve function is not called when _tokenAddress != wrappedNativeToken. The code directly attempts to transfer tokens from the capitalPoolAddr without checking if the CapitalPool::approve has been called for the specific _tokenAddress to have sufficient allowance.

if (_tokenAddress == wrappedNativeToken) {
@=> _transfer( // Check and Call Approve Here
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
...
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
@=> _safe_transfer_from( // No check and No approve
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}

Impact

  1. Incorrect Input Parameter: The incorrect input parameter address(this) in the TokenManager::_transfer function causes the approval process to fail, leading to unexpected reverts during token transfers.

  2. Missing Allowance Check: The lack of an allowance check in the TokenManager::withdraw function for tokens that are not wrappedNativeToken can result in failed withdrawals due to insufficient allowance, especially if CapitalPool::approve has not been called for the specific token.

Tools Used

Manual, Foundry

Recommendations

  • Modify the TokenManager::_transfer function to pass the correct _token parameter to the approve function, ensuring that the approval is correctly set up for the intended token:

  • Ensure that the approve function is also called within TokenManager::withdraw when _tokenAddress != wrappedNativeToken. This can be done by checking the allowance and approving the necessary amount before performing the token transfer.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby 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. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, 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. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.