Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Overpayment mishandling in `TokenDivider::buyOrder` resulting in ether loss for users.

cyberray

Description The TokenDivider contract currently handles Ether payments for ERC-20 token purchases but fails to manage overpayment situations effectively. The TokenDivider::buyOrder function in the contract accepts Ether via msg.value to facilitate the purchase of ERC-20 tokens from a seller. However, buyOrder lacks the check for if the user sends more ether than required.
When a user overpays (sending more Ether than the required amount), the contract processes the transaction without returning the excess Ether to the user.

Impact When a user sends a higher value of Ether than what is necessary for the transaction (i.e., the price of the tokens plus any associated fees) by calling buyOrder, the contract only processes the token transfer and related fee deductions but does not handle the excess Ether. This results in the user losing the excess Ether, which is never refunded or handled by the contract.

Since there is no built-in mechanism to handle overpayments, the contract does not offer any way for users to recover their lost funds.

Proof of Concepts

  1. USER sells tokens at a price of 1 ETH. The contract includes a fee of 0.01%

  2. USER2 sends 3 ETH to purchase the tokens, which are priced at 1 ETH plus the fee

  3. USER2 should receive the requested tokens, and the contract should only deduct 1 ETH for the tokens and a small fee. The excess Ether (1.99 ETH) should be refunded to USER2.
    However, the contract fails to return the overpaid Ether to USER2. Instead, USER2 experiences a financial loss by losing the excess Ether.

Insert the following test in TokenDividerTest.t.sol:

function testMishandelingEthOverpayment() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
uint256 ownerBalanceBefore = address(tokenDivider.owner()).balance;
uint256 userBalanceBefore = address(USER).balance;
uint256 user2BalanceBefore = address(USER2).balance; // user2 ether balance
​
vm.startPrank(USER);
​
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), AMOUNT, 1e18); // Create a sell order for 1 ETH
​
uint256 fees = AMOUNT / 100;
vm.stopPrank();
​
vm.prank(USER2);
tokenDivider.buyOrder{value: (3e18)}(0, USER); // user2 sends 3e18 ether
​
uint256 ownerBalanceAfter = address(tokenDivider.owner()).balance;
uint256 userBalanceAfter = address(USER).balance;
uint256 user2BalanceAfter = address(USER2).balance;
​
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), 1e18);
​
uint256 expectedUser2BalanceAfter = user2BalanceBefore - (1e18 + fees / 2); // User should pay 1e18 + fees/2
assertEq(user2BalanceAfter, expectedUser2BalanceAfter, "USER2 Ether balance should reflect overpayment handling.");
​
assertEq(ownerBalanceAfter - fees, ownerBalanceBefore);
​
if(block.chainid != 31337) {
assertEq(userBalanceAfter - AMOUNT + fees / 2, userBalanceBefore);
}
}

the following test fails and returns:

[FAIL: USER2 Ether balance should reflect overpayment handling.: 7000000000000000000 != 8990000000000000000] testMishandelingERC20Overpayment() (gas: 1028828)

which proves that the user lost the excess ether he overpaid.

Recommended mitigation

You could implement a check to refund in case of overpayment:

​
+ uint256 totalRequiredAmount = order.price + sellerFee;
+ uint256 excessAmount = msg.value - totalRequiredAmount ;
+ if (excessAmount > 0) {
+ payable(msg.sender).transfer(excessAmount); // Refund excess Ether to the buyer
+ }

Alternatively, you can revert the transaction if an overpayment is detected:

+ error TokenDivider__ExcessivePaymentDetected();
​
+ if (msg.value > totalRequiredAmount) {
+ revert TokenDivider__ExcessivePaymentDetected();
+ }

The first solution (refund mechanism) ensures the user is refunded any excess funds, preventing financial loss. The second solution (revert mechanism) prevents overpayment from being accepted in the first place, ensuring that users cannot accidentally overpay.

Updates

Lead Judging Commences

juan_pedro_ventu Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Token misshandling

The extra eth sent by the user in the buy order will be locked in the contract forever

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.