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

ETH sent with Non-WETH Token passed as `CreateOfferParams.tokenAddress` parameter to `PreMarkets::createOffer()` function Results in Loss of ETH Funds when user tries to withdraw with `TokenManager::withdraw()` function.

justawanderkid

Description

The PreMarkets::createOffer() function in the PreMarktes contract is payable which allows users to send Ether via the tillIn() function which is called inside the PreMarkets::createOffer() function, which basically forward msg.value Sent towards TokenManager Contract. problem arises when we pass a token address other than WETH (Wrapped Ether) to the PreMarkets::createOffer(CreateOfferParams calldata params) and also Send ETH at same time when creating a offer. This can lead to the Ether being locked in the contract and user unable to withdraw full amount deposited when creating a offer after closing the offer`.

Impact

in this case if we pass any other address to CreateOfferParams.tokenAddress than WETH and also try to send ETH at the same time, the user will pay more than required amount to create offer and also if user decides to close the offer and withdraw funds, user will receive less than expected amount.

For example, if we create an offer with 0.01 * 1e18 USDC and 1 ether, we expect to withdraw slightly less than the deposited amount. However, the 1 ether remains in the TokenManager contract and is not retrievable after closing the offer. The user will only receive USDC back, even though the offer was created with both 1 ether and 0.01 * 1e18 USDC.

Proof of Concept

  1. User calls PreMarkets::createOffer() function with CreateOfferParams.tokenAddress set to a non-WETH token address (in this example we use USDC) and sends 1 ether at the same
    time.

  2. The tillIn() function inside the PreMarkets::createOffer() function sends our ETH and non-WETH token to TokenManager contract.

  3. User decides to close the offer, so calls the PreMarkets::closeOffer() function which then increases user TokenManagerStorage::userTokenBalanceMap mapping value of the user, which
    enables user to withdraw.

  4. then user calls TokenManager::withdraw() function, to get his 1 ether and USDC amount sent back when he created the offer.

  5. User successfully receives the Correct USDC amount but User Does not receieve any ether and the 1 ether we sent to TokenManager contract, will stay there and cannot do anything about it to get it back.

function test_user_creates_offer_with_passing_nonWETH_tokenAddress_plus_sendingETH() external {
vm.startPrank(user);
vm.deal(user, 1 ether); // fund the user with `1 ether`.
​
address offerAddr = GenerateAddress.generateOfferAddress(preMarktes.offerId()); // our offerId will be `0`
address stockAddr = GenerateAddress.generateStockAddress(preMarktes.offerId());
​
// get user USDC and native ETH balance, because we will have to assertions later on this test.
uint256 usdcBalanceOfuser_before = mockUSDCToken.balanceOf(user);
uint256 ethBalanceOfuser_before = user.balance;
​
// create offer
preMarktes.createOffer{value: 1 ether}( // we create offer by sending `1 ether` and `0.01 * 1e18` USDC.
CreateOfferParams(
marketPlace, // marketPlace
address(mockUSDCToken), // tokenAddress is USDC.
1000, // points
0.01 * 1e18, // amount USDC we send to `TokenManager` contract when creating a offer.
12000, // collateralRate
300, // eachTradeTax
OfferType.Ask, // offerType 0.Ask/Sell 1.Bid/Buy
OfferSettleType.Turbo // isTurboMode 0.Protected 1.Turbo
)
);
​
// check user balances
uint256 usdc_transferedAmount = OfferLibraries.getDepositAmount( OfferType.Ask, 12000, 0.01 * 1e18, true, Math.Rounding.Ceil );
assertEq(usdcBalanceOfuser_before - usdc_transferedAmount, mockUSDCToken.balanceOf(user));
assertEq(ethBalanceOfuser_before - 1 ether, user.balance);
​
// we close offer here. after closing offer, user needs to call `withdraw()` function in `TokenManager` contract.
preMarktes.closeOffer(stockAddr, offerAddr);
​
// get `capitalPool` address and call `CapitalPool::approve()` to approve `TokenManager` contract to be able to user safeTransferFrom(capitalPool, user, amount).
address capitalPoolAddr = tadleFactory.relatedContracts(RelatedContractLibraries.CAPITAL_POOL);
ICapitalPool(capitalPoolAddr).approve(address(mockUSDCToken));
​
// expected USDC amount the user will receive after withdrawal.
uint256 expectedUsdc_refundAmount = tokenManager.userTokenBalanceMap(user, address(mockUSDCToken), TokenBalanceType.MakerRefund);
​
console2.log("current user balanceOf USDC before withdrawing: ", mockUSDCToken.balanceOf(user));
console2.log("user balance of USDC after withdrawal should be: ", mockUSDCToken.balanceOf(user) + expectedUsdc_refundAmount);
console2.log("");
console2.log("current user ETH balance before withdrawing: ", user.balance);
console2.log("user ETH balance after withdrawal should be: : ", user.balance + 1 ether);
console2.log("");
​
// user withdraws.
tokenManager.withdraw(address(mockUSDCToken), TokenBalanceType.MakerRefund);
​
​
console2.log("current user balanceOf USDC after withdrawing: ", mockUSDCToken.balanceOf(user));
console2.log("current user ETH balance after withdrawing: ", user.balance);
​
assertEq(mockUSDCToken.balanceOf(user), usdcBalanceOfuser_before); // this line works fine.
assertEq(address(tokenManager).balance, 1 ether); // our `1 ether` which we sent to `TokenManager` contract when creating offer, it's still in there and it's not been refunded back to us.
assertFalse(user.balance == 1 ether); // user balance should be 1 ether if withdrawal counted the fact that we also sent ether along with usdc. since it's not, the return value will be `false`.
​
vm.stopPrank();
}

run the test with following command:

forge test --match-test test_user_creates_offer_with_passing_nonWETH_tokenAddress_plus_sendingETH -vvvv

take a look at the Logs:

Logs:
current user balanceOf USDC before withdrawing: 99999999988000000000000000
user balance of USDC after withdrawal should be: 100000000000000000000000000
current user ETH balance before withdrawing: 0
user ETH balance after withdrawal should be: : 1000000000000000000
current user balanceOf USDC after withdrawing: 100000000000000000000000000
current user ETH balance after withdrawing: 0

Recommended Mitigation

Add a check to ensure Ether is only sent when the specified token address is WETH.

function createOffer(CreateOfferParams calldata params) external payable {
/**
* @dev points and amount must be greater than 0
* @dev eachTradeTax must be less than 100%, decimal scaler is 10000
* @dev collateralRate must be more than 100%, decimal scaler is 10000
*/
if (params.points == 0x0 || params.amount == 0x0) {
revert Errors.AmountIsZero();
}
​
if (params.eachTradeTax > Constants.EACH_TRADE_TAX_DECIMAL_SCALER) {
revert InvalidEachTradeTaxRate();
}
​
if (params.collateralRate < Constants.COLLATERAL_RATE_DECIMAL_SCALER) {
revert InvalidCollateralRate();
}
​
+ if (msg.value > 0 && params.tokenAddress != address(WETH)) revert("Ether can only be sent with WETH token address");
... // rest of the code

This check ensures Ether is only sent with WETH, preventing loss of funds due to incorrect token handling.

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.