Relevant GitHub Links
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L70
https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L169
Summary
In Solidity, it's generally recommended to avoid using the transfer function to send Ether, and instead, use the call function.
Vulnerability Details
use of payable(msg.sender).transfer in TokenManager::withdraw and payable(to).transfer(amount) in Rescuable::rescue, for transfering ETH makes the application incompatible with smart contract wallets.
In TokenManager contract:
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
...
if (_tokenAddress == wrappedNativeToken) {
_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
);
}
...
}
In Rescuable contract:
function rescue(
address to,
address token,
uint256 amount
) external onlyOwner {
if (token == address(0x0)) {
@> payable(to).transfer(amount);
} else {
_safe_transfer(token, to, amount);
}
emit Rescue(to, token, amount);
}
Impact
The user can't withdraw ( TokenManager.withdraw ) or rescue ( Rescuable.rescue ) his funds to smart contract wallets.
Tools Used
Manual analysis
Recommendations
use (bool success, ) =payable(address).call{value: _amount}("") in place of payable(àddress).transfer to transfer eth token from one address to and other.
In Rescuable contract:
function rescue(
address to,
address token,
uint256 amount
) external onlyOwner {
if (token == address(0x0)) {
- payable(to).transfer(amount);
+ (bool success, ) =payable(to).call{value: amount}("");
} else {
_safe_transfer(token, to, amount);
}
emit Rescue(to, token, amount);
}
In TokenManager contract:
function withdraw(
address _tokenAddress,
TokenBalanceType _tokenBalanceType
) external whenNotPaused {
...
if (_tokenAddress == wrappedNativeToken) {
_transfer(
wrappedNativeToken,
capitalPoolAddr,
address(this),
claimAbleAmount,
capitalPoolAddr
);
IWrappedNativeToken(wrappedNativeToken).withdraw(claimAbleAmount);
- payable(msg.sender).transfer(claimAbleAmount);
+ (bool success, ) =payable(msg.sender).call{value: claimAbleAmount}("");
} else {
/**
* @dev token is ERC20 token
* @dev transfer from capital pool to msg sender
*/
_safe_transfer_from(
_tokenAddress,
capitalPoolAddr,
_msgSender(),
claimAbleAmount
);
}
...
}
