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

[M-2] `TokenManger::withdraw` doesn't check for before and after balances of ERC20 tokens that return false instead of reverting, causing user balance to be lost.

karanel

Relevant Links

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

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

https://github.com/Cyfrin/2024-08-tadle/blob/main/src/utils/Rescuable.sol#L104-L117

Summary

TokenManger::withdraw calls _transfer function for WETH tokens but uses _safe_transfer_from function for other ERC20 tokens.
In case of ERC20 tokens that return false instead of reverting on transfer failure like ZRX, it doesn't check the before and after balances, causing user balances to be incorrectly marked as withdrawn.

Vulnerability Details

TokenManger::_transfer function checks for approvals as well as before and after balances of the transferred token.

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));
}
​
_safe_transfer_from(_token, _from, _to, _amount);
​
uint256 fromBalanceAft = IERC20(_token).balanceOf(_from);
uint256 toBalanceAft = IERC20(_token).balanceOf(_to);
​
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
​
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}

Rescuable::_safe_transfer_from function only checks the return value of low level call function. If transfer for a token like ZRX was successful, bool success would have a value of true. However, if the transfer failed, bool success would still be true.

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

As we can see below in the TokenManager::withdraw function, when any other ERC20 except for WETH is transferred Rescuable::_safe_transfer_from is used instead of TokenManager::_transfer:

function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
uint256 claimAbleAmount = userTokenBalanceMap[_msgSender()][
_tokenAddress
][_tokenBalanceType];
​
if (claimAbleAmount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @dev transfer from capital pool to msg sender
* @dev withdraw native token to token manager contract
* @dev transfer native token to msg sender
*/
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
​
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
payable(msg.sender).transfer(claimAbleAmount);
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
​
emit Withdraw(
_msgSender(),
_tokenAddress,
_tokenBalanceType,
claimAbleAmount
);
}

Impact

Likelihood: High

Impact: Medium - This would cause users balance to be lost and tracked as withdrawn even though they weren't able to withdraw tokens like ZRX. However this would only exist after a valid fix for [H-1] userTokenBalanceMap is never updated while withdrawing, causing protocol to be drained. is applied.

Overall severity is Medium.

Tools Used

Manual Review

Recommendations

Use TokenManager::_transfer inside TokenManager::withdraw instead of Rescuable::_safe_transfer_from

Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-weird-erc-20-return-boolean-Rescuable

I believe the issues and duplicates do not warrant low severity severity as even if the call to transfers returns false instead of reverting, there is no impact as it is arguably correct given there will be insufficient funds to perform a rescue/withdrawal. This will not affect `tillIn()` as there are explicit balance [checks that revert accordingly](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L255-L260) to prevent allowing creation of offers without posting the necessary collateral

Appeal created

karanel Submitter
10 months ago
0xnevi Lead Judge
10 months ago
karanel Submitter
10 months ago
0xnevi Lead Judge
10 months ago
karanel Submitter
10 months ago
0xnevi Lead Judge
10 months ago
karanel Submitter
10 months ago
0xnevi Lead Judge
10 months ago
karanel Submitter
10 months ago
0xnevi Lead Judge 10 months 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.