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

`PreMarkets` Payable Functions Will not Credit or Refund a User When Calling it With a Miscalculated `msg.value`

izuman

Summary

This value will likely be calculated on the front-end of the website and it will cause a loss in user funds.

All, functions marked as payable function in the PreMarket contract are subject to this vulnerability.

Vulnerability Details

For example,
PreMarkets::createOffer takes an uint256 amount parameter in the CreateOfferParams struct. When using weth as the payment/collateral method it only verifies that msg.value is not less than amount or it will revert. This is bad because if the front-end of the website calculates the incorrect msg.value to send with the transaction the extraAmount = msg.value - amount all the extraAmount will be lost in the contract.

#PoC
Place the following code into PreMarkets.t.sol and run with forge test --mt test_wrong_msgvalue_loses_funds -vvv
Notice the console logged data.

function test_wrong_msgvalue_loses_funds() public {
vm.startPrank(user);
console.log("Balancer before: ", address(user).balance);
​
preMarktes.createOffer{value: 0.03 * 1e18}(
CreateOfferParams(
marketPlace, address(weth9), 1000, 0.01 * 1e18, 12000, 300, OfferType.Bid, OfferSettleType.Turbo
)
);
vm.stopPrank();
​
vm.startPrank(user1);
deal(address(weth9), address(user1), 1e18);
deal(address(user1), 1e18);
weth9.approve(address(tokenManager), type(uint256).max);
address stockAddr = GenerateAddress.generateStockAddress(0);
address offerAddr = GenerateAddress.generateOfferAddress(0);
​
address stockAddr1 = GenerateAddress.generateStockAddress(1);
vm.stopPrank();
​
vm.prank(user1);
systemConfig.updateMarket("Backpack", address(mockPointToken), 0.01 * 1e18, block.timestamp - 1, 3600);
​
vm.startPrank(user);
​
deliveryPlace.closeBidOffer(offerAddr);
console.log("Balancer after: ", address(user).balance);
​
​
TokenBalanceType tokenBalanceType = TokenBalanceType.TaxIncome;
// Get the amount they should be giving us
uint256 claimAbleAmount = tokenManager.userTokenBalanceMap(address(user), address(weth9), tokenBalanceType);
​
console2.log("Trade tax is:", claimAbleAmount);
console2.log(
"claimable amount is: ",
tokenManager.userTokenBalanceMap(address(user), address(weth9), TokenBalanceType.MakerRefund)
);
console2.log(
"Remaining Cash: ",
tokenManager.userTokenBalanceMap(address(user), address(weth9), TokenBalanceType.RemainingCash)
);
console2.log(
"Sales Revenue: ",
tokenManager.userTokenBalanceMap(address(user), address(weth9), TokenBalanceType.SalesRevenue)
);
console2.log(
"Point Token: ",
tokenManager.userTokenBalanceMap(address(user), address(weth9), TokenBalanceType.PointToken)
);
console2.log(
"Referral Bonus : ",
tokenManager.userTokenBalanceMap(address(user), address(weth9), TokenBalanceType.ReferralBonus)
);
​
vm.stopPrank();
}

Impact

If the Tadle front-end over calculates the msg.value in any of the PreMarkets payable functions user's will lose funds.

Tools Used

Foundry and manual review

Recommendations

In the PreMarkets::createOffer function make the following changes, so even if the msg.value is over calculated users will still only deposit the amount they passed as a parameter. Thus, protecting them from unexpected losses.

- tokenManager.tillIn{value: msg.value}(
+ tokenManager.tillIn{value: msg.value != 0 ? transferAmount : 0}(
_msgSender(),
params.tokenAddress,
transferAmount,
false
);

In the PreMarkets::_depositTokenWhenCreateTaker:

- tokenManager.tillIn{value: msg.value}(_msgSender(), makerInfo.tokenAddress, transferAmount, false);
+ tokenManager.tillIn{value: msg.value != 0 ? transferAmount : 0}(_msgSender(), makerInfo.tokenAddress, transferAmount, false)
​

In the PreMarkets::listOffer:

- tokenManager.tillIn{value: msg.value}(_msgSender(), makerInfo.tokenAddress, transferAmount, false);
+ tokenManager.tillIn{value: msg.value != 0 ? transferAmount : 0}(_msgSender(), makerInfo.tokenAddress, transferAmount, false);

In the PreMarkets::relistOffer:

- tokenManager.tillIn{value: msg.value}(_msgSender(), makerInfo.tokenAddress, depositAmount, false);
+ tokenManager.tillIn{value: msg.value != 0 ? transferAmount : 0}(_msgSender(), makerInfo.tokenAddress, transferAmount, false);
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.