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

Ether sent to `TokenManager::tillIn` when not calling with the `wrappedNativeToken` address results in lost of funds

0x5chn0uf

Summary

The TokenManager::tillIn function is marked as payable, allowing it to receive Ether. However, when the function is called with a non-native token address (i.e., an ERC20 token address that is not the wrappedNativeToken), any Ether sent with the transaction is not used or returned.

This Ether becomes permanently locked in the TokenManager contract, leading to a loss of funds for users.

Vulnerability Details

The root cause is that the tillIn function only handles the msg.value when the token address matches the wrappedNativeToken. For all other token addresses, it ignores any Ether sent with the transaction.

function tillIn(/* ... */) external payable // @audit - Can receive Ether
{
// ...
​
if (_tokenAddress == wrappedNativeToken) {
// ...
if (msg.value < _amount) { // @audit Correctly handled in this case
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}
} else {
//@audit What if msg.value is > 0 here?
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
​
// ...
}

Proof of Concept

Consider the following scenario based on the provided test:

  • A taker wants to create a taker position for an ERC20 token (MockedUSDC in this case).

  • The taker accidentally sends 1 Ether along with the createTaker transaction.

  • The createTaker function calls TokenManager::tillIn .

  • Since MockedUSDC is not the wrappedNativeToken, the Ether is ignored and becomes locked in the contract.

Proof of Code

function test_depositTokenWhenCreateTaker() public {
// Setup
address makerAsking = address(43);
address taker = address(42);
​
vm.prank(makerAsking);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
vm.prank(taker);
mockUSDCToken.approve(address(tokenManager), type(uint256).max);
​
deal(address(mockUSDCToken), taker, 1e18);
deal(address(mockUSDCToken), makerAsking, 1.2e18);
​
uint256 points = 500;
address offerAddr = GenerateAddress.generateOfferAddress(0);
// Create Ask Offer
vm.prank(makerAsking);
preMarktes.createOffer(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
1 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
​
deal(taker, 1 ether);
uint256 initialBalance = mockUSDCToken.balanceOf(taker);
vm.prank(taker);
preMarktes.createTaker{value: 1 ether}(offerAddr, points); // Create a take order with 1 ether
​
uint256 takerBalanceEther = taker.balance;
assertEq(takerBalanceEther, 0, "Taker's ether balance should be 0 after the transaction");
// Call did not revert! Taker just lost 1 ether
// The transaction went through for the mockUSDCToken
uint256 finalBalance = mockUSDCToken.balanceOf(taker);
uint256 makerAskingTokenBalance = tokenManager.userTokenBalanceMap(
makerAsking,
address(mockUSDCToken),
TokenBalanceType.SalesRevenue
);
​
assertTrue(initialBalance > finalBalance);
assertTrue(makerAskingTokenBalance > 0);
}
​

In this scenario, the taker loses 1 Ether, which becomes permanently locked in the TokenManager contract.

Impact

Users can permanently lose Ether by accidentally sending it when interacting with ERC20 tokens through functions that call tillIn , for example createTaker

This could lead to significant financial losses, especially if high-value transactions are involved.

Tools Used

Manual Review - Testing

Recommendations

Modify the TokenManager::tillIn function to revert if Ether is sent with a non-native token transaction:

​
​
function tillIn(/* ... */) external payable /* ... */ {
if (_amount == 0) {
return;
}
​
+ if (_tokenAddress != wrappedNativeToken && msg.value > 0) {
+ revert Errors.UnexpectedEther();
+ }
​
// ...
}
​

This change will prevent users from accidentally sending Ether when interacting with ERC20 tokens, thus avoiding the potential for locked funds.

Updates

Lead Judging Commences

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

[invalid] finding-TokenManager-tillin-excess

Invalid, these are by default, invalid based on codehawks [general guidelines](https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity#findings-that-may-be-invalid). The check implemented is simply a sufficiency check, it is users responsibility to only send an appropriate amount of native tokens where amount == msg.value when native token is intended to be used as collateral (which will subsequently be deposited as wrapped token). All excess ETH can be rescued using the `Rescuable.sol` contract. > Users sending ETH/native tokens > If contracts allow users to send tokens acc111identally.

Support

FAQs

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

Give us feedback!