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

`TokenManager::tillIn` Does Not Refund Extra Native Token Sent

4rdiii

[M-02] TokenManager::tillIn Does Not Refund Extra Native Token Sent

Summary

The tillIn function within the TokenManager contract is designed to handle the process of depositing either native tokens or ERC20 tokens as collateral. However, it lacks proper handling for scenarios where the amount of Ether (msg.value) sent exceeds the specified _amount. Additionally, when dealing with ERC20 tokens, there is no check to prevent users from sending Ether alongside the ERC20 tokens, which can lead to unintended loss of funds.

Vulnerability Details

The tillIn function performs a check to ensure that the amount of Ether sent (msg.value) is less than or equal to the specified _amount. However, it does not implement a mechanism to refund the difference if msg.value is greater than _amount. Furthermore, when the collateral token is an ERC20 token, the function also does not verify whether Ether was sent alongside it, potentially allowing users to send Ether without realizing it.

function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
/// @notice return if amount is 0
if (_amount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
if (capitalPoolAddr == address(0x0)) {
revert Errors.ContractIsNotDeployed();
}
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @notice check msg value
* @dev if msg value is less than _amount, revert
* @dev wrap native token and transfer to capital pool
*/
@> if (msg.value < _amount) {
@> revert Errors.NotEnoughMsgValue(msg.value, _amount);
@> }
@> //e no Action done for case msg.value > _amount!
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
@> //e there is no check for msg.value == 0!
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
}

Impact

Users may inadvertently send more Ether than intended when interacting with the tillIn or PreMarket::createOffer function, especially when dealing with ERC20 tokens. Since the function does not refund the excess Ether or prevent the sending of Ether altogether in these cases, users could lose funds without realizing it.

Proof of Concept

The PoC tests were designed to validate the existence of these vulnerabilities and to demonstrate the impact of the proposed fixes.
Test for Excess Ether Not Refunded

function test_my_Excess_Eth_non_refundable() public {
vm.deal(user, 1 ether);
assertEq(address(user).balance, 1 ether);
​
vm.startPrank(user);
preMarktes.createOffer{value: 1 ether}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.8 ether,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
address _stock = GenerateAddress.generateStockAddress(0);
address _offer = GenerateAddress.generateOfferAddress(0);
preMarktes.closeOffer(_stock, _offer);
// tokenManager.withdraw(
// address(mockUSDCToken),
// TokenBalanceType.MakerRefund
// );
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
assertEq(address(tokenManager).balance, (1 - 0.8 * 1.2) * 1e18); // excess amount = msg.value - collateral_amount * collateral_rate
}

Test for ETH Sent Alongside ERC20 Tokens

function test_my_ETH_Sent_along_ERC20_is_Not_Refundable() public {
vm.deal(user, 1 ether);
assertEq(address(user).balance, 1 ether);
​
vm.prank(user);
preMarktes.createOffer{value: 1 ether}(
CreateOfferParams(
marketPlace,
address(mockUSDCToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
assertEq(address(tokenManager).balance, 1 ether);
vm.startPrank(user);
address _stock = GenerateAddress.generateStockAddress(0);
address _offer = GenerateAddress.generateOfferAddress(0);
preMarktes.closeOffer(_stock, _offer);
tokenManager.withdraw(
address(mockUSDCToken),
TokenBalanceType.MakerRefund
);
tokenManager.withdraw(address(weth9), TokenBalanceType.MakerRefund);
assertEq(address(tokenManager).balance, 1 ether);
}

Tools Used

Manual Review

Recommendations

To mitigate this issue, the tillIn function should be modified to include a refund mechanism for excess Ether and to add a check that prevents the sending of Ether when the collateral token is an ERC20 token. Here's how the revised function might look:
This revision ensures that excess Ether is refunded to the sender and prevents the sending of Ether when the collateral is an ERC20 token, thereby safeguarding users against unintended fund losses.

function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
/// @notice return if amount is 0
if (_amount == 0) {
return;
}
​
address capitalPoolAddr = tadleFactory.relatedContracts(
RelatedContractLibraries.CAPITAL_POOL
);
if (capitalPoolAddr == address(0x0)) {
revert Errors.ContractIsNotDeployed();
}
​
if (_tokenAddress == wrappedNativeToken) {
/**
* @dev token is native token
* @notice check msg value
* @dev if msg value is less than _amount, revert
* @dev wrap native token and transfer to capital pool
*/
if (msg.value < _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount);
+ } else if(msg.value > _amount){
+ payable(_msgSender()).call{value: _amount - msg.value}('')
+ }
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
+ if (msg.value > 0 ){
+ revert("ETH SENT WHILE COLLATERAL IS ERC20);
+ }
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
}
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!