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

MultiSig Wallets Can't Receive Native ETH Due To The `transfer()` Function Gas Constraint In The `TokenManager.sol::withdraw()` Function Leaving The Funds To Be Stuck In The `CaptialPool.sol` Contract

Fortis Audits

Summary

In the provided TokenManager.sol contract, the withdraw function is responsible for handling withdrawals of both ERC20 tokens and the native token (e.g., Ether on Ethereum). For native token withdrawals, the contract uses the payable(msg.sender).transfer(claimAbleAmount) method to transfer the funds to the caller.

Vulnerability Details

The transfer function is a low-level call that automatically forwards 2300 gas to the recipient (msg.sender) when transferring native tokens (e.g., ETH). However, transfer() only forwards 2300 gas, which is not enough for the recipient to execute any non-trivial logic in a receive() or fallback function. For instance, it is not enough for Safes (such as this one) to receive funds, which require more than 6k gas for the call to reach the implementation contract and emit an event.

Code Snippet

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

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

  • Denial of Service: If a multisig wallet tries to withdraw native tokens using this withdraw function, the transaction may fail due to insufficient gas being forwarded by the transfer method. This can lead to a denial of service where legitimate users are unable to withdraw their funds from the contract.

  • Loss of Funds: If the withdrawal process fails, users may not be able to retrieve their funds, potentially locking them in the contract indefinitely unless the contract is modified.

Proof Of Concept

  1. Lets say alice uses a multisig wallet.

  2. Alice deposits some collateral amount for particiapting in the trade.

  3. Alice uses native ETH for the trade as collateral.

  4. After the trade gets over alice tries to withdraw the collateral amount.

  5. The withdraw function will fail because the multisig wallet will not be able to receive the native ETH due to the gas constraint in the transfer function.

Proof Of Code

For testing the POC, run the following command:

forge test --mt test_FortisAudits_MultiSigWallets_CannotRecive_TheirFundsBack -vvvv

Add the following code to the PreMarkets.t.sol contract:

function test_FortisAudits_MultiSigWallets_CannotRecive_TheirFundsBack() public {
MultiSigWallet alice_wallet = new MultiSigWallet();
uint256 amount = 100;
deal(address(preMarktes), amount);
​
// After some operations alice's ETH funds are added to her balance mapping
vm.startPrank(address(preMarktes));
tokenManager.tillIn{value: amount}(address(alice_wallet), address(weth9), amount, false);
tokenManager.addTokenBalance(TokenBalanceType.RemainingCash, address(alice_wallet), address(weth9), amount);
vm.stopPrank();
​
vm.prank(address(capitalPool));
capitalPool.approve(address(weth9));
​
vm.expectRevert();
vm.startPrank(address(alice_wallet));
tokenManager.withdraw(address(weth9), TokenBalanceType.RemainingCash);
vm.stopPrank();
assertEq(address(alice_wallet).balance, 0);
assertEq(weth9.balanceOf(address(capitalPool)), amount);
}
​
contract MultiSigWallet {
event Received(address indexed sender, uint256 value);
​
receive() external payable {
// MultiSigWallets needs more than 6000 gas to emit event
uint256 gas = gasleft();
require(gas > 6000, "Not engough gas to emit");
emit Received(msg.sender, msg.value);
}
}

Tools Used

  • Manual Review

  • Foundry

Recommendations

To mitigate this vulnerability, consider replacing transfer with the call method, which allows you to specify the gas limit. This would ensure compatibility with contracts (like multisig wallets) that require more gas for their operations.

Here's the updated code snippet:

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);
+ (bool success, ) = payable(msg.sender).call{value: claimAbleAmount}("");
+ require(success, "Transfer failed");
} 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);
}
Updates

Lead Judging Commences

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