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

Does not Refund the Remaining Ether When creating Taker.

0xhunter

Summary

Users will suffer a loss of funds because the remaining funds cannot be refunded.

Vulnerability Details

The createTaker function is called by a user to purchase points from a seller's offer. At that time, the user provides an offer address and the number of points they want to purchase.

function createTaker(address _offer, uint256 _points) external payable {
...
@ _depositTokenWhenCreateTaker(
platformFee,
depositAmount,
tradeTax,
makerInfo,
offerInfo,
tokenManager
);
​
...
}

createTaker() calls _depositTokenWhenCreateTaker() to deposit assets.

function _depositTokenWhenCreateTaker(){
...
transferAmount = transferAmount + platformFee + tradeTax;
​
tokenManager.tillIn{value: msg.value}(
_msgSender(),
makerInfo.tokenAddress,
transferAmount,
false
);
}

As you can see, user has to pay transferAmount of tillIn().However, in many cases, msg.value can be larger than transferAmount. The user can send more ether than the specified amount, or transferAmount can be smaller. For example, transferAmount can be smaller due to platformFee updates.

However, In the TokenManager.sol#tillIn() function, there is no part that returns the remaining ETH when msg.value >= _amount.

function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
SNIP...
​
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);
--> }
--> IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
--> _safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
SNIP...
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
}

The remaining ETH will be permanently locked in the TokenManager contract and the user will lose their funds.

Proof Of Code

function test_ask_offer_protected_eth_refund() public {
vm.startPrank(user);
​
preMarktes.createOffer{value: 0.012 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Protected
)
);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateUserPlatformFeeRate(
user,
2
)
vm.stopPrank();
​
vm.startPrank(user);
address offerAddr = GenerateAddress.generateOfferAddress(0);
preMarktes.createTaker{value: 0.005175 * 1e18}(offerAddr, 500);
assertEq(payable(address(tokenManager)).balance, 0);
vm.stopPrank();
}

Impact

Users will suffer a loss of funds because the remaining funds cannot be refunded.

Tools Used

Mannual Review

Recommendations

The TokenManager.sol#tillIn() is modified as follow.

function tillIn(
address _accountAddress,
address _tokenAddress,
uint256 _amount,
bool _isPointToken
)
external
payable
onlyRelatedContracts(tadleFactory, _msgSender())
onlyInTokenWhiteList(_isPointToken, _tokenAddress)
{
SNIP...
​
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);
}
​
+++ (bool success, ) = payable(_accountAddress).call{value: msg.value - _amount}("");
+++ require(success, "ETH transfer failed");
​
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
SNIP...
}
​
emit TillIn(_accountAddress, _tokenAddress, _amount, _isPointToken);
}
Updates

Lead Judging Commences

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