Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Users will lose excess ETH sent to `TokenManager::tillIn()`

4gontuk

Summary

Vulnerability Detail

The TokenManager contract implements a tillIn() function that allows users to deposit tokens, including wrapped ETH (WETH), into a capital pool. This function is designed to handle both ERC20 tokens and native ETH transactions. When dealing with native ETH, the function wraps it into WETH before transferring to the capital pool.

The tillIn() function includes a check to ensure that the user has sent enough ETH to cover the specified amount:

if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}

However, the function does not handle the case where a user sends more ETH than required (msg.value > _amount). In such scenarios, the excess ETH becomes locked in the contract without any mechanism for retrieval.

The root cause of this issue lies in the incomplete handling of ETH value checks. While the function correctly reverts if insufficient ETH is sent, it fails to account for or refund excess ETH, leading to potential fund loss for users.

Relevant code:

if (_tokenAddress == wrappedNativeToken) {
if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
}

Impact

Users who inadvertently send more ETH than the specified _amount will permanently lose access to their excess funds. This issue could lead to significant financial losses for users, especially if large transactions are involved.

The severity of this issue is heightened by the fact that users might not immediately realize they've sent excess ETH, as the transaction would complete successfully. This could result in a loss of trust in the protocol and potential reputational damage.

Moreover, the locked ETH accumulates in the contract over time, creating a honeypot that could attract malicious actors to attempt to exploit the contract to access these funds.

Proof of Concept

  1. Alice intends to deposit 1 ETH into the capital pool using the tillIn() function.

  2. Due to a frontend error or user mistake, Alice sends 2 ETH along with the transaction.

  3. The tillIn() function processes the transaction:

    • It checks if msg.value < _amount, which passes as 2 ETH > 1 ETH.

    • It wraps 1 ETH into WETH and transfers it to the capital pool.

    • The excess 1 ETH remains in the contract.

  4. The transaction completes successfully, but Alice has permanently lost access to 1 ETH.

Tools Used

Manual review

Recommended Mitigation Steps

Implement a mechanism to refund any excess ETH sent by users. This can be achieved by modifying the tillIn() function to check for excess ETH and return it to the sender.

Here's a suggested fix:

function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
// ... existing code ...
​
if (_tokenAddress == wrappedNativeToken) {
if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
}
+ uint256 excessEth = msg.value - _amount;
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
+ if (excessEth > 0) {
+ (bool success, ) = msg.sender.call{value: excessEth}("");
+ require(success, "ETH refund failed");
+ }
} else {
// ... existing code for ERC20 tokens ...
}
​
// ... rest of the function ...
}

This modification ensures that any excess ETH is returned to the sender, preventing unintentional loss of funds.

Updates

Lead Judging Commences

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