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

Miss CapitalPool's approval when withdraw ERC20 tokens.

p0wd3r

Summary

Miss CapitalPool's approval when withdraw ERC20 tokens.

Vulnerability Details

The documentation for CapitalPool.approve states that approve can only be called by TokenManager.

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

/**
* @dev Approve token for token manager
* @notice only can be called by token manager
* @param tokenAddr address of token
*/
function approve(address tokenAddr) external {

In TokenManager, only _transfer can call CapitalPool.approve.

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

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

During withdraw, if the token is not a wrapped native, it will directly call _safe_transfer_from to transfer assets from the CapitalPool, instead of calling _transfer.

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

/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

Therefore, withdraw cannot call CapitalPool.approve, and without CapitalPool.approve, withdraw cannot be completed.

Impact

withdraw cannot call CapitalPool.approve, and without CapitalPool.approve, withdraw cannot be completed.

Due to the current implementation of approve being inconsistent with the documentation and lacking verification of msg.sender, it is possible to execute approve first and then withdraw separately. However, this does not meet the requirements of the documentation and prevents withdraw from being completed in a single transaction, so I classify it as a medium risk.

Tools Used

vscode

Recommendations

Check approval and call CapitalPool.approve before _safe_transfer_from

Updates

Lead Judging Commences

0xnevi Lead Judge over 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.

Give us feedback!