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

The `msg.value` check in the `_tokenAddress == wrappedNativeToken` branch of the `TokenManager::tillIn()` function is incorrect, which may cause user losses.

joicygiore

Summary

The msg.value check in the _tokenAddress == wrappedNativeToken branch of the TokenManager::tillIn() function is incorrect, which may cause user losses.

Vulnerability Details

In the TokenManager::tillIn() function, as indicated by the @> mark in the code snippet, the function only reverts when msg.value < _amount. If a user mistakenly sends an amount greater than _amount while using ETH to call functions like PreMarkets::listOffer() or PreMarkets::createOffer(), the excess ETH will remain in the TokenManager contract, leading to potential user losses.

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
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
// SNIP...
}

Poc

To demonstrate the issue, add the following test code to test/PreMarkets.t.sol and run it:

function testTokenManager_tillIn_eth_error() public {
console2.log("ETH balance at the start of address (tokenManager)",address(tokenManager).balance);
/////////////////////////////////
// user createOffer with eth //
/////////////////////////////////
vm.startPrank(user);
// In the correct case, value == 0.012 * 1e18
preMarktes.createOffer{value: 0.12 * 1e18}(
CreateOfferParams(
marketPlace,
address(weth9),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
deal(user2, 2e18);
vm.stopPrank();
address userOfferAddr = GenerateAddress.generateOfferAddress(0);
/////////////////////////////////
// user createTaker with eth //
/////////////////////////////////
vm.prank(user2);
// In the correct case, value == 0.01035 * 1e18
preMarktes.createTaker{value: 0.1035 * 1e18}(userOfferAddr, 1000);
vm.stopPrank();
​
address user2StockAddr = GenerateAddress.generateStockAddress(1);
​
////////////////////////
// admin updateMarket //
////////////////////////
vm.prank(user1);
systemConfig.updateMarket(
"Backpack",
address(mockPointToken),
0.01 * 1e18,
block.timestamp - 1,
3600
);
​
/////////////////////////
// user settleAskMaker //
/////////////////////////
vm.startPrank(user);
mockPointToken.approve(address(tokenManager), 10000 * 10 ** 18);
deliveryPlace.settleAskMaker(userOfferAddr, 1000);
vm.stopPrank();
​
/////////////////////////
// user closeBidTaker //
/////////////////////////
vm.prank(user2);
deliveryPlace.closeBidTaker(user2StockAddr);
​
​
console2.log("weth9.balanceOf(address(capitalPool)):",weth9.balanceOf(address(capitalPool)));
console2.log("ETH balance at the end of address(tokenManager):",address(tokenManager).balance);
}
// [PASS] testTokenManager_tillIn_eth_error() (gas: 1199801)
// Logs:
// ETH balance at the start of address (tokenManager) 0
// weth9.balanceOf(address(capitalPool)): 22350000000000000
// ETH balance at the end of address(tokenManager): 201150000000000000
// 201150000000000000 == 0.12e18 + 0.1035e18 - 22350000000000000

From the output, we can see that the excess ETH remains in the TokenManager contract address, leading to potential losses for users.

Code Snippet

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L47-L103

Impact

The incorrect msg.value check in the _tokenAddress == wrappedNativeToken branch of the TokenManager::tillIn() function can cause users to lose excess ETH, which remains locked in the contract.

Tools Used

Manual Review

Recommendations

Consider making the following changes:

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) {
+ if (msg.value != _amount) {
revert Errors.NotEnoughMsgValue(msg.value, _amount); // The error code should be modified at the same time
}
IWrappedNativeToken(wrappedNativeToken).deposit{value: _amount}();
_safe_transfer(wrappedNativeToken, capitalPoolAddr, _amount);
} else {
/// @notice token is ERC20 token
_transfer(
_tokenAddress,
_accountAddress,
capitalPoolAddr,
_amount,
capitalPoolAddr
);
}
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.