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

[M-1] Incorrect Ether Handling in buyOrder Function

koinerdegen

Summary

The buyOrder function in the TokenDivider contract allows buyers to send any amount of Ether greater than the required order.price + sellerFee. However, the contract does not include logic to refund the excess Ether or revert the transaction, leading to potential financial loss for users.

Vulnerability Details

Affected Function:

function buyOrder(uint256 orderIndex, address seller) external payable {
if(seller == address(0)) {
revert TokenDivider__InvalidSeller();
}
​
SellOrder memory order = s_userToSellOrders[seller][orderIndex];
​
if(msg.value < order.price) {
revert TokenDivider__IncorrectEtherAmount();
}
​
uint256 fee = order.price / 100;
uint256 sellerFee = fee / 2;
​
if(msg.value < order.price + sellerFee) {
revert TokenDivider__InsuficientEtherForFees();
}
​
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);
(bool success, ) = payable(order.seller).call{value: (order.price - sellerFee)}("");
if(!success) {
revert TokenDivider__TransferFailed();
}
​
(bool taxSuccess, ) = payable(owner()).call{value: fee}("");
if(!taxSuccess) {
revert TokenDivider__TransferFailed();
}
​
IERC20(order.erc20Address).transfer(msg.sender, order.amount);
}

Issue:

  • The function only checks if the Ether sent is greater than or equal to order.price + sellerFee but does not handle scenarios where the buyer sends excess Ether.

  • The buyer may unintentionally overpay, resulting in loss of funds as the contract does not include logic to refund excess amounts.

Example Scenario:

If the required amount is 1 ETH and the buyer mistakenly sends 3 ETH, the contract will process the order, and the extra 1 ETH will be unrecoverable by the buyer.

function testCheckFees() public {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
vm.stopPrank();
​
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
vm.stopPrank();
​
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
assertEq(ERC20Mock(erc20Mock).balanceOf(address(tokenDivider)), AMOUNT);
assertEq(tokenDivider.getOrderPrice(USER, 0), 1e18);
​
uint256 balanceEtherOfUserBeforeBuy = USER2.balance;
console.log("[*] Balance ETH before buy: ", balanceEtherOfUserBeforeBuy);
​
vm.startPrank(USER2);
tokenDivider.buyOrder{value: 3e18}(0, USER);
vm.stopPrank();
uint256 balanceEtherOfUserAfterBuy = USER2.balance;
console.log("[*] Balance ETH after buy: ", balanceEtherOfUserAfterBuy);
​
assertEq(balanceEtherOfUserAfterBuy, balanceEtherOfUserBeforeBuy - 3e18);
​
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), AMOUNT);
assertEq(ERC20Mock(erc20Mock).balanceOf(address(tokenDivider)), 0);
​
}
[*] Balance ETH before buy: 10000000000000000000 // 10 ETH
[*] order price: 1000000000000000000 // 1 ETH
[*] fee: 10000000000000000 // 0.01 ETH
[*] sellerFee: 5000000000000000 // 0.005 ETH = 0.01 / 2
[*] order.price + sellerFee: 1005000000000000000 // 1.005
[*] Balance ETH after buy: 7000000000000000000 // 7 ETH

Impact

  • Financial Loss: Buyers can lose excess funds if they send more Ether than required.

  • User Experience: Poor UX as users may not realize they are overpaying without any refund mechanism.

  • Potential Legal/Reputational Risks: May lead to user complaints and loss of trust in the contract.

Tools Used

  • Manual Code Review

  • Static Analysis Tools

Recommendations

  1. Implement Refund Logic: Add logic to refund excess Ether to the sender.

    uint256 excessAmount = msg.value - (order.price + sellerFee);
    if (excessAmount > 0) {
    (bool refundSuccess, ) = payable(msg.sender).call{value: excessAmount}("");
    require(refundSuccess, "Refund failed");
    }

  2. Revert Transactions on Overpayment: Instead of processing the transaction, revert it if the sent amount does not exactly match the required value.

    require(msg.value == order.price + sellerFee, "Incorrect Ether amount");

  3. Enhance Documentation: Clearly document the expected behavior for users interacting with the function.

By implementing these recommendations, the contract can improve its security and usability, preventing unintended financial losses for users.

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.