Tadle

Summary

It is intended that withdraw function reverts the transaction if the CapitalPool contract does not have enough balance for the token transfer however it won't be failed even if CapitalPool does not have any token. Therefore the withdraw function will be executed succesfuly althought it is supposed to fail.

Vulnerability Details

As It is shown below withdraw function uses the _safe_transfer_from() method in order to send ERC20 tokens from CapitalPool to TokenManager contract.

The _safe_transfer_from method of Rescuable.sol() will do following low level call (bool success, ) = token.call( abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount) ) where TRANSFER_FROM_SELECTOR is bytes4 private constant TRANSFER_FROM_SELECTOR = bytes4(keccak256(bytes("transferFrom(address,address,uint256)"))) and check if it's return value false however the low level call function won't be reverted if CapitalPool has not enough funds to send. This issue can be reproduced by following test method :

Impact

Since low level transfer calls used in TokenManager::tillIn() as well ,TokenManager contract can work unintendedly and this may affect overall contract logic.

Tools Used

Manual analysis and Foundry

Recommendations

The problem can be fixed as implementing following dependencies to TokenManager.sol : SafeERC20 library of OpenZeppelin and ERC20 interface then changing this line to IERC20(_tokenAddress).safeTransferFrom(capitalPoolAddr, _msgSender(), claimAbleAmount). Additionaly changing all the low level transfer and transferFrom function calls with safeTransfer and safeTransferFrom would be better because the low level calls don't revert on failure if call isn't failed because of unsufficient gas or called function's revert.In case the called function does not exist or it simply returns a value,low level call returns true.Therefore in such situations following code snippets from Rescuable.sol won't revert even if transferFrom or transfer function does not work correctly.