Submission Details
Severity:
Potential loss of Ether due to overpayment and No means of refunding Buyer in such cases.
Summary
In the buyOrder function of the TokenDivider contract, there's no strict check on the amount of Ether sent (msg.value) to match exactly the sum of the order price and seller fee. However, there's no provision for refunding the buyer if they overpay, leading to potential loss of excess Ether.
Lines of code
https://github.com/Cyfrin/2025-01-pieces-protocol/blob/main/src/TokenDivider.sol#L293
Vulnerability Details
If user A has a sell order which price is 1 ether.
User B wanted to buy the Order, expected to pay 1 ether + the fee (0.5% of the price)
User B entered his/her totalBalance(10 ether) as the msg.value, 10 ether will deducted from him/her
Making him/her overpaying.
**Proof Of Concept**
https://github.com/Cyfrin/2025-01-pieces-protocol/blob/main/src/TokenDivider.sol#L261-L313
The code snippet below illustrates the issue:
```solidity
if(msg.value < order.price + sellerFee) {
revert TokenDivider__IncorrectEtherAmount();
}
```
Issue
The function doesn't strictly checks if the msg.value matches the exact cost using ` msg.value < order.price + sellerFee)`.
If the buyer accidentally sends more Ether `(msg.value > order.price + sellerFee)` than expected, there is no mechanism to refund the excess funds to the buyer.
This leads to potential loss of funds, as the excess amount is retained by the contract.
A test case
```solidity
function testBuyErc20() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
uint256 ownerBalanceBefore = address(tokenDivider.owner()).balance;
uint256 userBalanceBefore = address(USER).balance;
uint256 user2TokenBalanceBefore = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
uint256 user2BalanceBefore = address(USER2).balance;
console.log("user2BalanceBefore :", user2BalanceBefore);
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
uint256 price = 1e18;
tokenDivider.sellErc20(address(erc721Mock), price, AMOUNT); // Creamos una orden de venta por 1 ETH
uint256 fees = price / 100;
uint256 sellerFee = fees / 2;
vm.stopPrank();
vm.prank(USER2);
tokenDivider.buyOrder{value: (user2BalanceBefore)}(0, USER);
uint256 ownerBalanceAfter = address(tokenDivider.owner()).balance;
uint256 user2TokenBalanceAfter = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
uint256 userBalanceAfter = address(USER).balance;
uint256 user2BalanceAfter = address(USER2).balance;
//AMount the USER2 supposed to pay
console.log("AmountUser2IntendToPay :", price + fees / 2);
console.log("user2BalanceAfter :", user2BalanceAfter);
assertEq(user2TokenBalanceAfter - AMOUNT, user2TokenBalanceBefore);
assertEq(ownerBalanceAfter - fees, ownerBalanceBefore);
vm.expectRevert();
assertEq(user2BalanceAfter, user2BalanceBefore - price - sellerFee, "All USER2 ETH gone");
if (block.chainid != 31337) {
assertEq(userBalanceAfter - AMOUNT + fees / 2, userBalanceBefore);
} else {
assertEq(user2TokenBalanceAfter, 1e18);
}
}
```
the result
```solidity
Ran 1 test for test/unit/TokenDividerTest.t.sol:TokenDiverTest
[PASS] testBuyErc20() (gas: 1237861)
Logs:
user2BalanceBefore : 10000000000000000000
AmountUser2IntendToPay : 1005000000000000000
user2BalanceAfter : 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.46ms (4.73ms CPU time)
Ran 1 test suite in 49.65ms (12.46ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
```
Impact
In the `buyErc20()` function, there is no logic to handle cases where the buyer sends excess Ether compared to the order.price + sellerFee. This could lead to a situation where buyers lose funds inadvertently by overpaying without a means to refund, causing potential financial harm.
Tools Used
Manual Review and Foundry
Recommendations
1. To address this issue, modify the buyErc20() function to refund any excess Ether to the buyer if msg.value exceeds the cost.
2. Use stricty equality while checking for msg.value the buyer is using
Fixed Code:
```solidity
if(msg.value != order.price + sellerFee) {
revert TokenDivider__IncorrectEtherAmount();
}
```
Updates
Lead Judging Commences
fishy 5 months ago
Submission Judgement Published Assigned finding tags:
Token misshandling
The extra eth sent by the user in the buy order will be locked in the contract forever
Rewards breakdown
nSLOC
181This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.