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

Lack of refund mechanism for excessive ETH sent by buyer

0xalipede

Summary

The current implementation of the TokenDivider::buyOrder() function does not refund any excessive ETH sent by the buyer. If the buyer sends more ETH than necessary to cover the price and associated fees, the contract will not return the excess amount to the buyer. This can result in unintended loss of funds for the buyer and can lead to potential trust issues with the contract.

Vulnerability Details

Link: https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L261-L307

The function buyOrder() accepts ETH payments from the buyer but lacks a mechanism to detect and refund excess ETH sent by the buyer. The current logic assumes the buyer sends an exact amount matching the order.price + fee, but there is no check for overpayment or refund process for surplus ETH.

Consider the follwoing example:

  • buyer sends 2 ETH = 2e18

  • order.price: 1.5 ETH = 1.5e18

  • fee: 1% of order.price = 1.5e16

  • sellerFee: 0.5% of order.price = 7.5e15

The seller receives 1.5e18 - 7.5e15 (sellerFee is substracted) and owner receives 1.5e16 ETH.

The leftover ETH is 2e18 - 1.5e18 - 1.5e16 = 0.485e18 = 0.485 ETH (including sellerFee which is substracted from the price and stays in the contract).

If the buyer sends 2.0 ETH (more than 1.515 ETH required), the transaction will not revert, but the excess 0.485 ETH (or 4.85e17) will be lost since the contract has no refund logic.

Even if this is a design choice, there is still no way to transfer the excessive ETH out of the TokenDivider contract, locking the ETH forever.

PoC

Add the following test to the TokenDividerTest.t.sol:

function test_BuyErc20KeepingExcessiveETH() public nftDivided {
ERC20Mock erc20Mock = ERC20Mock(
tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address
);
​
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1.5e18, AMOUNT);
vm.stopPrank();
​
uint256 balanceOfUSER2Before = USER2.balance;
uint256 fee = 1.5e18 / 100;
​
vm.prank(USER2);
tokenDivider.buyOrder{value: 2 ether}(0, USER);
​
assertEq(USER2.balance, balanceOfUSER2Before - 1.5e18 - fee);
}

The test reverts with => "Failing tests:
Encountered 1 failing test in test/unit/TokenDividerTest.t.sol:TokenDiverTest
[FAIL: assertion failed: 8000000000000000000 != 8485000000000000000] test_BuyErc20KeepingExcessiveETH() (gas: 1038447)" meaning that the user was not refunded for the excessive ETH.

Impact

This vulnerability can lead to unintended loss of funds for the buyer if they accidentally send more ETH than necessary. While the transaction will not revert (unless insufficient ETH is sent), the buyer will lose the extra ETH. This can harm the reputation of the contract and reduce trust in the system, especially in high-value transactions.

  • Excess ETH sent is lost, causing financial harm to the buyer.

  • The absence of a refund mechanism might discourage users from interacting with the contract.

Even if such behavior is intended, the excessive ETH it will be locked forever since there is no function to transfer the ETH out of the contract, leading to loss of funds.

Tools Used

  • Manual review

  • Foundry

Recommendations

Implement a refund mechanism that checks for excess ETH sent by the buyer and sends it back.

function buyOrder(uint256 orderIndex, address seller) external payable {
if(seller == address(0)) {
revert TokenDivider__InvalidSeller();
}
​
SellOrder memory order = s_userToSellOrders[seller][orderIndex];
​
uint256 fee = order.price / 100;
uint256 sellerFee = fee / 2;
// price + contract fee
uint256 requiredAmount = order.price + fee;
if(msg.value < requiredAmount) {
revert TokenDivider__InsuficientEtherForFees();
}
​
// Refund excess ETH if any
uint256 excessAmount = msg.value - requiredAmount;
if (excessAmount > 0) {
(bool success, ) = payable(msg.sender).call{value: excessAmount}("");
if (!success) {
revert TokenDivider__TransferFailed();
}
}
​
balances[msg.sender][order.erc20Address] += order.amount;
s_userToSellOrders[seller][orderIndex] = s_userToSellOrders[seller][s_userToSellOrders[seller].length - 1];
s_userToSellOrders[seller].pop();
​
emit OrderSelled(msg.sender, order.price);
​
// Send ETH to the seller (price - sellerFee)
(bool successToSeller, ) = payable(order.seller).call{value: (order.price - sellerFee)}("");
if(!successToSeller) {
revert TokenDivider__TransferFailed();
}
​
// Send fee to the contract owner
(bool successToOwner, ) = payable(owner()).call{value: fee}("");
if(!successToOwner) {
revert TokenDivider__TransferFailed();
}
​
// Transfer the ERC20 token to the buyer
IERC20(order.erc20Address).transfer(msg.sender, order.amount);
}

Updates

Lead Judging Commences

fishy Lead Judge 7 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.