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

[M-1] `TokenDivider::buyOrder` is not returning any left funds to the buyer, causing any additional sent funds to be locked.

pindarew

Description buyOrder function is used for buying orders that are listen for sell. When the buyer call this function, the eth or any token accepted to pay, is sent to the seller if the transfer executed correctly, then this contract, which has all the tokens, send the tokens to the msg.sender. There are some fees are collected to the owner of the contract.

Impact The function does not handle the scenario where the user sends more ETH (msg.value) than required for the transaction. If the user sends excess ETH, it is not returned, leading to overpayment. This could result in financial losses for users if they mistakenly send more ETH than needed for the swap.

Proof of Concepts

  1. Nft is being divided to ERC20s

  2. Then user can make selling order of this tokens for example order price will be X

  3. Then another user can buy this order sending few times more than X

  4. Sended value will not be returned to user and will locked in TokenDivider contract

Place this test inside of the TokenDividerTest.t.sol and run forge test --mt testbuyOrderSendMoreThanPrice -vvvv

function testbuyOrderSendMoreThanPrice() public nftDivided {
uint256 sellingPrice = AMOUNT;
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
vm.startPrank(USER);
​
erc20Mock.approve(address(tokenDivider), sellingPrice);
​
tokenDivider.sellErc20(address(erc721Mock), sellingPrice, 1e18);
​
uint256 fees = (sellingPrice / 100) / 2;
​
vm.stopPrank();
​
vm.prank(USER2);
​
uint256 user2BalanceBeforeBuying = USER2.balance;
tokenDivider.buyOrder{value: (STARTING_USER_BALANCE)}(0, USER);
uint256 user2BalanceAfterBuying = USER2.balance;
​
uint256 spentFunds = user2BalanceBeforeBuying - user2BalanceAfterBuying;
console.log("USER2 balance before: ", user2BalanceBeforeBuying);
console.log("USER2 balance after: ", user2BalanceAfterBuying);
console.log("USER2 paid: ", spentFunds);
console.log("For Order selling price: ", sellingPrice);
console.log("And fees: ", fees);
console.log("Funds locked in contract: ", spentFunds - (sellingPrice + fees));
// This is pure robbery, no funds are returned to the user after buying the Order
​
assert(spentFunds != sellingPrice + fees);
}

Recommended mitigation Consider adding some save transfer function like sendValue from OpenZeppelin at the end of the function for returning the funds left back.
Example:

+import {Address} from "@openzeppelin/contracts/utils/Address.sol";
​
contract TokenDivider is IERC721Receiver, Ownable {
+ using Address for address payable;
.
.
.
.
function buyOrder(uint256 orderIndex, address seller) external payable {
.
.
.
​
if (!success) {
revert TokenDivider__TransferFailed();
}
​
(bool taxSuccess,) = payable(owner()).call{value: fee}("");
​
if (!taxSuccess) {
revert TokenDivider__TransferFailed();
}
​
+ payable(msg.sender).sendValue(msg.value - (order.price + sellerFee));
​
IERC20(order.erc20Address).transfer(msg.sender, order.amount);
}
​
Updates

Lead Judging Commences

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