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

Native token withdrawal can fail with contract wallets

izcoser

Summary

TokenManager uses transfer() to transfer the native token to the user address. If the user is a smart contract and it consumes more than 2300 gas on the receive callback, it might have its funds locked forever.

Vulnerability Details

Suppose a user is using a smart contract account, for example:

import {PreMarktes} from "../../src/core/PreMarkets.sol";
import {TokenManager} from "../../src/core/TokenManager.sol";
import {CreateOfferParams} from "../../src/interfaces/IPerMarkets.sol";
import {GenerateAddress} from "../../src/libraries/GenerateAddress.sol";
import {TokenBalanceType} from "../../src/interfaces/ITokenManager.sol";
​
contract BadReceiver{
function createAndCloseOffer(address preMarktes, CreateOfferParams calldata params) external{
PreMarktes(preMarktes).createOffer{value: 0.012 * 1e18}(params);
​
address offerAddr = GenerateAddress.generateOfferAddress(0);
address stockAddr = GenerateAddress.generateStockAddress(0);
​
PreMarktes(preMarktes).closeOffer(stockAddr, offerAddr);
}
​
function withdraw(address tokenManager, address weth9) external{
TokenManager(tokenManager).withdraw(weth9, TokenBalanceType.MakerRefund);
}
​
receive() external payable {
for(uint i = 0; i < 10000; i++){
i += i;
}
}
}

When he tries to withdraw his funds from the protocol, TokenManager will call on TokenManager.sol, line 169:

payable(msg.sender).transfer(claimAbleAmount);

Our contract wallet cannot receive that with only 2300 gas.

We can check with a test:

function test_contract_wallet_might_lock_funds() public {
BadReceiver badReceiver = new BadReceiver();
deal(address(badReceiver), INITIAL_TOKEN_VALUE);
deal(user, INITIAL_TOKEN_VALUE);
​
vm.startPrank(user);
badReceiver.createAndCloseOffer(address(preMarktes), CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
));
​
capitalPool.approve(address(weth9));
​
badReceiver.withdraw(address(tokenManager), address(weth9));
console.log(address(badReceiver).balance);
vm.stopPrank();
}

The result is:

│ │ │ ├─ [2300] BadReceiver::receive{value: 12000000000000000}()
│ │ │ │ └─ ← [OutOfGas] EvmError: OutOfGas
│ │ │ └─ ← [Revert] EvmError: Revert

Impact

Users with smart wallets might have their funds frozen until the protocol decides to upgrade its implementation. Because funds are directly at risk, I'd say this is a HIGH issue.

Tools Used

Forge

Recommendations

Swap line 169 in TokenManager.sol for:

(bool sent, bytes memory data) = payable(msg.sender).call{value: claimAbleAmount}("");
require(sent, "Failed to send Ether");

That way, the receiving contract will be able to do whatever he needs to, as long as he pays the gas fee. Also swapping all occurrences of transfer to call might be worth looking into with caution, as they might introduce reentrancy vulnerabilities.

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.