Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Potential Dangerous Transfer in `withdraw` Function Due to Use of `transfer()`

0xbeastboy

Github

Summary

The TokenManager:withdraw function currently uses transfer() method to send ETH to msg.sender. This method, while simple and widely used, have significant risks and limitations due to its hard-coded gas cost of 2300 gas units.

Vulnerability Details

The use of transfer() can lead to transaction failures under certain conditions, particularly when interacting with contracts that have more complex fallback or receive functions.

See the following code:

/**
* @notice Withdraw
* @dev Caller must be owner
* @param _tokenAddress Token address
* @param _tokenBalanceType Token balance type
*/
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

The use of transfer() in the withdraw function can result in several issues, including:

  • The 2300 gas units provided by transfer() are often insufficient for complex operations. If the recipient contract's fallback or receive function requires more than 2300 gas, the transaction will fail, causing the ETH transfer to be reverted.

  • Even if a fallback function typically consumes less than 2300 gas, interactions through a proxy contract may increase gas consumption, leading to failed transactions.

  • Some multisig wallets require more than 2300 gas units to execute transactions, which makes transfer() incompatible with these wallets.

Additional details can be found in the ConsenSys blog post Stop Using Solidity’s transfer() Now, which highlights the dangers of relying on transfer().

Tools Used

Manual Review

Recommendations

By using call() or a well-audited library like OpenZeppelin’s Address.sendValue, these risks can be mitigated, ensuring more reliable and secure ETH transfers.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

[invalid] finding-TokenManager-withdraw-transfer-2300-gas

Invalid, known issues [Medium-2](https://github.com/Cyfrin/2024-08-tadle/issues/1)

Support

FAQs

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

Give us feedback!